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@xxxxxxxxx/)
> > > >
> > > > 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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > Cc: Vlad Poenaru <vlad.wing@xxxxxxxxx>
> > Cc: Usama Arif <usamaarif642@xxxxxxxxx>
> > Cc: Frederic Weisbecker <frederic@xxxxxxxxxx>
> >
> > 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
