Hi,

Unsurprisingly, the scheduler revamp that was committed last week has bugs.
The main issue is that threads could sleep while there are still tasks to
run, there are also fairness issues between the global runqueue and the local
one.
The attached patches should fix that.

Regards,

Olivier
>From f47ca20747c1cfc7b9e6413afe9c8819a84e485a Mon Sep 17 00:00:00 2001
From: Olivier Houchard <ohouch...@haproxy.com>
Date: Mon, 28 May 2018 13:51:06 +0200
Subject: [PATCH 1/3] BUG/MEDIUM: tasks: Don't forget to increase/decrease
 tasks_run_queue.

Don't forget to increase tasks_run_queue when we're adding a task to the
tasklet list, and to decrease it when we remove a task from a runqueue,
or its value won't be accurate, and could lead to tasks not being executed
when put in the global run queue.
---
 include/proto/task.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/proto/task.h b/include/proto/task.h
index c8004143b..ef296f6e9 100644
--- a/include/proto/task.h
+++ b/include/proto/task.h
@@ -177,6 +177,7 @@ static inline struct task *task_unlink_wq(struct task *t)
  */
 static inline struct task *__task_unlink_rq(struct task *t)
 {
+       HA_ATOMIC_SUB(&tasks_run_queue, 1);
        eb32sc_delete(&t->rq);
        if (likely(t->nice))
                HA_ATOMIC_SUB(&niced_tasks, 1);
@@ -219,6 +220,7 @@ static inline void task_insert_into_tasklet_list(struct 
task *t)
         */
        if (unlikely(!HA_ATOMIC_CAS(&t->rq.node.leaf_p, &expected, 0x1)))
                return;
+       HA_ATOMIC_ADD(&tasks_run_queue, 1);
        task_list_size[tid]++;
        tl = (struct tasklet *)t;
        LIST_ADDQ(&task_list[tid], &tl->list);
-- 
2.14.3

>From 4f28192f2cf16920fbe69e4077a8b948c3bd8f4b Mon Sep 17 00:00:00 2001
From: Olivier Houchard <ohouch...@haproxy.com>
Date: Mon, 28 May 2018 14:53:08 +0200
Subject: [PATCH 2/3] MINOR: task: Also consider the task list size when
 getting global tasks.

We're taking tasks from the global runqueue based on the number of tasks
the thread already have in its local runqueue, but now that we have a task
list, we also have to take that into account.
---
 src/task.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/task.c b/src/task.c
index 327518896..f6bf9156d 100644
--- a/src/task.c
+++ b/src/task.c
@@ -260,7 +260,7 @@ void process_runnable_tasks()
                 * much elements from the global list as to have a bigger local 
queue
                 * than the average.
                 */
-               while (rqueue_size[tid] <= average) {
+               while ((task_list_size[tid] + rqueue_size[tid]) <= average) {
 
                        /* we have to restart looking up after every batch */
                        rq_next = eb32sc_lookup_ge(&rqueue, rqueue_ticks - 
TIMER_LOOK_BACK, tid_bit);
-- 
2.14.3

>From beff1e28af2c1b7529dd94586f881ac81a04c797 Mon Sep 17 00:00:00 2001
From: Olivier Houchard <ohouch...@haproxy.com>
Date: Mon, 28 May 2018 14:54:49 +0200
Subject: [PATCH 3/3] BUG/MEDIUM: task: Don't forget to decrement max_processed
 after each task.

When the task list was introduced, we bogusly lost max_processed--, that means
we would execute as much tasks as present in the list, and we would never
set active_tasks_mask, so the thread would go to sleep even if more tasks were
to be executed.
---
 src/task.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/task.c b/src/task.c
index f6bf9156d..fb4840738 100644
--- a/src/task.c
+++ b/src/task.c
@@ -367,6 +367,7 @@ void process_runnable_tasks()
                                task_queue(t);
                }
 
+               max_processed--;
                if (max_processed <= 0) {
                        active_tasks_mask |= tid_bit;
                        activity[tid].long_rq++;
-- 
2.14.3

Reply via email to