RCU-tasks implementation

From: Steven Rostedt <[email protected]>

This commit exports the RCU-tasks APIs, call_rcu_tasks(), synchronize_rcu_tasks(), and rcu_barrier_tasks(), to GPL-licensed kernel modules.

Signed-off-by Steven Rostedt <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

Reviewed-by Josh Triplett <[email protected]>

---
 kernel/rcu/update.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/rcu/update.c +2 -1

index 4cece6e886ee..8f53a41dd9ee 100644
@@ -433,6 +433,7 @@ void synchronize_rcu_tasks(void)
        /* Wait for the grace period. */
        wait_rcu_gp(call_rcu_tasks);
 }
+EXPORT_SYMBOL_GPL(synchronize_rcu_tasks);
 
 /**
  * rcu_barrier_tasks - Wait for in-flight call_rcu_tasks() callbacks.

@@ -445,6 +446,7 @@ void rcu_barrier_tasks(void)
        /* There is only one callback queue, so this is easy.  ;-) */
        synchronize_rcu_tasks();
 }
+EXPORT_SYMBOL_GPL(rcu_barrier_tasks);
 
 /* See if tasks are still holding out, complain if so. */
 static void check_holdout_task(struct task_struct *t)
--

1.8.1.5

Quoted 5 lines · Paul E. McKenney

From: Steven Rostedt <[email protected]>

This commit exports the RCU-tasks APIs, call_rcu_tasks(), synchronize_rcu_tasks(), and rcu_barrier_tasks(), to GPL-licensed kernel modules.

Only two of these are being exported in this patch. Patch 1 is adding the export for call_rcu_tasks().

Quoted 30 lines

Signed-off-by Steven Rostedt <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

Reviewed-by Josh Triplett <[email protected]>

---
 kernel/rcu/update.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/rcu/update.c +2 -1

index 4cece6e886ee..8f53a41dd9ee 100644
@@ -433,6 +433,7 @@ void synchronize_rcu_tasks(void)
        /* Wait for the grace period. */
        wait_rcu_gp(call_rcu_tasks);
 }
+EXPORT_SYMBOL_GPL(synchronize_rcu_tasks);
 
 /**
  * rcu_barrier_tasks - Wait for in-flight call_rcu_tasks() callbacks.

@@ -445,6 +446,7 @@ void rcu_barrier_tasks(void)
        /* There is only one callback queue, so this is easy.  ;-) */
        synchronize_rcu_tasks();
 }
+EXPORT_SYMBOL_GPL(rcu_barrier_tasks);
 
 /* See if tasks are still holding out, complain if so. */
 static void check_holdout_task(struct task_struct *t)
--

1.8.1.5

Signature · 1 line

Pranith

From: "Paul E. McKenney" <[email protected]>

This commit adds torture tests for RCU-tasks. It also fixes a bug that would segfault for an RCU flavor lacking a callback-barrier function.

Signed-off-by Paul E. McKenney <[email protected]>

Reviewed-by Josh Triplett <[email protected]>

---
 include/linux/rcupdate.h |  1 +
 kernel/rcu/rcutorture.c  | 50 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

include/linux/rcupdate.h +1 -0

index e6aea256ad39..f504f797c9c8 100644
@@ -55,6 +55,7 @@ enum rcutorture_type {
        RCU_FLAVOR,
        RCU_BH_FLAVOR,
        RCU_SCHED_FLAVOR,
+       RCU_TASKS_FLAVOR,
        SRCU_FLAVOR,
        INVALID_RCU_FLAVOR
 };

kernel/rcu/rcutorture.c +29 -0

index febe07062ac5..52423f2c74da 100644
@@ -601,6 +601,52 @@ static struct rcu_torture_ops sched_ops = {
        .name           = "sched"
 };
 
