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

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Apr 10 2025 at 12:38, Gabriele Monaco wrote:
> On Thu, 2025-04-10 at 10:26 +0200, Thomas Gleixner wrote:
>> > + /* 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).

This should be prevented by the core infrastructure. Isolating _ALL_
CPUs is broken by definition.

>> 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.

Indeed.
>> > +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..

Correct. cpumask_var_t is the magic macro construct which allows to
switch from cpumask on stack to allocated ones at compile time, but
what's handed around is a pointer to struct cpumask.

Thanks,

        tglx