| Message ID | 20241218165008.37820-1-frederic@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | hrtimer: Fix timers queued locally from offline CPUs | expand |
On Wed, Dec 18, 2024 at 05:50:05PM +0100, Frederic Weisbecker wrote: > 5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier") > was introduced to fix stalls with scheduler bandwidth timers getting > migrated while some kthreads handling CPU hotplug rely on bandwidth. > > However this has introduced several other issues which used to be > confined to RCU. But not anymore as it is spreading to hotplug code > itself (https://lore.kernel.org/all/20241213203739.1519801-1-usamaarif642@gmail.com/) > > Instead of introducing yet another new hackery, fix the problem in > hrtimers for everyone. The good news is that this passes 12 hours of 400*TREE03. (Yay!!!) The so-so news is that this gives only about 70% confidence that these patches help, but on the other hand, it also gives much higher confidence that these patches are not hurting anything. At least for TREE03. The not-so-good news is that this series causes build failures for rcutorture scenarios (such as SRCU-T) that build with CONFIG_SMP=n: ------------------------------------------------------------------------ kernel/time/hrtimer.c: In function ‘enqueue_hrtimer_offline’: kernel/time/hrtimer.c:1229:42: error: ‘migration_base’ undeclared (first use in this function); did you mean ‘is_migration_base’? ------------------------------------------------------------------------ When built with KCSAN enabled (--kcsan to kvm.sh), there is this additional build failure on that same line of code: ------------------------------------------------------------------------ kernel/time/hrtimer.c:1229:3: error: incompatible pointer types assigning to 'volatile typeof (timer->base)' (aka 'struct hrtimer_clock_base *volatile') from 'bool (*)(struct hrtimer_clock_base *)' (aka '_Bool (*)(struct hrtimer_clock_base *)') [-Werror,-Wincompatible-pointer-types] 1229 | WRITE_ONCE(timer->base, &migration_base); ------------------------------------------------------------------------ Me, I am a bit surprised that enqueue_hrtimer_offline() is even built in a CONFIG_SMP=n kernel. But there might be some reason why #ifdef-ing out that function's body would be a bad idea, so over to you! ;-) Thanx, Paul > Frederic Weisbecker (3): > hrtimers: Force migrate away hrtimers queued after > CPUHP_AP_HRTIMERS_DYING > rcu: Remove swake_up_one_online() bandaid > Revert "rcu/nocb: Fix rcuog wake-up from offline softirq" > > include/linux/hrtimer_defs.h | 1 + > kernel/rcu/tree.c | 34 +------------------- > kernel/rcu/tree_exp.h | 2 +- > kernel/rcu/tree_nocb.h | 10 ++---- > kernel/time/hrtimer.c | 60 +++++++++++++++++++++++++++++++----- > 5 files changed, 58 insertions(+), 49 deletions(-) > > -- > 2.46.0 >
On Thu, Dec 19, 2024 at 09:42:48AM -0800, Paul E. McKenney wrote: > On Wed, Dec 18, 2024 at 05:50:05PM +0100, Frederic Weisbecker wrote: > > 5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier") > > was introduced to fix stalls with scheduler bandwidth timers getting > > migrated while some kthreads handling CPU hotplug rely on bandwidth. > > > > However this has introduced several other issues which used to be > > confined to RCU. But not anymore as it is spreading to hotplug code > > itself (https://lore.kernel.org/all/20241213203739.1519801-1-usamaarif642@gmail.com/) > > > > Instead of introducing yet another new hackery, fix the problem in > > hrtimers for everyone. > > The good news is that this passes 12 hours of 400*TREE03. (Yay!!!) > > The so-so news is that this gives only about 70% confidence that these > patches help, but on the other hand, it also gives much higher confidence > that these patches are not hurting anything. > > At least for TREE03. Another day, another 12 hours of 400*TREE03 passed. This gets us up beyond 90% confidence that these patches help, and even more confidence that they are not too severely hurting anything. > The not-so-good news is that this series causes build failures for > rcutorture scenarios (such as SRCU-T) that build with CONFIG_SMP=n: > > ------------------------------------------------------------------------ > kernel/time/hrtimer.c: In function ‘enqueue_hrtimer_offline’: > kernel/time/hrtimer.c:1229:42: error: ‘migration_base’ undeclared (first use in this function); did you mean ‘is_migration_base’? > ------------------------------------------------------------------------ > > When built with KCSAN enabled (--kcsan to kvm.sh), there is this > additional build failure on that same line of code: > > ------------------------------------------------------------------------ > kernel/time/hrtimer.c:1229:3: error: incompatible pointer types assigning to 'volatile typeof (timer->base)' (aka 'struct hrtimer_clock_base *volatile') from 'bool (*)(struct hrtimer_clock_base *)' (aka '_Bool (*)(struct hrtimer_clock_base *)') [-Werror,-Wincompatible-pointer-types] > > 1229 | WRITE_ONCE(timer->base, &migration_base); > ------------------------------------------------------------------------ > > Me, I am a bit surprised that enqueue_hrtimer_offline() is even built > in a CONFIG_SMP=n kernel. > > But there might be some reason why #ifdef-ing out that function's body > would be a bad idea, so over to you! ;-) Curiosity overcame me, and the #ifdef makes things at least appear to work for CONFIG_SMP=n kernels. Experimental patch below, which I intend to use for further testing, but I have no plans to push it upstream. Thanx, Paul ------------------------------------------------------------------------ commit 61a0b92b3a3cfef69e3848806e51d1b99a9e9406 Author: Paul E. McKenney <paulmck@kernel.org> Date: Fri Dec 20 15:10:26 2024 -0800 EXP hrtimers: No-op enqueue_hrtimer_offline() if !HOTPLUG_CPU In kernels built with CONFIG_HOTPLUG_CPU=n, this experimental commit uses #ifdef to remove the body of the enqueue_hrtimer_offline() function. This might or might not be the correct fix to the build complaint about migration_base being undeclared. Signed-off-by: Paul E. McKenney <paulmck@kernel.org> Cc: Vlad Poenaru <vlad.wing@gmail.com> Cc: Usama Arif <usamaarif642@gmail.com> Cc: Frederic Weisbecker <frederic@kernel.org> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c index 48c0078d2c4f2..4235b7825b152 100644 --- a/kernel/time/hrtimer.c +++ b/kernel/time/hrtimer.c @@ -1213,6 +1213,7 @@ static void enqueue_hrtimer_offline(struct hrtimer *timer, struct hrtimer_clock_base *base, const enum hrtimer_mode mode) { +#ifdef CONFIG_HOTPLUG_CPU struct hrtimer_cpu_base *new_cpu_base, *old_cpu_base, *this_cpu_base; struct hrtimer_clock_base *new_base; int cpu; @@ -1238,6 +1239,7 @@ static void enqueue_hrtimer_offline(struct hrtimer *timer, if (enqueue_hrtimer(timer, new_base, mode)) smp_call_function_single_async(cpu, &new_cpu_base->csd); +#endif }
Le Fri, Dec 20, 2024 at 03:19:31PM -0800, Paul E. McKenney a écrit : > On Thu, Dec 19, 2024 at 09:42:48AM -0800, Paul E. McKenney wrote: > > On Wed, Dec 18, 2024 at 05:50:05PM +0100, Frederic Weisbecker wrote: > > > 5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier") > > > was introduced to fix stalls with scheduler bandwidth timers getting > > > migrated while some kthreads handling CPU hotplug rely on bandwidth. > > > > > > However this has introduced several other issues which used to be > > > confined to RCU. But not anymore as it is spreading to hotplug code > > > itself (https://lore.kernel.org/all/20241213203739.1519801-1-usamaarif642@gmail.com/) > > > > > > Instead of introducing yet another new hackery, fix the problem in > > > hrtimers for everyone. > > > > The good news is that this passes 12 hours of 400*TREE03. (Yay!!!) > > > > The so-so news is that this gives only about 70% confidence that these > > patches help, but on the other hand, it also gives much higher confidence > > that these patches are not hurting anything. > > > > At least for TREE03. > > Another day, another 12 hours of 400*TREE03 passed. This gets us up > beyond 90% confidence that these patches help, and even more confidence > that they are not too severely hurting anything. Cool :-) > > > The not-so-good news is that this series causes build failures for > > rcutorture scenarios (such as SRCU-T) that build with CONFIG_SMP=n: > > > > ------------------------------------------------------------------------ > > kernel/time/hrtimer.c: In function ‘enqueue_hrtimer_offline’: > > kernel/time/hrtimer.c:1229:42: error: ‘migration_base’ undeclared (first use in this function); did you mean ‘is_migration_base’? > > ------------------------------------------------------------------------ > > > > When built with KCSAN enabled (--kcsan to kvm.sh), there is this > > additional build failure on that same line of code: > > > > ------------------------------------------------------------------------ > > kernel/time/hrtimer.c:1229:3: error: incompatible pointer types assigning to 'volatile typeof (timer->base)' (aka 'struct hrtimer_clock_base *volatile') from 'bool (*)(struct hrtimer_clock_base *)' (aka '_Bool (*)(struct hrtimer_clock_base *)') [-Werror,-Wincompatible-pointer-types] > > > > 1229 | WRITE_ONCE(timer->base, &migration_base); > > ------------------------------------------------------------------------ > > > > Me, I am a bit surprised that enqueue_hrtimer_offline() is even built > > in a CONFIG_SMP=n kernel. > > > > But there might be some reason why #ifdef-ing out that function's body > > would be a bad idea, so over to you! ;-) > > Curiosity overcame me, and the #ifdef makes things at least appear to > work for CONFIG_SMP=n kernels. Experimental patch below, which I intend > to use for further testing, but I have no plans to push it upstream. > > Thanx, Paul > > ------------------------------------------------------------------------ > > commit 61a0b92b3a3cfef69e3848806e51d1b99a9e9406 > Author: Paul E. McKenney <paulmck@kernel.org> > Date: Fri Dec 20 15:10:26 2024 -0800 > > EXP hrtimers: No-op enqueue_hrtimer_offline() if !HOTPLUG_CPU > > In kernels built with CONFIG_HOTPLUG_CPU=n, this experimental commit > uses #ifdef to remove the body of the enqueue_hrtimer_offline() function. > This might or might not be the correct fix to the build complaint about > migration_base being undeclared. > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > Cc: Vlad Poenaru <vlad.wing@gmail.com> > Cc: Usama Arif <usamaarif642@gmail.com> > Cc: Frederic Weisbecker <frederic@kernel.org> > > diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c > index 48c0078d2c4f2..4235b7825b152 100644 > --- a/kernel/time/hrtimer.c > +++ b/kernel/time/hrtimer.c > @@ -1213,6 +1213,7 @@ static void enqueue_hrtimer_offline(struct hrtimer *timer, > struct hrtimer_clock_base *base, > const enum hrtimer_mode mode) > { > +#ifdef CONFIG_HOTPLUG_CPU > struct hrtimer_cpu_base *new_cpu_base, *old_cpu_base, *this_cpu_base; > struct hrtimer_clock_base *new_base; > int cpu; > @@ -1238,6 +1239,7 @@ static void enqueue_hrtimer_offline(struct hrtimer *timer, > > if (enqueue_hrtimer(timer, new_base, mode)) > smp_call_function_single_async(cpu, &new_cpu_base->csd); > +#endif > } I was thinking about something like that. Can I fold that and add your SOB? Thanks. > >
On Sat, Dec 21, 2024 at 12:26:01AM +0100, Frederic Weisbecker wrote: > Le Fri, Dec 20, 2024 at 03:19:31PM -0800, Paul E. McKenney a écrit : > > On Thu, Dec 19, 2024 at 09:42:48AM -0800, Paul E. McKenney wrote: > > > On Wed, Dec 18, 2024 at 05:50:05PM +0100, Frederic Weisbecker wrote: > > > > 5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier") > > > > was introduced to fix stalls with scheduler bandwidth timers getting > > > > migrated while some kthreads handling CPU hotplug rely on bandwidth. > > > > > > > > However this has introduced several other issues which used to be > > > > confined to RCU. But not anymore as it is spreading to hotplug code > > > > itself (https://lore.kernel.org/all/20241213203739.1519801-1-usamaarif642@gmail.com/) > > > > > > > > Instead of introducing yet another new hackery, fix the problem in > > > > hrtimers for everyone. > > > > > > The good news is that this passes 12 hours of 400*TREE03. (Yay!!!) > > > > > > The so-so news is that this gives only about 70% confidence that these > > > patches help, but on the other hand, it also gives much higher confidence > > > that these patches are not hurting anything. > > > > > > At least for TREE03. > > > > Another day, another 12 hours of 400*TREE03 passed. This gets us up > > beyond 90% confidence that these patches help, and even more confidence > > that they are not too severely hurting anything. > > Cool :-) > > > > > > The not-so-good news is that this series causes build failures for > > > rcutorture scenarios (such as SRCU-T) that build with CONFIG_SMP=n: > > > > > > ------------------------------------------------------------------------ > > > kernel/time/hrtimer.c: In function ‘enqueue_hrtimer_offline’: > > > kernel/time/hrtimer.c:1229:42: error: ‘migration_base’ undeclared (first use in this function); did you mean ‘is_migration_base’? > > > ------------------------------------------------------------------------ > > > > > > When built with KCSAN enabled (--kcsan to kvm.sh), there is this > > > additional build failure on that same line of code: > > > > > > ------------------------------------------------------------------------ > > > kernel/time/hrtimer.c:1229:3: error: incompatible pointer types assigning to 'volatile typeof (timer->base)' (aka 'struct hrtimer_clock_base *volatile') from 'bool (*)(struct hrtimer_clock_base *)' (aka '_Bool (*)(struct hrtimer_clock_base *)') [-Werror,-Wincompatible-pointer-types] > > > > > > 1229 | WRITE_ONCE(timer->base, &migration_base); > > > ------------------------------------------------------------------------ > > > > > > Me, I am a bit surprised that enqueue_hrtimer_offline() is even built > > > in a CONFIG_SMP=n kernel. > > > > > > But there might be some reason why #ifdef-ing out that function's body > > > would be a bad idea, so over to you! ;-) > > > > Curiosity overcame me, and the #ifdef makes things at least appear to > > work for CONFIG_SMP=n kernels. Experimental patch below, which I intend > > to use for further testing, but I have no plans to push it upstream. > > > > Thanx, Paul > > > > ------------------------------------------------------------------------ > > > > commit 61a0b92b3a3cfef69e3848806e51d1b99a9e9406 > > Author: Paul E. McKenney <paulmck@kernel.org> > > Date: Fri Dec 20 15:10:26 2024 -0800 > > > > EXP hrtimers: No-op enqueue_hrtimer_offline() if !HOTPLUG_CPU > > > > In kernels built with CONFIG_HOTPLUG_CPU=n, this experimental commit > > uses #ifdef to remove the body of the enqueue_hrtimer_offline() function. > > This might or might not be the correct fix to the build complaint about > > migration_base being undeclared. > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > Cc: Vlad Poenaru <vlad.wing@gmail.com> > > Cc: Usama Arif <usamaarif642@gmail.com> > > Cc: Frederic Weisbecker <frederic@kernel.org> > > > > diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c > > index 48c0078d2c4f2..4235b7825b152 100644 > > --- a/kernel/time/hrtimer.c > > +++ b/kernel/time/hrtimer.c > > @@ -1213,6 +1213,7 @@ static void enqueue_hrtimer_offline(struct hrtimer *timer, > > struct hrtimer_clock_base *base, > > const enum hrtimer_mode mode) > > { > > +#ifdef CONFIG_HOTPLUG_CPU > > struct hrtimer_cpu_base *new_cpu_base, *old_cpu_base, *this_cpu_base; > > struct hrtimer_clock_base *new_base; > > int cpu; > > @@ -1238,6 +1239,7 @@ static void enqueue_hrtimer_offline(struct hrtimer *timer, > > > > if (enqueue_hrtimer(timer, new_base, mode)) > > smp_call_function_single_async(cpu, &new_cpu_base->csd); > > +#endif > > } > > I was thinking about something like that. Can I fold that and add your > SOB? Works for me! Thanx, Paul
5c0930ccaad5 ("hrtimers: Push pending hrtimers away from outgoing CPU earlier") was introduced to fix stalls with scheduler bandwidth timers getting migrated while some kthreads handling CPU hotplug rely on bandwidth. However this has introduced several other issues which used to be confined to RCU. But not anymore as it is spreading to hotplug code itself (https://lore.kernel.org/all/20241213203739.1519801-1-usamaarif642@gmail.com/) Instead of introducing yet another new hackery, fix the problem in hrtimers for everyone. Frederic Weisbecker (3): hrtimers: Force migrate away hrtimers queued after CPUHP_AP_HRTIMERS_DYING rcu: Remove swake_up_one_online() bandaid Revert "rcu/nocb: Fix rcuog wake-up from offline softirq" include/linux/hrtimer_defs.h | 1 + kernel/rcu/tree.c | 34 +------------------- kernel/rcu/tree_exp.h | 2 +- kernel/rcu/tree_nocb.h | 10 ++---- kernel/time/hrtimer.c | 60 +++++++++++++++++++++++++++++++----- 5 files changed, 58 insertions(+), 49 deletions(-)