+#ifdef CONFIG_TASKS_RCU
+
+/*
+ * Definitions for RCU-tasks torture testing.
+ */
+
+static int tasks_torture_read_lock(void)
+{
+       return 0;
+}
+
+static void tasks_torture_read_unlock(int idx)
+{
+}
+
+static void rcu_tasks_torture_deferred_free(struct rcu_torture *p)
+{
+       call_rcu_tasks(&p->rtort_rcu, rcu_torture_cb);
+}
+
+static struct rcu_torture_ops tasks_ops = {
+       .ttype          = RCU_TASKS_FLAVOR,
+       .init           = rcu_sync_torture_init,
+       .readlock       = tasks_torture_read_lock,
+       .read_delay     = rcu_read_delay,  /* just reuse rcu's version. */
+       .readunlock     = tasks_torture_read_unlock,
+       .completed      = rcu_no_completed,
+       .deferred_free  = rcu_tasks_torture_deferred_free,
+       .sync           = synchronize_rcu_tasks,

kernel/rcu/rcutorture.c +10 -2

+
+#else /* #ifdef CONFIG_TASKS_RCU */
+
+#define RCUTORTURE_TASKS_OPS
+
+#endif /* #else #ifdef CONFIG_TASKS_RCU */
+
 /*
  * RCU torture priority-boost testing.  Runs one real-time thread per
  * CPU for moderate bursts, repeatedly registering RCU callbacks and

@@ -1295,7 +1341,8 @@ static int rcu_torture_barrier_cbs(void *arg)
                if (atomic_dec_and_test(&barrier_cbs_count))
                        wake_up(&barrier_wq);
        } while (!torture_must_stop());
-       cur_ops->cb_barrier();
+       if (cur_ops->cb_barrier != NULL)
+               cur_ops->cb_barrier();
        destroy_rcu_head_on_stack(&rcu);
        torture_kthread_stopping("rcu_torture_barrier_cbs");
        return 0;

@@ -1534,6 +1581,7 @@ rcu_torture_init(void)
        int firsterr = 0;
        static struct rcu_torture_ops *torture_ops[] = {
                &rcu_ops, &rcu_bh_ops, &rcu_busted_ops, &srcu_ops, &sched_ops,
+               RCUTORTURE_TASKS_OPS
        };
 
        if (!torture_init_begin(torture_type, verbose, &rcutorture_runnable))
--

1.8.1.5

Quoted 65 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

This commit adds torture tests for RCU-tasks. It also fixes a bug that would segfault for an RCU flavor lacking a callback-barrier function.

Signed-off-by Paul E. McKenney <[email protected]>

Reviewed-by Josh Triplett <[email protected]>

---
 include/linux/rcupdate.h |  1 +
 kernel/rcu/rcutorture.c  | 50 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

include/linux/rcupdate.h +1 -0

index e6aea256ad39..f504f797c9c8 100644
@@ -55,6 +55,7 @@ enum rcutorture_type {
        RCU_FLAVOR,
        RCU_BH_FLAVOR,
        RCU_SCHED_FLAVOR,
+       RCU_TASKS_FLAVOR,
        SRCU_FLAVOR,
        INVALID_RCU_FLAVOR
 };

kernel/rcu/rcutorture.c +33 -0

index febe07062ac5..52423f2c74da 100644
@@ -601,6 +601,52 @@ static struct rcu_torture_ops sched_ops = {
        .name           = "sched"
 };
 
+#ifdef CONFIG_TASKS_RCU
+
+/*
+ * Definitions for RCU-tasks torture testing.
+ */
+
+static int tasks_torture_read_lock(void)
+{
+       return 0;
+}
+
+static void tasks_torture_read_unlock(int idx)
+{
+}
+
+static void rcu_tasks_torture_deferred_free(struct rcu_torture *p)
+{
+       call_rcu_tasks(&p->rtort_rcu, rcu_torture_cb);
+}
+
+static struct rcu_torture_ops tasks_ops = {
+       .ttype          = RCU_TASKS_FLAVOR,
+       .init           = rcu_sync_torture_init,
+       .readlock       = tasks_torture_read_lock,
+       .read_delay     = rcu_read_delay,  /* just reuse rcu's version. */
+       .readunlock     = tasks_torture_read_unlock,
+       .completed      = rcu_no_completed,
+       .deferred_free  = rcu_tasks_torture_deferred_free,
+       .sync           = synchronize_rcu_tasks,
+       .exp_sync       = synchronize_rcu_tasks,
+       .call           = call_rcu_tasks,
+       .cb_barrier     = rcu_barrier_tasks,
+       .fqs            = NULL,

kernel/rcu/rcutorture.c static struct rcu_torture_ops sched_ops = { · line 642

Quoted 6 lines

kernel/rcu/rcutorture.c +6 -0

+       .stats          = NULL,
+       .irq_capable    = 1,
+       .name           = "tasks"
+};
+
+#define RCUTORTURE_TASKS_OPS &tasks_ops,

Not sure about the comma here, no harm but still... a minor nit :)

Quoted 31 lines

kernel/rcu/rcutorture.c +10 -2

+
+#else /* #ifdef CONFIG_TASKS_RCU */
+
+#define RCUTORTURE_TASKS_OPS
+
+#endif /* #else #ifdef CONFIG_TASKS_RCU */
+
 /*
  * RCU torture priority-boost testing.  Runs one real-time thread per
  * CPU for moderate bursts, repeatedly registering RCU callbacks and

@@ -1295,7 +1341,8 @@ static int rcu_torture_barrier_cbs(void *arg)
                if (atomic_dec_and_test(&barrier_cbs_count))
                        wake_up(&barrier_wq);
        } while (!torture_must_stop());
-       cur_ops->cb_barrier();
+       if (cur_ops->cb_barrier != NULL)
+               cur_ops->cb_barrier();
        destroy_rcu_head_on_stack(&rcu);
        torture_kthread_stopping("rcu_torture_barrier_cbs");
        return 0;

@@ -1534,6 +1581,7 @@ rcu_torture_init(void)
        int firsterr = 0;
        static struct rcu_torture_ops *torture_ops[] = {
                &rcu_ops, &rcu_bh_ops, &rcu_busted_ops, &srcu_ops, &sched_ops,
+               RCUTORTURE_TASKS_OPS
        };
 
        if (!torture_init_begin(torture_type, verbose, &rcutorture_runnable))
--

1.8.1.5

Signature · 1 line

Pranith

Quoted 67 lines · Pranith Kumar

Quoted 65 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

This commit adds torture tests for RCU-tasks. It also fixes a bug that would segfault for an RCU flavor lacking a callback-barrier function.

Signed-off-by Paul E. McKenney <[email protected]>

Reviewed-by Josh Triplett <[email protected]>

---
 include/linux/rcupdate.h |  1 +
 kernel/rcu/rcutorture.c  | 50 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

include/linux/rcupdate.h +1 -0

index e6aea256ad39..f504f797c9c8 100644
@@ -55,6 +55,7 @@ enum rcutorture_type {
        RCU_FLAVOR,
        RCU_BH_FLAVOR,
        RCU_SCHED_FLAVOR,
+       RCU_TASKS_FLAVOR,
        SRCU_FLAVOR,
        INVALID_RCU_FLAVOR
 };

kernel/rcu/rcutorture.c +33 -0

index febe07062ac5..52423f2c74da 100644
@@ -601,6 +601,52 @@ static struct rcu_torture_ops sched_ops = {
        .name           = "sched"
 };
 
+#ifdef CONFIG_TASKS_RCU
+
+/*
+ * Definitions for RCU-tasks torture testing.
+ */
+
+static int tasks_torture_read_lock(void)
+{
+       return 0;
+}
+
+static void tasks_torture_read_unlock(int idx)
+{
+}
+
+static void rcu_tasks_torture_deferred_free(struct rcu_torture *p)
+{
+       call_rcu_tasks(&p->rtort_rcu, rcu_torture_cb);
+}
+
+static struct rcu_torture_ops tasks_ops = {
+       .ttype          = RCU_TASKS_FLAVOR,
+       .init           = rcu_sync_torture_init,
+       .readlock       = tasks_torture_read_lock,
+       .read_delay     = rcu_read_delay,  /* just reuse rcu's version. */
+       .readunlock     = tasks_torture_read_unlock,
+       .completed      = rcu_no_completed,
+       .deferred_free  = rcu_tasks_torture_deferred_free,
+       .sync           = synchronize_rcu_tasks,
+       .exp_sync       = synchronize_rcu_tasks,
+       .call           = call_rcu_tasks,
+       .cb_barrier     = rcu_barrier_tasks,
+       .fqs            = NULL,

Quoted 8 lines

kernel/rcu/rcutorture.c static struct rcu_torture_ops sched_ops = { · line 642

Quoted 6 lines

kernel/rcu/rcutorture.c +6 -0

+       .stats          = NULL,
+       .irq_capable    = 1,
+       .name           = "tasks"
+};
+
+#define RCUTORTURE_TASKS_OPS &tasks_ops,

Not sure about the comma here, no harm but still... a minor nit :)

Good point, it would be better to parenthesize this an put the comma at the point of use. Fixed!

Quoted 37 lines

Quoted 31 lines

kernel/rcu/rcutorture.c +10 -2

+
+#else /* #ifdef CONFIG_TASKS_RCU */
+
+#define RCUTORTURE_TASKS_OPS
+
+#endif /* #else #ifdef CONFIG_TASKS_RCU */
+
 /*
  * RCU torture priority-boost testing.  Runs one real-time thread per
  * CPU for moderate bursts, repeatedly registering RCU callbacks and

@@ -1295,7 +1341,8 @@ static int rcu_torture_barrier_cbs(void *arg)
                if (atomic_dec_and_test(&barrier_cbs_count))
                        wake_up(&barrier_wq);
        } while (!torture_must_stop());
-       cur_ops->cb_barrier();
+       if (cur_ops->cb_barrier != NULL)
+               cur_ops->cb_barrier();
        destroy_rcu_head_on_stack(&rcu);
        torture_kthread_stopping("rcu_torture_barrier_cbs");
        return 0;

@@ -1534,6 +1581,7 @@ rcu_torture_init(void)
        int firsterr = 0;
        static struct rcu_torture_ops *torture_ops[] = {
                &rcu_ops, &rcu_bh_ops, &rcu_busted_ops, &srcu_ops, &sched_ops,
+               RCUTORTURE_TASKS_OPS
        };
 
        if (!torture_init_begin(torture_type, verbose, &rcutorture_runnable))
--

1.8.1.5

Signature · 1 line

Pranith

From: "Paul E. McKenney" <[email protected]>

This commit adds a three-minute RCU-tasks stall warning. The actual time is controlled by the boot/sysfs parameter rcu_task_stall_timeout, with values less than or equal to zero disabling the stall warnings. The default value is three minutes, which means that the tasks that have not yet responded will get their stacks dumped every ten minutes, until they pass through a voluntary context switch.

Signed-off-by Paul E. McKenney <[email protected]>

---
 Documentation/kernel-parameters.txt |  5 +++++
 kernel/rcu/update.c                 | 27 ++++++++++++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

Documentation/kernel-parameters.txt +5 -0

index 910c3829f81d..8cdbde7b17f5 100644
@@ -2921,6 +2921,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
        rcupdate.rcu_cpu_stall_timeout= [KNL]
                        Set timeout for RCU CPU stall warning messages.
 
+       rcupdate.rcu_task_stall_timeout= [KNL]
+                       Set timeout in jiffies for RCU task stall warning
+                       messages.  Disable with a value less than or equal
+                       to zero.
+
        rdinit=         [KNL]
                        Format: <full_path>
                        Run specified binary instead of /init from the ramdisk,

kernel/rcu/update.c +24 -4

index 8f53a41dd9ee..f1535404a79e 100644
@@ -374,7 +374,7 @@ static DEFINE_RAW_SPINLOCK(rcu_tasks_cbs_lock);
 DEFINE_SRCU(tasks_rcu_exit_srcu);
 
 /* Control stall timeouts.  Disable with <= 0, otherwise jiffies till stall. */
-static int rcu_task_stall_timeout __read_mostly = HZ * 60 * 3;
+static int rcu_task_stall_timeout __read_mostly = HZ * 60 * 10;
 module_param(rcu_task_stall_timeout, int, 0644);
 
 /* Post an RCU-tasks callback. */

@@ -449,7 +449,8 @@ void rcu_barrier_tasks(void)
 EXPORT_SYMBOL_GPL(rcu_barrier_tasks);
 
 /* See if tasks are still holding out, complain if so. */
-static void check_holdout_task(struct task_struct *t)
+static void check_holdout_task(struct task_struct *t,
+                              bool needreport, bool *firstreport)
 {
        if (!ACCESS_ONCE(t->rcu_tasks_holdout) ||
            t->rcu_tasks_nvcsw != ACCESS_ONCE(t->nvcsw) ||

@@ -457,7 +458,15 @@ static void check_holdout_task(struct task_struct *t)
                ACCESS_ONCE(t->rcu_tasks_holdout) = 0;
                list_del_rcu(&t->rcu_tasks_holdout_list);
                put_task_struct(t);
+               return;
        }
+       if (!needreport)
+               return;
+       if (*firstreport) {
+               pr_err("INFO: rcu_tasks detected stalls on tasks:\n");
+               *firstreport = false;
+       }
+       sched_show_task(t);
 }
 
 /* RCU-tasks kthread that detects grace periods and invokes callbacks. */

@@ -465,6 +474,7 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 {
        unsigned long flags;
        struct task_struct *g, *t;
+       unsigned long lastreport;
        struct rcu_head *list;
        struct rcu_head *next;
        LIST_HEAD(rcu_tasks_holdouts);

@@ -543,13 +553,24 @@ static int __noreturn rcu_tasks_kthread(void *arg)
                 * of holdout tasks, removing any that are no longer
                 * holdouts.  When the list is empty, we are done.
                 */
+               lastreport = jiffies;
                while (!list_empty(&rcu_tasks_holdouts)) {
+                       bool firstreport;
+                       bool needreport;
+                       int rtst;
+
                        schedule_timeout_interruptible(HZ);
+                       rtst = ACCESS_ONCE(rcu_task_stall_timeout);
+                       needreport = rtst > 0 &&
+                                    time_after(jiffies, lastreport + rtst);
+                       if (needreport)
+                               lastreport = jiffies;
+                       firstreport = true;
                        WARN_ON(signal_pending(current));
                        rcu_read_lock();
                        list_for_each_entry_rcu(t, &rcu_tasks_holdouts,
                                                rcu_tasks_holdout_list)
-                               check_holdout_task(t);
+                               check_holdout_task(t, needreport, &firstreport);
                        rcu_read_unlock();
                }
 
--

1.8.1.5

Quoted 9 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

This commit adds a three-minute RCU-tasks stall warning. The actual time is controlled by the boot/sysfs parameter rcu_task_stall_timeout, with values less than or equal to zero disabling the stall warnings. The default value is three minutes, which means that the tasks that have not yet responded will get their stacks dumped every ten minutes, until they pass through a voluntary context switch.

Quoted 1 line

Signed-off-by Paul E. McKenney <[email protected]>

Something about 3 minutes and 10 minutes is mixed up here!

Quoted 97 lines

---
 Documentation/kernel-parameters.txt |  5 +++++
 kernel/rcu/update.c                 | 27 ++++++++++++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

Documentation/kernel-parameters.txt +5 -0

index 910c3829f81d..8cdbde7b17f5 100644
@@ -2921,6 +2921,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
        rcupdate.rcu_cpu_stall_timeout= [KNL]
                        Set timeout for RCU CPU stall warning messages.
 
+       rcupdate.rcu_task_stall_timeout= [KNL]
+                       Set timeout in jiffies for RCU task stall warning
+                       messages.  Disable with a value less than or equal
+                       to zero.
+
        rdinit=         [KNL]
                        Format: <full_path>
                        Run specified binary instead of /init from the ramdisk,

kernel/rcu/update.c +24 -4

index 8f53a41dd9ee..f1535404a79e 100644
@@ -374,7 +374,7 @@ static DEFINE_RAW_SPINLOCK(rcu_tasks_cbs_lock);
 DEFINE_SRCU(tasks_rcu_exit_srcu);
 
 /* Control stall timeouts.  Disable with <= 0, otherwise jiffies till stall. */
-static int rcu_task_stall_timeout __read_mostly = HZ * 60 * 3;
+static int rcu_task_stall_timeout __read_mostly = HZ * 60 * 10;
 module_param(rcu_task_stall_timeout, int, 0644);
 
 /* Post an RCU-tasks callback. */

@@ -449,7 +449,8 @@ void rcu_barrier_tasks(void)
 EXPORT_SYMBOL_GPL(rcu_barrier_tasks);
 
 /* See if tasks are still holding out, complain if so. */
-static void check_holdout_task(struct task_struct *t)
+static void check_holdout_task(struct task_struct *t,
+                              bool needreport, bool *firstreport)
 {
        if (!ACCESS_ONCE(t->rcu_tasks_holdout) ||
            t->rcu_tasks_nvcsw != ACCESS_ONCE(t->nvcsw) ||

@@ -457,7 +458,15 @@ static void check_holdout_task(struct task_struct *t)
                ACCESS_ONCE(t->rcu_tasks_holdout) = 0;
                list_del_rcu(&t->rcu_tasks_holdout_list);
                put_task_struct(t);
+               return;
        }
+       if (!needreport)
+               return;
+       if (*firstreport) {
+               pr_err("INFO: rcu_tasks detected stalls on tasks:\n");
+               *firstreport = false;
+       }
+       sched_show_task(t);
 }
 
 /* RCU-tasks kthread that detects grace periods and invokes callbacks. */

@@ -465,6 +474,7 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 {
        unsigned long flags;
        struct task_struct *g, *t;
+       unsigned long lastreport;
        struct rcu_head *list;
        struct rcu_head *next;
        LIST_HEAD(rcu_tasks_holdouts);

@@ -543,13 +553,24 @@ static int __noreturn rcu_tasks_kthread(void *arg)
                 * of holdout tasks, removing any that are no longer
                 * holdouts.  When the list is empty, we are done.
                 */
+               lastreport = jiffies;
                while (!list_empty(&rcu_tasks_holdouts)) {
+                       bool firstreport;
+                       bool needreport;
+                       int rtst;
+
                        schedule_timeout_interruptible(HZ);
+                       rtst = ACCESS_ONCE(rcu_task_stall_timeout);
+                       needreport = rtst > 0 &&
+                                    time_after(jiffies, lastreport + rtst);
+                       if (needreport)
+                               lastreport = jiffies;
+                       firstreport = true;
                        WARN_ON(signal_pending(current));
                        rcu_read_lock();
                        list_for_each_entry_rcu(t, &rcu_tasks_holdouts,
                                                rcu_tasks_holdout_list)
-                               check_holdout_task(t);
+                               check_holdout_task(t, needreport, &firstreport);
                        rcu_read_unlock();
                }
 
--

1.8.1.5

Signature · 1 line

Pranith

From: "Paul E. McKenney" <[email protected]>

The current RCU-tasks implementation uses strict polling to detect callback arrivals. This works quite well, but is not so good for energy efficiency. This commit therefore replaces the strict polling with a wait queue.

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcu/update.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

kernel/rcu/update.c +2 -0

index f1535404a79e..1256a900cd01 100644
@@ -368,6 +368,7 @@ early_initcall(check_cpu_stall_init);
 /* Global list of callbacks and associated lock. */
 static struct rcu_head *rcu_tasks_cbs_head;
 static struct rcu_head **rcu_tasks_cbs_tail = &rcu_tasks_cbs_head;
+static DECLARE_WAIT_QUEUE_HEAD(rcu_tasks_cbs_wq);
 static DEFINE_RAW_SPINLOCK(rcu_tasks_cbs_lock);
 
 /* Track exiting tasks in order to allow them to be waited for. */

@@ -381,13 +382,17 @@ module_param(rcu_task_stall_timeout, int, 0644);
 void call_rcu_tasks(struct rcu_head *rhp, void (*func)(struct rcu_head *rhp))
 {
        unsigned long flags;
+       bool needwake;
 
        rhp->next = NULL;

kernel/rcu/update.c +7 -3

@@ -498,8 +503,12 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 
                /* If there were none, wait a bit and start over. */
                if (!list) {
-                       schedule_timeout_interruptible(HZ);
-                       WARN_ON(signal_pending(current));
+                       wait_event_interruptible(rcu_tasks_cbs_wq,
+                                                rcu_tasks_cbs_head);
+                       if (!rcu_tasks_cbs_head) {
+                               WARN_ON(signal_pending(current));
+                               schedule_timeout_interruptible(HZ/10);
+                       }
                        continue;
                }
 

@@ -605,6 +614,7 @@ static int __noreturn rcu_tasks_kthread(void *arg)
                        list = next;
                        cond_resched();
                }
+               schedule_timeout_uninterruptible(HZ/10);
        }
 }
 
--

1.8.1.5

Quoted 31 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The current RCU-tasks implementation uses strict polling to detect callback arrivals. This works quite well, but is not so good for energy efficiency. This commit therefore replaces the strict polling with a wait queue.

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcu/update.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

kernel/rcu/update.c +2 -0

index f1535404a79e..1256a900cd01 100644
@@ -368,6 +368,7 @@ early_initcall(check_cpu_stall_init);
 /* Global list of callbacks and associated lock. */
 static struct rcu_head *rcu_tasks_cbs_head;
 static struct rcu_head **rcu_tasks_cbs_tail = &rcu_tasks_cbs_head;
+static DECLARE_WAIT_QUEUE_HEAD(rcu_tasks_cbs_wq);
 static DEFINE_RAW_SPINLOCK(rcu_tasks_cbs_lock);
 
 /* Track exiting tasks in order to allow them to be waited for. */

@@ -381,13 +382,17 @@ module_param(rcu_task_stall_timeout, int, 0644);
 void call_rcu_tasks(struct rcu_head *rhp, void (*func)(struct rcu_head *rhp))
 {
        unsigned long flags;
+       bool needwake;
 
        rhp->next = NULL;

kernel/rcu/update.c module_param(rcu_task_stall_timeout, int, 0644); · line 397

Quoted 10 lines

kernel/rcu/update.c +3 -0

        rhp->func = func;
        raw_spin_lock_irqsave(&rcu_tasks_cbs_lock, flags);
+       needwake = !rcu_tasks_cbs_head;
        *rcu_tasks_cbs_tail = rhp;
        rcu_tasks_cbs_tail = &rhp->next;
        raw_spin_unlock_irqrestore(&rcu_tasks_cbs_lock, flags);
+       if (needwake)
+               wake_up(&rcu_tasks_cbs_wq);
 }
 EXPORT_SYMBOL_GPL(call_rcu_tasks);

I think you want

needwake = !!rcu_tasks_cbs_head;

otherwise it will wake up when rcu_tasks_cbs_head is null, no?

Quoted 26 lines

kernel/rcu/update.c +7 -3

@@ -498,8 +503,12 @@ static int __noreturn rcu_tasks_kthread(void *arg)
 
                /* If there were none, wait a bit and start over. */
                if (!list) {
-                       schedule_timeout_interruptible(HZ);
-                       WARN_ON(signal_pending(current));
+                       wait_event_interruptible(rcu_tasks_cbs_wq,
+                                                rcu_tasks_cbs_head);
+                       if (!rcu_tasks_cbs_head) {
+                               WARN_ON(signal_pending(current));
+                               schedule_timeout_interruptible(HZ/10);
+                       }
                        continue;
                }
 

@@ -605,6 +614,7 @@ static int __noreturn rcu_tasks_kthread(void *arg)
                        list = next;
                        cond_resched();
                }
