| Message ID | 20221115131926.3409974-1-qiang1.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rcu: Avoid invalid wakeup for rcuc kthreads in RCU_KTHREAD_OFFCPU status | expand |
On Tue, Nov 15, 2022 at 09:19:26PM +0800, Zqiang wrote: > For CONFIG_PREEMPT_RT=y kernel, the "use_softirq=0" will be set, the > RCU_SOFTIRQ processing is moved to per-CPU rcuc kthreads which created > by smpboot_register_percpu_thread(). when CPU is going offline, the > corresponding rcu_data.rcu_cpu_kthread_status is set RCU_KTHREAD_OFFCPU, > and the rcuc kthreads enter TASK_PARKED state, kthreads in TASK_PARKED > state only accept kthread_unpark() to wakeup. > > Therefore, This commit avoid invoke wake_up_process() to rcuc kthreads > in TASK_PARKED state. > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > --- > kernel/rcu/tree.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 3ccad468887e..49dd87356851 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -2375,7 +2375,8 @@ static void rcu_wake_cond(struct task_struct *t, int status) > * If the thread is yielding, only wake it when this > * is invoked from idle > */ > - if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current))) > + if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current)) && > + status != RCU_KTHREAD_OFFCPU) > wake_up_process(t); There is a tiny window where this can happen (between CPUHP_TEARDOWN_CPU and CPUHP_AP_SMPBOOT_THREADS) and it can't cause a spurious unpark because wake_up_process() only wakes up from TASK_[UN]INTERRUPTIBLE states. And even if it did, the KTHREAD_SHOULD_PARK bit would still be on. And more important! On unpark time RCU_KTHREAD_OFFCPU isn't cleared. Only the rcuc kthread does it, and after your patch it couldn't be awaken to perform that, unless rcuc is lucky enough to have rcu_data.rcu_cpu_has_work = 1 by the time it unparks and that isn't guaranteed. So rcuc may sleep forever. OTOH one cleanup that could be done is to make rcu_cpu_kthread_park() to use __this_cpu_write as it's guaranteed that cpu == smp_processor_id(). Thanks.
On Tue, Nov 15, 2022 at 09:19:26PM +0800, Zqiang wrote: > For CONFIG_PREEMPT_RT=y kernel, the "use_softirq=0" will be set, the > RCU_SOFTIRQ processing is moved to per-CPU rcuc kthreads which created > by smpboot_register_percpu_thread(). when CPU is going offline, the > corresponding rcu_data.rcu_cpu_kthread_status is set RCU_KTHREAD_OFFCPU, > and the rcuc kthreads enter TASK_PARKED state, kthreads in TASK_PARKED > state only accept kthread_unpark() to wakeup. > > Therefore, This commit avoid invoke wake_up_process() to rcuc kthreads > in TASK_PARKED state. > > Signed-off-by: Zqiang <qiang1.zhang@intel.com> > --- > kernel/rcu/tree.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 3ccad468887e..49dd87356851 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -2375,7 +2375,8 @@ static void rcu_wake_cond(struct task_struct *t, int status) > * If the thread is yielding, only wake it when this > * is invoked from idle > */ > - if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current))) > + if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current)) && > + status != RCU_KTHREAD_OFFCPU) > wake_up_process(t); >There is a tiny window where this can happen (between CPUHP_TEARDOWN_CPU >and CPUHP_AP_SMPBOOT_THREADS) and it can't cause a spurious unpark because >wake_up_process() only wakes up from TASK_[UN]INTERRUPTIBLE states. And even >if it did, the KTHREAD_SHOULD_PARK bit would still be on. Yes even if it did, because KTHREAD_SHOULD_PARK bit would still be on, this kthreads will schedule out again. > >And more important! On unpark time RCU_KTHREAD_OFFCPU isn't cleared. Only the >rcuc kthread does it, and after your patch it couldn't be awaken to perform >that, unless rcuc is lucky enough to have rcu_data.rcu_cpu_has_work = 1 >by the time it unparks and that isn't guaranteed. So rcuc may sleep forever. Thanks for review, yes I should register an unpark function to clear RCU_KTHREAD_OFFCPU. Is the following modification more appropriate? diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 3ccad468887e..a2248af0ccda 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -2375,7 +2375,8 @@ static void rcu_wake_cond(struct task_struct *t, int status) * If the thread is yielding, only wake it when this * is invoked from idle */ - if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current))) + if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current)) && + status != RCU_KTHREAD_OFFCPU) wake_up_process(t); } @@ -2407,7 +2408,14 @@ static void invoke_rcu_core(void) static void rcu_cpu_kthread_park(unsigned int cpu) { - per_cpu(rcu_data.rcu_cpu_kthread_status, cpu) = RCU_KTHREAD_OFFCPU; + WARN_ON_ONCE(cpu != smp_processor_id()); + __this_cpu_write(rcu_data.rcu_cpu_kthread_status, RCU_KTHREAD_OFFCPU); +} + +static void rcu_cpu_kthread_unpark(unsigned int cpu) +{ + WARN_ON_ONCE(cpu != smp_processor_id()); + __this_cpu_write(rcu_data.rcu_cpu_kthread_status, RCU_KTHREAD_ONCPU); } static int rcu_cpu_kthread_should_run(unsigned int cpu) @@ -2460,6 +2468,7 @@ static struct smp_hotplug_thread rcu_cpu_thread_spec = { .thread_comm = "rcuc/%u", .setup = rcu_cpu_kthread_setup, .park = rcu_cpu_kthread_park, + .unpark = rcu_cpu_kthread_unpark, }; /* diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h index fcb5d696eb17..c4b9606968db 100644 --- a/kernel/rcu/tree.h +++ b/kernel/rcu/tree.h @@ -36,8 +36,9 @@ struct rcu_exp_work { #define RCU_KTHREAD_RUNNING 1 #define RCU_KTHREAD_WAITING 2 #define RCU_KTHREAD_OFFCPU 3 -#define RCU_KTHREAD_YIELDING 4 -#define RCU_KTHREAD_MAX 4 +#define RCU_KTHREAD_ONCPU 4 +#define RCU_KTHREAD_YIELDING 5 +#define RCU_KTHREAD_MAX 5 Thanks Zqiang > >OTOH one cleanup that could be done is to make rcu_cpu_kthread_park() to use >__this_cpu_write as it's guaranteed that cpu == smp_processor_id(). > >Thanks.
On Wed, Nov 16, 2022 at 02:07:28PM +0000, Zhang, Qiang1 wrote: > On Tue, Nov 15, 2022 at 09:19:26PM +0800, Zqiang wrote: > >And more important! On unpark time RCU_KTHREAD_OFFCPU isn't cleared. Only the > >rcuc kthread does it, and after your patch it couldn't be awaken to perform > >that, unless rcuc is lucky enough to have rcu_data.rcu_cpu_has_work = 1 > >by the time it unparks and that isn't guaranteed. So rcuc may sleep forever. > > Thanks for review, yes I should register an unpark function to clear RCU_KTHREAD_OFFCPU. > Is the following modification more appropriate? > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 3ccad468887e..a2248af0ccda 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -2375,7 +2375,8 @@ static void rcu_wake_cond(struct task_struct *t, int status) > * If the thread is yielding, only wake it when this > * is invoked from idle > */ > - if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current))) > + if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current)) && > + status != RCU_KTHREAD_OFFCPU) > wake_up_process(t); > } > > @@ -2407,7 +2408,14 @@ static void invoke_rcu_core(void) > > static void rcu_cpu_kthread_park(unsigned int cpu) > { > - per_cpu(rcu_data.rcu_cpu_kthread_status, cpu) = RCU_KTHREAD_OFFCPU; > + WARN_ON_ONCE(cpu != smp_processor_id()); > + __this_cpu_write(rcu_data.rcu_cpu_kthread_status, RCU_KTHREAD_OFFCPU); > +} > + > +static void rcu_cpu_kthread_unpark(unsigned int cpu) > +{ > + WARN_ON_ONCE(cpu != smp_processor_id()); > + __this_cpu_write(rcu_data.rcu_cpu_kthread_status, RCU_KTHREAD_ONCPU); > } > > static int rcu_cpu_kthread_should_run(unsigned int cpu) > @@ -2460,6 +2468,7 @@ static struct smp_hotplug_thread rcu_cpu_thread_spec = { > .thread_comm = "rcuc/%u", > .setup = rcu_cpu_kthread_setup, > .park = rcu_cpu_kthread_park, > + .unpark = rcu_cpu_kthread_unpark, Well, personally I don't think it's worth the burden because wake_up_process() already does an early exit if it's not dealing with a TASK_[UN]INTERRUPTIBLE task and the window is so short and rare that it doesn't look like a good candidate for extra optimization; Thanks.
On Wed, Nov 16, 2022 at 02:07:28PM +0000, Zhang, Qiang1 wrote: > On Tue, Nov 15, 2022 at 09:19:26PM +0800, Zqiang wrote: > >And more important! On unpark time RCU_KTHREAD_OFFCPU isn't cleared. Only the > >rcuc kthread does it, and after your patch it couldn't be awaken to perform > >that, unless rcuc is lucky enough to have rcu_data.rcu_cpu_has_work = 1 > >by the time it unparks and that isn't guaranteed. So rcuc may sleep forever. > > Thanks for review, yes I should register an unpark function to clear RCU_KTHREAD_OFFCPU. > Is the following modification more appropriate? > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 3ccad468887e..a2248af0ccda 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -2375,7 +2375,8 @@ static void rcu_wake_cond(struct task_struct *t, int status) > * If the thread is yielding, only wake it when this > * is invoked from idle > */ > - if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current))) > + if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current)) && > + status != RCU_KTHREAD_OFFCPU) > wake_up_process(t); > } > > @@ -2407,7 +2408,14 @@ static void invoke_rcu_core(void) > > static void rcu_cpu_kthread_park(unsigned int cpu) > { > - per_cpu(rcu_data.rcu_cpu_kthread_status, cpu) = RCU_KTHREAD_OFFCPU; > + WARN_ON_ONCE(cpu != smp_processor_id()); > + __this_cpu_write(rcu_data.rcu_cpu_kthread_status, RCU_KTHREAD_OFFCPU); > +} > + > +static void rcu_cpu_kthread_unpark(unsigned int cpu) > +{ > + WARN_ON_ONCE(cpu != smp_processor_id()); > + __this_cpu_write(rcu_data.rcu_cpu_kthread_status, RCU_KTHREAD_ONCPU); > } > > static int rcu_cpu_kthread_should_run(unsigned int cpu) > @@ -2460,6 +2468,7 @@ static struct smp_hotplug_thread rcu_cpu_thread_spec = { > .thread_comm = "rcuc/%u", > .setup = rcu_cpu_kthread_setup, > .park = rcu_cpu_kthread_park, > + .unpark = rcu_cpu_kthread_unpark, >Well, personally I don't think it's worth the burden because wake_up_process() >already does an early exit if it's not dealing with a TASK_[UN]INTERRUPTIBLE task and >the window is so short and rare that it doesn't look like a good candidate for >extra optimization; Agree, in try_to_wake_up(), acquire rcuc task->pi_lock(there's almost no lock contention here) and then check tasks state, for TASK_[UN]INTERRUPTIBLE task, not dealing and early exit. Thanks Zqiang > >Thanks.
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 3ccad468887e..49dd87356851 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -2375,7 +2375,8 @@ static void rcu_wake_cond(struct task_struct *t, int status) * If the thread is yielding, only wake it when this * is invoked from idle */ - if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current))) + if (t && (status != RCU_KTHREAD_YIELDING || is_idle_task(current)) && + status != RCU_KTHREAD_OFFCPU) wake_up_process(t); }
For CONFIG_PREEMPT_RT=y kernel, the "use_softirq=0" will be set, the RCU_SOFTIRQ processing is moved to per-CPU rcuc kthreads which created by smpboot_register_percpu_thread(). when CPU is going offline, the corresponding rcu_data.rcu_cpu_kthread_status is set RCU_KTHREAD_OFFCPU, and the rcuc kthreads enter TASK_PARKED state, kthreads in TASK_PARKED state only accept kthread_unpark() to wakeup. Therefore, This commit avoid invoke wake_up_process() to rcuc kthreads in TASK_PARKED state. Signed-off-by: Zqiang <qiang1.zhang@intel.com> --- kernel/rcu/tree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)