BUG/MEDIUM: task/h2: add an idempotent task removal fucntion

Previous commit 3ea351368 ("BUG/MEDIUM: h2: Remove the tasklet from the
task list if unsubscribing.") uncovered an issue which needs to be
addressed in the scheduler's API. The function task_remove_from_task_list()
was initially designed to remove a task from the running tasklet list from
within the scheduler, and had to be used in h2 to abort pending I/O events.
However this function was not designed to be idempotent, occasionally
causing a double removal from the tasklet list, with the second doing
nothing but affecting the apparent tasks count and making haproxy use
100% CPU on some tests consisting in stopping the client during some
transfers. The h2_unsubscribe() function can sometimes be called upon
stream exit after an error where the tasklet was possibly already
removed, so it.

This patch does 2 things :
  - it renames task_remove_from_task_list() to
    __task_remove_from_tasklet_list() to discourage users from calling
    it. Also note the fix in the naming since it's a tasklet list and
    not a task list. This function is still uesd from the scheduler.
  - it adds a new, idempotent, task_remove_from_tasklet_list() function
    which does nothing if the task is already not in the tasklet list.

This patch will need to be backported where the commit above is backported.

diff --git a/include/proto/task.h b/include/proto/task.h
index 0f8017d1b14d909ca1151207b6c8c1608dd14879..c876e7320a7a060f5c12273a98c0d7abe40aed6d 100644 (file)
--- a/include/proto/task.h
+++ b/include/proto/task.h
@@ -271,7 +271,10 @@ static inline void task_insert_into_tasklet_list(struct task *t)
        LIST_ADDQ(&task_per_thread[tid].task_list, &tl->list);
 }
 
-static inline void task_remove_from_task_list(struct task *t)
+/* remove the task from the tasklet list. The task MUST already be there. If
+ * unsure, use task_remove_from_task_list() instead.
+ */
+static inline void __task_remove_from_tasklet_list(struct task *t)
 {
        LIST_DEL_INIT(&((struct tasklet *)t)->list);
        task_per_thread[tid].task_list_size--;
@@ -280,6 +283,12 @@ static inline void task_remove_from_task_list(struct task *t)
        _HA_ATOMIC_SUB(&tasks_run_queue, 1);
 }
 
+static inline void task_remove_from_tasklet_list(struct task *t)
+{
+       if (likely(!LIST_ISEMPTY(&((struct tasklet *)t)->list)))
+               __task_remove_from_tasklet_list(t);
+}
+
 /*
  * Unlinks the task and adjusts run queue stats.
  * A pointer to the task itself is returned.

diff --git a/src/mux_h2.c b/src/mux_h2.c
index 959bcfbc032493b2c044db7fc4833bfcf43c7d2c..b6c2c809347a98569a80d05c50243c4a4e2d5117 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -5182,7 +5182,7 @@ static int h2_unsubscribe(struct conn_stream *cs, int event_type, void *param)
                sw = param;
                if (h2s->send_wait == sw) {
                        sw->events &= ~SUB_CALL_UNSUBSCRIBE;
-                       task_remove_from_task_list((struct task *)h2s->send_wait->task);
+                       task_remove_from_tasklet_list((struct task *)h2s->send_wait->task);
                        h2s->send_wait = NULL;
                        LIST_DEL(&h2s->list);
                        LIST_INIT(&h2s->list);
@@ -5270,7 +5270,7 @@ static void h2_stop_senders(struct h2c *h2c)
 
        list_for_each_entry_safe(h2s, h2s_back, &h2c->sending_list, sending_list) {
                LIST_DEL_INIT(&h2s->sending_list);
-               task_remove_from_task_list((struct task *)h2s->send_wait->task);
+               task_remove_from_tasklet_list((struct task *)h2s->send_wait->task);
                h2s->send_wait->events |= SUB_RETRY_SEND;
                h2s->send_wait->events &= ~SUB_CALL_UNSUBSCRIBE;
        }

diff --git a/src/task.c b/src/task.c
index 637e23551225e36e49eda61acaca3e2f2fb46f88..7f34bc5a318fa5782489c7e6ab92454ee7f12978 100644 (file)
--- a/src/task.c
+++ b/src/task.c
@@ -416,7 +416,7 @@ void process_runnable_tasks()
                t = (struct task *)LIST_ELEM(task_per_thread[tid].task_list.n, struct tasklet *, list);
                state = _HA_ATOMIC_XCHG(&t->state, TASK_RUNNING);
                __ha_barrier_atomic_store();
-               task_remove_from_task_list(t);
+               __task_remove_from_tasklet_list(t);
 
                ctx = t->context;
                process = t->process;