+               schedule_timeout_uninterruptible(HZ/10);
        }
 }
 
--

1.8.1.5

Signature · 1 line

Pranith

Quoted 46 lines · Pranith Kumar

Quoted 31 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The current RCU-tasks implementation uses strict polling to detect callback arrivals. This works quite well, but is not so good for energy efficiency. This commit therefore replaces the strict polling with a wait queue.

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcu/update.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

kernel/rcu/update.c +2 -0

index f1535404a79e..1256a900cd01 100644
@@ -368,6 +368,7 @@ early_initcall(check_cpu_stall_init);
 /* Global list of callbacks and associated lock. */
 static struct rcu_head *rcu_tasks_cbs_head;
 static struct rcu_head **rcu_tasks_cbs_tail = &rcu_tasks_cbs_head;
+static DECLARE_WAIT_QUEUE_HEAD(rcu_tasks_cbs_wq);
 static DEFINE_RAW_SPINLOCK(rcu_tasks_cbs_lock);
 
 /* Track exiting tasks in order to allow them to be waited for. */

@@ -381,13 +382,17 @@ module_param(rcu_task_stall_timeout, int, 0644);
 void call_rcu_tasks(struct rcu_head *rhp, void (*func)(struct rcu_head *rhp))
 {
        unsigned long flags;
+       bool needwake;
 
        rhp->next = NULL;

kernel/rcu/update.c module_param(rcu_task_stall_timeout, int, 0644); · line 397

Quoted 10 lines

kernel/rcu/update.c +3 -0

        rhp->func = func;
        raw_spin_lock_irqsave(&rcu_tasks_cbs_lock, flags);
+       needwake = !rcu_tasks_cbs_head;
        *rcu_tasks_cbs_tail = rhp;
        rcu_tasks_cbs_tail = &rhp->next;
        raw_spin_unlock_irqrestore(&rcu_tasks_cbs_lock, flags);
+       if (needwake)
+               wake_up(&rcu_tasks_cbs_wq);
 }
 EXPORT_SYMBOL_GPL(call_rcu_tasks);

