Re: [PATCH] timers: Exclude isolated cpus from timer migation

[Gabriele Monaco <gmonaco@redhat.com>]

On Thu, 2025-04-10 at 10:26 +0200, Thomas Gleixner wrote:
> On Thu, Apr 10 2025 at 08:54, Gabriele Monaco wrote:
> >  
> > +/*  cpumask excluded from migration */
> > +static cpumask_var_t tmigr_unavailable_cpumask;
> 
> Why is this a negated mask instead of being the obvious and intuitive
> available mask?

Well, the way I started writing the patch I found it easier to do the
double andnot in tmigr_isolated_exclude_cpumask to see what changed.
I see the way it evolved is just messier..
I'll apply your solution which seems much neater!

> 
> >   if (firstexp != KTIME_MAX) {
> > - migrator = cpumask_any_but(cpu_online_mask, cpu);
> > + migrator = cpumask_nth_andnot(0, cpu_possible_mask,
> > +       tmigr_unavailable_cpumask);
> 
> That's exactly what this negated mask causes: incomprehensible code.
> 
>  cpumask_clear_cpu(cpu, available_mask);
>         ...               
>  migrator = cpumask_first(available_mask);
> 
> is too simple and obvious, right?
> 
> > + /* Fall back to any online in case all are isolated. */
> 
> How can that happen? There is always at least _ONE_ housekeeping,
> non-isolated, CPU online, no?
> 

In my understanding it shouldn't, but I'm not sure there's anything
preventing the user from isolating everything via cpuset.
Anyway that's something no one in their mind should do, so I guess I'd
just opt for the cpumask_first (or actually cpumask_any, like before
the change).

> > + if (migrator >= nr_cpu_ids)
> > + migrator = cpumask_any_but(cpu_online_mask, cpu);
> >   work_on_cpu(migrator, tmigr_trigger_active, NULL);
> >   }
> >  
> >   return 0;
> >  }
> >  
> > -static int tmigr_cpu_online(unsigned int cpu)
> > +static int tmigr_cpu_available(unsigned int cpu)
> >  {
> > - struct tmigr_cpu *tmc = this_cpu_ptr(&tmigr_cpu);
> > + struct tmigr_cpu *tmc = per_cpu_ptr(&tmigr_cpu, cpu);
> >  
> >   /* Check whether CPU data was successfully initialized */
> >   if (WARN_ON_ONCE(!tmc->tmgroup))
> >   return -EINVAL;
> >  
> > + /* Silently ignore online requests if isolated */
> 
> This comment makes no sense.
> 
>      /* Isolated CPUs are not participating in timer migration */
> 
> makes it entirely clear what this is about, no?
> 
> That brings me to the general design decision here. Your changelog
> explains at great length WHAT the change is doing, but completely
> fails
> to explain the consequences and the rationale why this is the right
> thing to do.
> 
> By excluding the isolated CPUs from migration completely, any
> 'global'
> timer, which is armed on such a CPU, has to be expired on that
> isolated
> CPU. That's fundamentaly different from e.g. RCU isolation.
> 
> It might be the right thing to do and harmless, but without a proper
> explanation it's a silent side effect of your changes, which leaves
> people scratching their heads.

Mmh, essentially the idea is that global timer should not migrate from
housekeeping to isolated cores. I assumed the opposite never occurs (as
global timers /should/ not even start on isolated cores on a properly
isolated system), but you're right, that's not quite true.

Thinking about it now, since global timers /can/ start on isolated
cores, that makes them quite different from offline ones and probably
considering them the same is just not the right thing to do..

I'm going to have a deeper thought about this whole approach, perhaps
something simpler just preventing migration in that one direction would
suffice.

> 
> > + if (cpu_is_isolated(cpu))
> > + return 0;
> >   raw_spin_lock_irq(&tmc->lock);
> > - trace_tmigr_cpu_online(tmc);
> > + trace_tmigr_cpu_available(tmc);
> >   tmc->idle = timer_base_is_idle();
> >   if (!tmc->idle)
> >   __tmigr_cpu_activate(tmc);
> > - tmc->online = true;
> > + tmc->available = true;
> > + tmc->idle = true;
> 
> How so?
> 
> > + cpumask_clear_cpu(cpu, tmigr_unavailable_cpumask);
> >   raw_spin_unlock_irq(&tmc->lock);
> >   return 0;
> >  }
> >  
> > +int tmigr_isolated_exclude_cpumask(cpumask_var_t exclude_cpumask)
> 
> cpumask_var_t is wrong here. 'const struct cpumask *' is what you
> want.

You mean in the function argument? That's exactly how it is handled in
workqueue_unbound_exclude_cpumask. I get cpumask_var_t is not
necessarily a pointer, perhaps it's worth changing it there too..
Or am I missing your point?

> 
> > +{
> > + int cpu;
> > + cpumask_var_t cpumask;
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#coding-style-notes
> 
> > + if (!zalloc_cpumask_var(&cpumask, GFP_KERNEL))
> > + return -ENOMEM;
> > +
> > + cpumask_copy(cpumask, tmigr_unavailable_cpumask);
> 
> What serializes this against concurrent CPU hotplug? I assume it's
> done
> by the caller, but then this code should have a lockdep assert to
> validate it. If it's not, then this is broken.
> 
> As it is serialized it does not need a copy either, right?
> 
> > + /* Was not excluded but should be excluded now. */
> > + for_each_cpu_andnot(cpu, exclude_cpumask, cpumask)
> > + tmigr_cpu_unavailable(cpu);
> > +
> > + /* Was excluded but should be included now */
> > + for_each_cpu_andnot(cpu, cpumask, exclude_cpumask)
> > + if (cpu_online(cpu))
> > + tmigr_cpu_available(cpu);
> 
> My brain hurts by now.
> 
>          for_each_cpu_and(cpu, available_mask, exclude_mask)
>           tmigr_cpu_unavailable(cpu);
> 
>          for_each_cpu_andnot(cpu, cpu_online_mask, exclude_mask) {
>           if (!cpumask_test_cpu(cpu, available_mask))
>                  tmigr_cpu_available(cpu);
>          }
> 
> No?
> 
> Also this patch is doing too many things at once. It want's to be
> split
> into:
> 
>     Patch 1: Rename 'online' to 'available' (bit and function names)
>     Patch 2: Add the available mask logic
>     Patch 3: Add the isolation functionality
> 

Good point, I'll keep it in mind if the patch keeps this shape.

Thanks for the feedback,
Gabriele