BUG/MINOR: task: do not set TASK_F_USR1 for no reason

This applicationn specific flag was added in 2.4-dev by commit 6fa8bcdc7
("MINOR: task: add an application specific flag to the state: TASK_F_USR1")
to help preserve a the idle connections status across wakeup calls. While
the code to do this was OK for tasklets, it was wrong for tasks, as in an
effort not to lose it when setting the RUNNING flag (that tasklets don't
have), it ended up being inconditionally set. It just happens that for now
no regular tasks use it, only tasklets.

This fix makes sure we always atomically perform (state & flags | running)
there, using a CAS. It also does it for tasklets because it was possible
to lose some such flags if set by another thread, even though this should
not happen with current code. In order to make the code more readable (and
avoid the previous mistake of repeated flags in the bit field), a new
TASK_PERSISTENT aggregate was declared in task.h for this.

In practice the CAS is cheap here because task states are stable or
convergent so the loop will almost never be taken.

This should be backported to 2.4.

diff --git a/include/haproxy/task-t.h b/include/haproxy/task-t.h
index b68ae147099e20019d80908b33ef44a691349b36..9cdfa310b860aa8e98d5de3e1f741979835b5ce8 100644 (file)
--- a/include/haproxy/task-t.h
+++ b/include/haproxy/task-t.h
@@ -58,6 +58,9 @@
 #define TASK_F_USR1       0x00010000  /* preserved user flag 1, application-specific, def:0 */
 /* unused: 0x20000..0x80000000 */
 
+/* These flags are persistent across scheduler calls */
+#define TASK_PERSISTENT   (TASK_SHARED_WQ | TASK_SELF_WAKING | TASK_KILLED | \
+                           TASK_HEAVY | TASK_F_TASKLET | TASK_F_USR1)
 
 struct notification {
        struct list purge_me; /* Part of the list of signals to be purged in the

diff --git a/src/task.c b/src/task.c
index e2a0b236235b71e4183f81b08aba6f2d22e96e61..3b351a73fed5a112ba8b7447987f794618adb554 100644 (file)
--- a/src/task.c
+++ b/src/task.c
@@ -526,19 +526,18 @@ unsigned int run_tasks_from_lists(unsigned int budgets[])
                }
 
                budgets[queue]--;
-               t = (struct task *)LIST_ELEM(tl_queues[queue].n, struct tasklet *, list);
-               state = t->state & (TASK_SHARED_WQ|TASK_SELF_WAKING|TASK_HEAVY|TASK_F_TASKLET|TASK_KILLED|TASK_F_USR1|TASK_KILLED);
-
-               th_ctx->flags &= ~TH_FL_STUCK; // this thread is still running
                activity[tid].ctxsw++;
+
+               t = (struct task *)LIST_ELEM(tl_queues[queue].n, struct tasklet *, list);
                ctx = t->context;
                process = t->process;
                t->calls++;
                th_ctx->current = t;
+               th_ctx->flags &= ~TH_FL_STUCK; // this thread is still running
 
                _HA_ATOMIC_DEC(&th_ctx->rq_total);
 
-               if (state & TASK_F_TASKLET) {
+               if (t->state & TASK_F_TASKLET) {
                        uint64_t before = 0;
 
                        LIST_DEL_INIT(&((struct tasklet *)t)->list);
@@ -555,7 +554,7 @@ unsigned int run_tasks_from_lists(unsigned int budgets[])
 #endif
                        }
 
-                       state = _HA_ATOMIC_XCHG(&t->state, state);
+                       state = _HA_ATOMIC_FETCH_AND(&t->state, TASK_PERSISTENT);
                        __ha_barrier_atomic_store();
 
                        if (likely(!(state & TASK_KILLED))) {
@@ -582,7 +581,11 @@ unsigned int run_tasks_from_lists(unsigned int budgets[])
 
                LIST_DEL_INIT(&((struct tasklet *)t)->list);
                __ha_barrier_store();
-               state = _HA_ATOMIC_XCHG(&t->state, state|TASK_RUNNING|TASK_F_USR1);
+
+               state = t->state;
+               while (!_HA_ATOMIC_CAS(&t->state, &state, (state & TASK_PERSISTENT) | TASK_RUNNING))
+                       ;
+
                __ha_barrier_atomic_store();
 
                /* OK then this is a regular task */