I think you want

Quoted 3 lines

needwake = !!rcu_tasks_cbs_head;

otherwise it will wake up when rcu_tasks_cbs_head is null, no?

Well, that is exactly what we want. Note that we do the test -before- the enqueue. This means that we do the wakeup if the list -was- empty before the enqueue, which is exactly the case where the task might be asleep without having already been sent a wakeup.

Assuming that wakeups are reliably delivered, of course. But if they are not reliably delivered, that is a bug that needs to be fixed.

From: "Paul E. McKenney" <[email protected]>

It is expected that many sites will have CONFIG_TASKS_RCU=y, but will never actually invoke call_rcu_tasks(). For such sites, creating rcu_tasks_kthread() at boot is wasteful. This commit therefore defers creation of this kthread until the time of the first call_rcu_tasks().

This of course means that the first call_rcu_tasks() must be invoked from process context after the scheduler is fully operational.

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcu/update.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

kernel/rcu/update.c +11 -4

index 1256a900cd01..d997163c7e92 100644
@@ -378,7 +378,12 @@ DEFINE_SRCU(tasks_rcu_exit_srcu);
 static int rcu_task_stall_timeout __read_mostly = HZ * 60 * 10;
 module_param(rcu_task_stall_timeout, int, 0644);
 
-/* Post an RCU-tasks callback. */
+static void rcu_spawn_tasks_kthread(void);
+
+/*
+ * Post an RCU-tasks callback.  First call must be from process context
+ * after the scheduler if fully operational.
+ */
 void call_rcu_tasks(struct rcu_head *rhp, void (*func)(struct rcu_head *rhp))
 {
        unsigned long flags;

@@ -391,8 +396,10 @@ void call_rcu_tasks(struct rcu_head *rhp, void (*func)(struct rcu_head *rhp))
        *rcu_tasks_cbs_tail = rhp;
        rcu_tasks_cbs_tail = &rhp->next;
        raw_spin_unlock_irqrestore(&rcu_tasks_cbs_lock, flags);
-       if (needwake)
+       if (needwake) {
+               rcu_spawn_tasks_kthread();
                wake_up(&rcu_tasks_cbs_wq);
+       }
 }
 EXPORT_SYMBOL_GPL(call_rcu_tasks);
 

@@ -618,15 +625,27 @@ static int __noreturn rcu_tasks_kthread(void *arg)
        }
 }
 
-/* Spawn rcu_tasks_kthread() at boot time. */
-static int __init rcu_spawn_tasks_kthread(void)
+/* Spawn rcu_tasks_kthread() at first call to call_rcu_tasks(). */
+static void rcu_spawn_tasks_kthread(void)

kernel/rcu/update.c +1 -2

+       mutex_unlock(&rcu_tasks_kthread_mutex);
 }
-early_initcall(rcu_spawn_tasks_kthread);
 
 #endif /* #ifdef CONFIG_TASKS_RCU */
--

1.8.1.5

Quoted 59 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

It is expected that many sites will have CONFIG_TASKS_RCU=y, but will never actually invoke call_rcu_tasks(). For such sites, creating rcu_tasks_kthread() at boot is wasteful. This commit therefore defers creation of this kthread until the time of the first call_rcu_tasks().

This of course means that the first call_rcu_tasks() must be invoked from process context after the scheduler is fully operational.

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcu/update.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

kernel/rcu/update.c +14 -5

index 1256a900cd01..d997163c7e92 100644
@@ -378,7 +378,12 @@ DEFINE_SRCU(tasks_rcu_exit_srcu);
 static int rcu_task_stall_timeout __read_mostly = HZ * 60 * 10;
 module_param(rcu_task_stall_timeout, int, 0644);
 
-/* Post an RCU-tasks callback. */
+static void rcu_spawn_tasks_kthread(void);
+
+/*
+ * Post an RCU-tasks callback.  First call must be from process context
+ * after the scheduler if fully operational.
+ */
 void call_rcu_tasks(struct rcu_head *rhp, void (*func)(struct rcu_head *rhp))
 {
        unsigned long flags;

@@ -391,8 +396,10 @@ void call_rcu_tasks(struct rcu_head *rhp, void (*func)(struct rcu_head *rhp))
        *rcu_tasks_cbs_tail = rhp;
        rcu_tasks_cbs_tail = &rhp->next;
        raw_spin_unlock_irqrestore(&rcu_tasks_cbs_lock, flags);
-       if (needwake)
+       if (needwake) {
+               rcu_spawn_tasks_kthread();
                wake_up(&rcu_tasks_cbs_wq);
+       }
 }
 EXPORT_SYMBOL_GPL(call_rcu_tasks);
 

@@ -618,15 +625,27 @@ static int __noreturn rcu_tasks_kthread(void *arg)
        }
 }
 
