From e73256fd2a49a4a0623add6dfb07c7edc102eeb6 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Mon, 25 Mar 2019 18:02:54 +0100 Subject: [PATCH] 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. --- include/proto/task.h | 11 ++++++++++- src/mux_h2.c | 4 ++-- src/task.c | 2 +- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/include/proto/task.h b/include/proto/task.h index 0f8017d1b..c876e7320 100644 --- 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 959bcfbc0..b6c2c8093 100644 --- 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 637e23551..7f34bc5a3 100644 --- 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;