-/* Spawn rcu_tasks_kthread() at boot time. */
-static int __init rcu_spawn_tasks_kthread(void)
+/* Spawn rcu_tasks_kthread() at first call to call_rcu_tasks(). */
+static void rcu_spawn_tasks_kthread(void)
 {
-       struct task_struct __maybe_unused *t;
+       static DEFINE_MUTEX(rcu_tasks_kthread_mutex);
+       static struct task_struct *rcu_tasks_kthread_ptr;
+       struct task_struct *t;
 

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 638

Quoted 4 lines

kernel/rcu/update.c +4 -0

+       if (ACCESS_ONCE(rcu_tasks_kthread_ptr)) {
+               smp_mb(); /* Ensure caller sees full kthread. */
+               return;
+       }

I don't see the need for this smp_mb(). The caller has already seen that rcu_tasks_kthread_ptr is assigned. What are we ensuring with this barrier again?

an smp_rmb() before this ACCESS_ONCE() and an smp_wmb() after assigning to rcu_tasks_kthread_ptr should be enough, right?

kernel/rcu/update.c static int __noreturn rcu_tasks_kthread(void *arg) · line 647

Quoted 10 lines

kernel/rcu/update.c +7 -1

+       mutex_lock(&rcu_tasks_kthread_mutex);
+       if (rcu_tasks_kthread_ptr) {
+               mutex_unlock(&rcu_tasks_kthread_mutex);
+               return;
+       }
        t = kthread_run(rcu_tasks_kthread, NULL, "rcu_tasks_kthread");
        BUG_ON(IS_ERR(t));
-       return 0;
+       smp_mb(); /* Ensure others see full kthread. */
+       ACCESS_ONCE(rcu_tasks_kthread_ptr) = t;

Isn't it better to reverse these two statements and change as follows?

ACCESS_ONCE(rcu_tasks_kthread_ptr) = t;
smp_wmb();

or

smp_store_release(rcu_tasks_kthread_ptr, t);

will ensure that this write to rcu_task_kthread_ptr is ordered with the previous read. I recently read memory-barriers.txt, so please excuse me if I am totally wrong. But I am confused! :(

Quoted 8 lines

kernel/rcu/update.c +1 -2

+       mutex_unlock(&rcu_tasks_kthread_mutex);
 }
-early_initcall(rcu_spawn_tasks_kthread);
 
 #endif /* #ifdef CONFIG_TASKS_RCU */
--

1.8.1.5

Signature · 1 line

Pranith

From: "Paul E. McKenney" <[email protected]>

Currently TASKS_RCU would ignore a CPU running a task in nohz_full= usermode execution. There would be neither a context switch nor a scheduling-clock interrupt to tell TASKS_RCU that the task in question had passed through a quiescent state. The grace period would therefore extend indefinitely. This commit therefore makes RCU's dyntick-idle subsystem record the task_struct structure of the task that is running in dyntick-idle mode on each CPU. The TASKS_RCU grace period can then access this information and record a quiescent state on behalf of any CPU running in dyntick-idle usermode.

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/init_task.h |  3 ++-
 include/linux/sched.h     |  2 ++
 kernel/rcu/tree.c         |  2 ++
 kernel/rcu/tree.h         |  2 ++
 kernel/rcu/tree_plugin.h  | 16 ++++++++++++++++
 kernel/rcu/update.c       |  4 +++-
 6 files changed, 27 insertions(+), 2 deletions(-)

include/linux/init_task.h +2 -1

index 78715ea7c30c..642828009324 100644
@@ -128,7 +128,8 @@ extern struct group_info init_groups;
 #define INIT_TASK_RCU_TASKS(tsk)                                       \
        .rcu_tasks_holdout = false,                                     \
        .rcu_tasks_holdout_list =                                       \
-               LIST_HEAD_INIT(tsk.rcu_tasks_holdout_list),
+               LIST_HEAD_INIT(tsk.rcu_tasks_holdout_list),             \
+       .rcu_tasks_idle_cpu = -1,
 #else
 #define INIT_TASK_RCU_TASKS(tsk)
 #endif

include/linux/sched.h +2 -0

index 3cf124389ec7..5fa041f7a034 100644
@@ -1277,6 +1277,7 @@ struct task_struct {
        unsigned long rcu_tasks_nvcsw;
        int rcu_tasks_holdout;
        struct list_head rcu_tasks_holdout_list;
+       int rcu_tasks_idle_cpu;
 #endif /* #ifdef CONFIG_TASKS_RCU */
 
 #if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)

@@ -2021,6 +2022,7 @@ static inline void rcu_copy_process(struct task_struct *p)
 #ifdef CONFIG_TASKS_RCU
        p->rcu_tasks_holdout = false;
        INIT_LIST_HEAD(&p->rcu_tasks_holdout_list);
+       p->rcu_tasks_idle_cpu = -1;
 #endif /* #ifdef CONFIG_TASKS_RCU */
 }
 

kernel/rcu/tree.c +2 -0

index 645a33efc0d4..0d9ee1e4f446 100644
@@ -526,6 +526,7 @@ static void rcu_eqs_enter_common(struct rcu_dynticks *rdtp, long long oldval,
        atomic_inc(&rdtp->dynticks);
        smp_mb__after_atomic();  /* Force ordering with next sojourn. */
        WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
+       rcu_dynticks_task_enter();
 
        /*
         * It is illegal to enter an extended quiescent state while

@@ -642,6 +643,7 @@ void rcu_irq_exit(void)
 static void rcu_eqs_exit_common(struct rcu_dynticks *rdtp, long long oldval,
                               int user)
 {
+       rcu_dynticks_task_exit();
        smp_mb__before_atomic();  /* Force ordering w/previous sojourn. */
        atomic_inc(&rdtp->dynticks);
        /* CPUs seeing atomic_inc() must see later RCU read-side crit sects */

kernel/rcu/tree.h +2 -0

index 0f69a79c5b7d..37ff593b7725 100644
@@ -579,6 +579,8 @@ static void rcu_sysidle_report_gp(struct rcu_state *rsp, int isidle,
 static void rcu_bind_gp_kthread(void);
 static void rcu_sysidle_init_percpu_data(struct rcu_dynticks *rdtp);
 static bool rcu_nohz_full_cpu(struct rcu_state *rsp);
+static void rcu_dynticks_task_enter(void);
+static void rcu_dynticks_task_exit(void);
 
 #endif /* #ifndef RCU_TREE_NONCORE */
 

kernel/rcu/tree_plugin.h +0 -0

index a86a363ea453..0d8ef5cb1976 100644

kernel/rcu/tree_plugin.h +9 -0

+}
+
+/* Record no current task on dyntick-idle exit. */
+static void rcu_dynticks_task_exit(void)
+{
+#if defined(CONFIG_TASKS_RCU) && defined(CONFIG_NO_HZ_FULL)
+       ACCESS_ONCE(current->rcu_tasks_idle_cpu) = -1;
+#endif /* #if defined(CONFIG_TASKS_RCU) && defined(CONFIG_NO_HZ_FULL) */
+}

kernel/rcu/update.c +0 -0

index d997163c7e92..a4140f25cf1a 100644

kernel/rcu/update.c +0 -1

                ACCESS_ONCE(t->rcu_tasks_holdout) = 0;
                list_del_rcu(&t->rcu_tasks_holdout_list);
                put_task_struct(t);
--

1.8.1.5

Quoted 94 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Currently TASKS_RCU would ignore a CPU running a task in nohz_full= usermode execution. There would be neither a context switch nor a scheduling-clock interrupt to tell TASKS_RCU that the task in question had passed through a quiescent state. The grace period would therefore extend indefinitely. This commit therefore makes RCU's dyntick-idle subsystem record the task_struct structure of the task that is running in dyntick-idle mode on each CPU. The TASKS_RCU grace period can then access this information and record a quiescent state on behalf of any CPU running in dyntick-idle usermode.

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/init_task.h |  3 ++-
 include/linux/sched.h     |  2 ++
 kernel/rcu/tree.c         |  2 ++
 kernel/rcu/tree.h         |  2 ++
 kernel/rcu/tree_plugin.h  | 16 ++++++++++++++++
 kernel/rcu/update.c       |  4 +++-
 6 files changed, 27 insertions(+), 2 deletions(-)

include/linux/init_task.h +2 -1

index 78715ea7c30c..642828009324 100644
@@ -128,7 +128,8 @@ extern struct group_info init_groups;
 #define INIT_TASK_RCU_TASKS(tsk)                                       \
        .rcu_tasks_holdout = false,                                     \
        .rcu_tasks_holdout_list =                                       \
-               LIST_HEAD_INIT(tsk.rcu_tasks_holdout_list),
+               LIST_HEAD_INIT(tsk.rcu_tasks_holdout_list),             \
+       .rcu_tasks_idle_cpu = -1,
 #else
 #define INIT_TASK_RCU_TASKS(tsk)
 #endif

include/linux/sched.h +2 -0

index 3cf124389ec7..5fa041f7a034 100644
@@ -1277,6 +1277,7 @@ struct task_struct {
        unsigned long rcu_tasks_nvcsw;
        int rcu_tasks_holdout;
        struct list_head rcu_tasks_holdout_list;
+       int rcu_tasks_idle_cpu;
 #endif /* #ifdef CONFIG_TASKS_RCU */
 
 #if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)

@@ -2021,6 +2022,7 @@ static inline void rcu_copy_process(struct task_struct *p)
 #ifdef CONFIG_TASKS_RCU
        p->rcu_tasks_holdout = false;
        INIT_LIST_HEAD(&p->rcu_tasks_holdout_list);
+       p->rcu_tasks_idle_cpu = -1;
 #endif /* #ifdef CONFIG_TASKS_RCU */
 }
 

kernel/rcu/tree.c +2 -0

index 645a33efc0d4..0d9ee1e4f446 100644
@@ -526,6 +526,7 @@ static void rcu_eqs_enter_common(struct rcu_dynticks *rdtp, long long oldval,
        atomic_inc(&rdtp->dynticks);
        smp_mb__after_atomic();  /* Force ordering with next sojourn. */
        WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
+       rcu_dynticks_task_enter();
 
        /*
         * It is illegal to enter an extended quiescent state while

@@ -642,6 +643,7 @@ void rcu_irq_exit(void)
 static void rcu_eqs_exit_common(struct rcu_dynticks *rdtp, long long oldval,
                               int user)
 {
+       rcu_dynticks_task_exit();
        smp_mb__before_atomic();  /* Force ordering w/previous sojourn. */
        atomic_inc(&rdtp->dynticks);
        /* CPUs seeing atomic_inc() must see later RCU read-side crit sects */

kernel/rcu/tree.h +2 -0

index 0f69a79c5b7d..37ff593b7725 100644
@@ -579,6 +579,8 @@ static void rcu_sysidle_report_gp(struct rcu_state *rsp, int isidle,
 static void rcu_bind_gp_kthread(void);
 static void rcu_sysidle_init_percpu_data(struct rcu_dynticks *rdtp);
 static bool rcu_nohz_full_cpu(struct rcu_state *rsp);
+static void rcu_dynticks_task_enter(void);
+static void rcu_dynticks_task_exit(void);
 
 #endif /* #ifndef RCU_TREE_NONCORE */
 

kernel/rcu/tree_plugin.h +0 -0

index a86a363ea453..0d8ef5cb1976 100644

kernel/rcu/tree_plugin.h static void rcu_bind_gp_kthread(void) · line 2861

Quoted 10 lines

kernel/rcu/tree_plugin.h +7 -0

@@ -2852,3 +2852,19 @@ static void rcu_bind_gp_kthread(void)
                set_cpus_allowed_ptr(current, cpumask_of(cpu));
 #endif /* #ifdef CONFIG_NO_HZ_FULL */
 }
+
+/* Record the current task on dyntick-idle entry. */
+static void rcu_dynticks_task_enter(void)
+{
+#if defined(CONFIG_TASKS_RCU) && defined(CONFIG_NO_HZ_FULL)
+       ACCESS_ONCE(current->rcu_tasks_idle_cpu) = smp_processor_id();
+#endif /* #if defined(CONFIG_TASKS_RCU) && defined(CONFIG_NO_HZ_FULL) */

Shouldn't we check that the cpu is actually a nohz_full cpu, like follows:

static void rcu_dynticks_task_enter(void)
{
#if defined(CONFIG_TASKS_RCU) && defined(CONFIG_NO_HZ_FULL)

Quoted 16 lines

kernel/rcu/tree_plugin.h +9 -0

+}
+
+/* Record no current task on dyntick-idle exit. */
+static void rcu_dynticks_task_exit(void)
+{
+#if defined(CONFIG_TASKS_RCU) && defined(CONFIG_NO_HZ_FULL)
+       ACCESS_ONCE(current->rcu_tasks_idle_cpu) = -1;
+#endif /* #if defined(CONFIG_TASKS_RCU) && defined(CONFIG_NO_HZ_FULL) */
+}

kernel/rcu/update.c +0 -0

index d997163c7e92..a4140f25cf1a 100644
@@ -466,7 +466,9 @@ static void check_holdout_task(struct task_struct *t,
 {
        if (!ACCESS_ONCE(t->rcu_tasks_holdout) ||

kernel/rcu/update.c static void check_holdout_task(struct task_struct *t, · line 471

Quoted 5 lines

kernel/rcu/update.c +3 -1

            t->rcu_tasks_nvcsw != ACCESS_ONCE(t->nvcsw) ||
-           !ACCESS_ONCE(t->on_rq)) {
+           !ACCESS_ONCE(t->on_rq) ||
+           (IS_ENABLED(CONFIG_NO_HZ_FULL) &&
+            !is_idle_task(t) && t->rcu_tasks_idle_cpu >= 0)) {

rcu_tasks_idle_cpu will be -1 in CONFIG_NO_HZ_FULL is not enabled. Why are you checking both here?

Quoted 6 lines

kernel/rcu/update.c +0 -1

                ACCESS_ONCE(t->rcu_tasks_holdout) = 0;
                list_del_rcu(&t->rcu_tasks_holdout_list);
                put_task_struct(t);
--

1.8.1.5

Signature · 1 line

Pranith

From: "Paul E. McKenney" <[email protected]>

In theory, synchronize_sched() requires a read-side critical section to order against. In practice, preemption can be thought of as being disabled across every machine instruction. So this commit removes the redundant preempt_disable() from rcu_note_voluntary_context_switch().

#define rcu_note_voluntary_context_switch(t) \

Quoted 6 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

In theory, synchronize_sched() requires a read-side critical section to order against. In practice, preemption can be thought of as being disabled across every machine instruction. So this commit removes the redundant preempt_disable() from rcu_note_voluntary_context_switch().

include/linux/rcupdate.h static inline void rcu_user_hooks_switch(struct task_struct *prev, · line 331

Quoted 7 lines

#define rcu_note_voluntary_context_switch(t) \

include/linux/rcupdate.h +0 -2

 	do { \
-		preempt_disable(); /* Exclude synchronize_sched(); */ \
 		if (ACCESS_ONCE((t)->rcu_tasks_holdout)) \
 			ACCESS_ONCE((t)->rcu_tasks_holdout) = 0; \
-		preempt_enable(); \
 	} while (0)

But that's more than 1 instruction.

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

In theory, synchronize_sched() requires a read-side critical section to order against. In practice, preemption can be thought of as being disabled across every machine instruction. So this commit removes the redundant preempt_disable() from rcu_note_voluntary_context_switch().

Quoted 9 lines

include/linux/rcupdate.h static inline void rcu_user_hooks_switch(struct task_struct *prev, · line 331

Quoted 7 lines

#define rcu_note_voluntary_context_switch(t) \

include/linux/rcupdate.h +0 -2

 	do { \
-		preempt_disable(); /* Exclude synchronize_sched(); */ \
 		if (ACCESS_ONCE((t)->rcu_tasks_holdout)) \
 			ACCESS_ONCE((t)->rcu_tasks_holdout) = 0; \
-		preempt_enable(); \
 	} while (0)

But that's more than 1 instruction.

Yeah, the commit log could use some help. The instruction in question is the store. The "if" is just an optimization.

So suppose that this sequence is preempted between the "if" and the store, and that the synchronize_sched() (and quite a bit more besides!) takes place during this preemption. The task is still in a quiescent state at the time of the store, so the store is still legitimate.

That said, it might be better to just leave preemption disabled, as that certainly makes things simpler. Thoughts?

Quoted 27 lines · Paul E. McKenney

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

In theory, synchronize_sched() requires a read-side critical section to order against. In practice, preemption can be thought of as being disabled across every machine instruction. So this commit removes the redundant preempt_disable() from rcu_note_voluntary_context_switch().

Quoted 9 lines

Review

Quoted 7 lines

#define rcu_note_voluntary_context_switch(t) \

(quoted diff) +0 -2

 	do { \
-		preempt_disable(); /* Exclude synchronize_sched(); */ \
 		if (ACCESS_ONCE((t)->rcu_tasks_holdout)) \
 			ACCESS_ONCE((t)->rcu_tasks_holdout) = 0; \
-		preempt_enable(); \
 	} while (0)

But that's more than 1 instruction.

Yeah, the commit log could use some help. The instruction in question is the store. The "if" is just an optimization.

So suppose that this sequence is preempted between the "if" and the store, and that the synchronize_sched() (and quite a bit more besides!) takes place during this preemption. The task is still in a quiescent state at the time of the store, so the store is still legitimate.

Quoted 2 lines

That said, it might be better to just leave preemption disabled, as that certainly makes things simpler. Thoughts?

A comment explaining it should be fine I think. I was just raising the obvious fail in the changelog.

From: "Paul E. McKenney" <[email protected]>

Because idle-task code may need to be patched, RCU-tasks need to wait for idle tasks to schedule. This commit therefore detects this case via context switch. Block CPU hotplug during this time to avoid sending IPIs to offline CPUs.

Note that checking for changes in the dyntick-idle counters is tempting, but wrong. The reason that it is wrong is that a interrupt or NMI can increment these counters without necessarily allowing the idle tasks to make any forward progress.

Quoted 11 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Because idle-task code may need to be patched, RCU-tasks need to wait for idle tasks to schedule. This commit therefore detects this case via context switch. Block CPU hotplug during this time to avoid sending IPIs to offline CPUs.

Note that checking for changes in the dyntick-idle counters is tempting, but wrong. The reason that it is wrong is that a interrupt or NMI can increment these counters without necessarily allowing the idle tasks to make any forward progress.

I'm going to NAK this.. with that rcu_idle patch I send there's typically only a single idle function thats out of bounds and if its more it can be made that with a bit of tlc to the cpuidle driver in question.

This needs _FAR_ more justification than a maybe and a want.

Quoted 18 lines · Peter Zijlstra

Quoted 11 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Because idle-task code may need to be patched, RCU-tasks need to wait for idle tasks to schedule. This commit therefore detects this case via context switch. Block CPU hotplug during this time to avoid sending IPIs to offline CPUs.

Note that checking for changes in the dyntick-idle counters is tempting, but wrong. The reason that it is wrong is that a interrupt or NMI can increment these counters without necessarily allowing the idle tasks to make any forward progress.

I'm going to NAK this.. with that rcu_idle patch I send there's typically only a single idle function thats out of bounds and if its more it can be made that with a bit of tlc to the cpuidle driver in question.

Quoted 1 line

This needs _FAR_ more justification than a maybe and a want.

Peter, your patch might be a good start, but I didn't see any reaction from Steven or Masami and it did only x86.

Quoted 19 lines · Paul E. McKenney

Quoted 18 lines · Peter Zijlstra

Quoted 11 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Because idle-task code may need to be patched, RCU-tasks need to wait for idle tasks to schedule. This commit therefore detects this case via context switch. Block CPU hotplug during this time to avoid sending IPIs to offline CPUs.

Note that checking for changes in the dyntick-idle counters is tempting, but wrong. The reason that it is wrong is that a interrupt or NMI can increment these counters without necessarily allowing the idle tasks to make any forward progress.

I'm going to NAK this.. with that rcu_idle patch I send there's typically only a single idle function thats out of bounds and if its more it can be made that with a bit of tlc to the cpuidle driver in question.

Quoted 4 lines

Quoted 1 line

This needs _FAR_ more justification than a maybe and a want.

Peter, your patch might be a good start, but I didn't see any reaction from Steven or Masami and it did only x86.

That's not an excuse for doing horrible things. And inventing new infra that needs to wake all CPUs is horrible.

I still need to look at the patches, but if this is just for the idle case, then we don't need it. The idle case can be solved with a simple sched_on_each_cpu(). I need a way to solve waiting for processes to finish from a preemption point.

That's all I want, and if we can remove the "idle" case and document it well that it's not covered and a sched_on_each_cpu() may be needed, then I'm fine with that.

	sched_on_each_cpu(dummy_op);
	call_rcu_tasks(free_tramp);

Quoted 11 lines · Steven Rostedt

I still need to look at the patches, but if this is just for the idle case, then we don't need it. The idle case can be solved with a simple sched_on_each_cpu(). I need a way to solve waiting for processes to finish from a preemption point.

That's all I want, and if we can remove the "idle" case and document it well that it's not covered and a sched_on_each_cpu() may be needed, then I'm fine with that.

	sched_on_each_cpu(dummy_op);
	call_rcu_tasks(free_tramp);

Sure, but why not dtrt and push rcu_idle hooks all the way down into the idle drivers if and where appropriate?

There isn't _that_ much idle driver code. Also, some stuff should be cleaned up; we're already calling stop_critical_timings() in the generic idle code, and then calling it again in the cpuidle drivers.

Quoted 17 lines · On Wed, 13 Aug 2014 16:07:05 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 11 lines · Steven Rostedt

I still need to look at the patches, but if this is just for the idle case, then we don't need it. The idle case can be solved with a simple sched_on_each_cpu(). I need a way to solve waiting for processes to finish from a preemption point.

That's all I want, and if we can remove the "idle" case and document it well that it's not covered and a sched_on_each_cpu() may be needed, then I'm fine with that.

	sched_on_each_cpu(dummy_op);
	call_rcu_tasks(free_tramp);

Sure, but why not dtrt and push rcu_idle hooks all the way down into the idle drivers if and where appropriate?

Quoted 3 lines

There isn't _that_ much idle driver code. Also, some stuff should be cleaned up; we're already calling stop_critical_timings() in the generic idle code, and then calling it again in the cpuidle drivers.

True, perhaps the rcu code should hook into the stop_critical_timings code?

o drivers/acpi/acpi_pad.c, power_saving_thread().

Looks like a kthread that does idle injection. Currently, RCU sees it as not a quiescent state. Would it kill these guys to put in a comment or two about what this is for???

So adding rcu_idle_enter() and rcu_idle_exit() here might actually fix a bug, though it is not clear how long this thing actually runs. If only for a few milliseconds, no harm done.

o drivers/thermal/intel_powerclamp.c, clamp_thread().

Looks similar to power_saving_thread(), but for thermal control. Probably short term, shouldn't be a problem either way.

Quoted 11 lines · Paul E. McKenney

o drivers/acpi/acpi_pad.c, power_saving_thread().

Looks like a kthread that does idle injection. Currently,
RCU sees it as not a quiescent state. Would it kill these guys to
put in a comment or two about what this is for???

So adding rcu_idle_enter() and rcu_idle_exit() here might actually fix a bug, though it is not clear how long this
thing actually runs. If only for a few milliseconds, no harm done.

Quoted 5 lines

o drivers/thermal/intel_powerclamp.c, clamp_thread().

Looks similar to power_saving_thread(), but for thermal
control. Probably short term, shouldn't be a problem either way.

There's patches somewhere that make that go-away

https://lkml.org/lkml/2014/6/4/56

Jacob was going to look at that.

Quoted 18 lines · On Wed, 13 Aug 2014 18:30:15 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 16 lines · Paul E. McKenney

o drivers/acpi/acpi_pad.c, power_saving_thread().

Looks like a kthread that does idle injection. Currently,
RCU sees it as not a quiescent state. Would it kill these guys to
put in a comment or two about what this is for???

So adding rcu_idle_enter() and rcu_idle_exit() here might actually fix a bug, though it is not clear how long this
thing actually runs. If only for a few milliseconds, no harm done.

o drivers/thermal/intel_powerclamp.c, clamp_thread().

Looks similar to power_saving_thread(), but for thermal
control. Probably short term, shouldn't be a problem either way.

Quoted 5 lines

There's patches somewhere that make that go-away

https://lkml.org/lkml/2014/6/4/56

Jacob was going to look at that.

yes, it is on my plate, plan to submit for 3.18. the idle period is only for a few miliseconds, default to 6ms.

So the first three look OK to hook rcu_idle_enter() and rcu_idle_exit() into, but the last two don't look so good.

That said, if you are OK not tracing the stuff under stop_critical_timings(), then I can use the RCU dyntick-idle state and not wake anything up.

Quoted 3 lines · Paul E. McKenney

So the first three look OK to hook rcu_idle_enter() and rcu_idle_exit() into, but the last two don't look so good.

Quoted 2 lines

That said, if you are OK not tracing the stuff under stop_critical_timings(), then I can use the RCU dyntick-idle state and not wake anything up.

Either way, Steve could easily whip up a debug thing that could validate that. Simply WARN whenever an __mcount happens when under rcu_idle.

And if we make these idle functions small enough that should not be a problem at all.

Quoted 17 lines · On Wed, 13 Aug 2014 16:07:05 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 11 lines · Steven Rostedt

I still need to look at the patches, but if this is just for the idle case, then we don't need it. The idle case can be solved with a simple sched_on_each_cpu(). I need a way to solve waiting for processes to finish from a preemption point.

That's all I want, and if we can remove the "idle" case and document it well that it's not covered and a sched_on_each_cpu() may be needed, then I'm fine with that.

	sched_on_each_cpu(dummy_op);
	call_rcu_tasks(free_tramp);

Sure, but why not dtrt and push rcu_idle hooks all the way down into the idle drivers if and where appropriate?

Quoted 5 lines

There isn't _that_ much idle driver code. Also, some stuff should be cleaned up; we're already calling stop_critical_timings() in the generic idle code, and then calling it again in the cpuidle drivers.

True, perhaps the rcu code should hook into the stop_critical_timings code?

Quoted 20 lines · On Wed, 13 Aug 2014 15:40:25 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 19 lines · Paul E. McKenney

Quoted 18 lines · Peter Zijlstra

Quoted 11 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Because idle-task code may need to be patched, RCU-tasks need to wait for idle tasks to schedule. This commit therefore detects this case via context switch. Block CPU hotplug during this time to avoid sending IPIs to offline CPUs.

Note that checking for changes in the dyntick-idle counters is tempting, but wrong. The reason that it is wrong is that a interrupt or NMI can increment these counters without necessarily allowing the idle tasks to make any forward progress.

I'm going to NAK this.. with that rcu_idle patch I send there's typically only a single idle function thats out of bounds and if its more it can be made that with a bit of tlc to the cpuidle driver in question.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

This needs _FAR_ more justification than a maybe and a want.

Peter, your patch might be a good start, but I didn't see any reaction from Steven or Masami and it did only x86.

That's not an excuse for doing horrible things. And inventing new infra that needs to wake all CPUs is horrible.

I still need to look at the patches, but if this is just for the idle case, then we don't need it. The idle case can be solved with a simple sched_on_each_cpu(). I need a way to solve waiting for processes to finish from a preemption point.

That's all I want, and if we can remove the "idle" case and document it well that it's not covered and a sched_on_each_cpu() may be needed, then I'm fine with that.

	sched_on_each_cpu(dummy_op);
	call_rcu_tasks(free_tramp);

Would that work?

Quoted 19 lines · Paul E. McKenney

Quoted 18 lines · Peter Zijlstra

Quoted 11 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

Because idle-task code may need to be patched, RCU-tasks need to wait for idle tasks to schedule. This commit therefore detects this case via context switch. Block CPU hotplug during this time to avoid sending IPIs to offline CPUs.

Note that checking for changes in the dyntick-idle counters is tempting, but wrong. The reason that it is wrong is that a interrupt or NMI can increment these counters without necessarily allowing the idle tasks to make any forward progress.

I'm going to NAK this.. with that rcu_idle patch I send there's typically only a single idle function thats out of bounds and if its more it can be made that with a bit of tlc to the cpuidle driver in question.

Quoted 4 lines

Quoted 1 line

This needs _FAR_ more justification than a maybe and a want.

Peter, your patch might be a good start, but I didn't see any reaction from Steven or Masami and it did only x86.

That's not an excuse for doing horrible things. And inventing new infra that needs to wake all CPUs is horrible.

Quoted 2 lines

That's not an excuse for doing horrible things. And inventing new infra that needs to wake all CPUs is horrible.

Does your patch even work?

Looks like it should, and yes, the idle loop
seems quite a bit simpler than it was a few years ago, but we really don't need some strange thing that leaves a CPU idle but not visible as such to RCU.

I have already said that I will be happy to rip out the wakeup code when it is no longer needed, and I agree that it would be way better if not needed.

But I won't base a patch on hypotheticals. You have already drawn way too much water from -that- well over the past years! ;-)

Auditing all idle functions will be somewhat of a pain, but its entirely doable. Looking at this stuff, it appears we can clean it up massively; see how the generic cpuidle code already has the broadcast logic in, so we can remove that from the drivers by setting the right flags.

We can similarly pull out the leave_mm() call by adding a CPUIDLE_FLAG_TLB_FLUSH. At which point all we'd need to do is mark the intel_idle (and all other cpuidle_state::enter functions with __notrace.

Quoted 5 lines · Peter Zijlstra

Auditing all idle functions will be somewhat of a pain, but its entirely doable. Looking at this stuff, it appears we can clean it up massively; see how the generic cpuidle code already has the broadcast logic in, so we can remove that from the drivers by setting the right flags.

Quoted 3 lines

We can similarly pull out the leave_mm() call by adding a CPUIDLE_FLAG_TLB_FLUSH. At which point all we'd need to do is mark the intel_idle (and all other cpuidle_state::enter functions with __notrace.

This removes the broadcast stuff from intel_idle.c; processor_idle.c hurts my brain, but something similar should be possible.

Quoted 5 lines · Peter Zijlstra

Auditing all idle functions will be somewhat of a pain, but its entirely doable. Looking at this stuff, it appears we can clean it up massively; see how the generic cpuidle code already has the broadcast logic in, so we can remove that from the drivers by setting the right flags.

Quoted 3 lines

We can similarly pull out the leave_mm() call by adding a CPUIDLE_FLAG_TLB_FLUSH. At which point all we'd need to do is mark the intel_idle (and all other cpuidle_state::enter functions with __notrace.

This removes the broadcast stuff from intel_idle.c; processor_idle.c hurts my brain, but something similar should be possible.

Quoted 4 lines · Paul E. McKenney

Quoted 2 lines

That's not an excuse for doing horrible things. And inventing new infra that needs to wake all CPUs is horrible.

Does your patch even work?

Haven't even tried compiling it, but making it work shouldn't be too hard.

Quoted 4 lines

Looks like it should, and yes, the idle loop
seems quite a bit simpler than it was a few years ago, but we really don't need some strange thing that leaves a CPU idle but not visible as such to RCU.

There's slightly more to it though; things like the x86 mwait idle wait functions tend to do far too much; for instance look at:

drivers/idle/intel_idle.c:intel_idle()

We should push the rcu_idle_{enter,exit}() down to around mwait_idle_with_hints(), so we don't call half the word with RCU disabled.

Quoted 3 lines

I have already said that I will be happy to rip out the wakeup code when it is no longer needed, and I agree that it would be way better if not needed.

I'd prefer to dtrt now and not needing to fix it later.

Auditing all idle functions will be somewhat of a pain, but its entirely doable. Looking at this stuff, it appears we can clean it up massively; see how the generic cpuidle code already has the broadcast logic in, so we can remove that from the drivers by setting the right flags.

We can similarly pull out the leave_mm() call by adding a CPUIDLE_FLAG_TLB_FLUSH. At which point all we'd need to do is mark the intel_idle (and all other cpuidle_state::enter functions with __notrace.

Quoted 2 lines

But I won't base a patch on hypotheticals. You have already drawn way too much water from -that- well over the past years! ;-)

not entirely sure what you're referring to there ;-)

cpuidle_idle_call() does a fastpath irq-enable/exit if need-resched, then does stop_critical_timings() and rcu_idle_enter(). Then we have the buried complexity with cpuidle_select(), but a negative return says to check need-resched and enable interrupts or to invoke arch_cpu_idle(), which executes various sleep instructions on various architectures. Some notable variants:

And various other architectures seem to work similarly, but lots of hair here. So Steven, you OK with the underlying arch_cpu_idle() functions being off-limits to tracing?

Now, if cpuidle_select() returns non-negative, we are dealing with the CPU-idle governor, which is invoked at the later cpuidle_enter().

Hmmm... On the CPU-idle drivers...

o apm_idle_driver puts the idle loop into the ->enter() function,

	apm_cpu_idle().

o ACPI puts the idle loop in acpi_idle_do_entry(), and does call
stop_critical_timings(), but not rcu_idle_enter(). So presumably stop_critical_timings() can nest? Not clear from the code.

o The CPS driver is even stranger... Is cps_gen_entry_code()
really depositing assembly instructions into a buffer that is passed back as a function?

o The intel_idle driver is the one with mwait_idle_with_hints(),
so you covered it below.

Your patch covers the cpuidle_enter() transition, which means that functions like cpuidle_enter(), acpi_idle_enter_c1(), and acpi_idle_do_entry() would be off-limits to trampolining. In the case of CPS, quite a bit of code.

Quoted 3 lines

We should push the rcu_idle_{enter,exit}() down to around mwait_idle_with_hints(), so we don't call half the word with RCU disabled.

That would be for the intel_idle.c CPU-idle driver. The other drivers also need rcu_idle_{enter,exit}().

Quoted 5 lines

Quoted 3 lines

I have already said that I will be happy to rip out the wakeup code when it is no longer needed, and I agree that it would be way better if not needed.

I'd prefer to dtrt now and not needing to fix it later.

Once it works, I might consider it "right" and adjust accordingly. At the moment, speculation.

Quoted 6 lines · Paul E. McKenney

cpuidle_idle_call() does a fastpath irq-enable/exit if need-resched, then does stop_critical_timings() and rcu_idle_enter(). Then we have the buried complexity with cpuidle_select(), but a negative return says to check need-resched and enable interrupts or to invoke arch_cpu_idle(), which executes various sleep instructions on various architectures. Some notable variants:

Quoted 3 lines

And various other architectures seem to work similarly, but lots of hair here. So Steven, you OK with the underlying arch_cpu_idle() functions being off-limits to tracing?

I didn't find anything particularly hairy in the arch_cpu_idle() implementations, lots of simple 'go sleep' or 'spin' like things.