Re: [PATCH v5 04/24] cpumask: Introduce cpu_preferred_mask

[Yury Norov <yury.norov@gmail.com>]

On Fri, Jun 26, 2026 at 11:39:01AM +0200, Peter Zijlstra wrote:
> On Thu, Jun 25, 2026 at 06:16:28PM +0530, Shrikanth Hegde wrote:
> 
> > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> > index 80211900f373..5a643d608ea6 100644
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -120,12 +120,20 @@ extern struct cpumask __cpu_enabled_mask;
> >  extern struct cpumask __cpu_present_mask;
> >  extern struct cpumask __cpu_active_mask;
> >  extern struct cpumask __cpu_dying_mask;
> > +
> > +#ifdef CONFIG_PREFERRED_CPU
> > +extern struct cpumask __cpu_preferred_mask;
> > +#else
> > +#define __cpu_preferred_mask __cpu_active_mask
> > +#endif
> 
> This is cure, but does it not result in set_cpu_preferred() changing
> active mask, and it that not somewhat unexpected behaviour?

I agree, and I think I already commented on it on previous round.
set_cpu_preferred() should be protected the same way as the
corresponding mask, and should be a NOP when CONFIG_PREFERRED_CPU
is disabled.

> >  #define cpu_possible_mask ((const struct cpumask *)&__cpu_possible_mask)
> >  #define cpu_online_mask   ((const struct cpumask *)&__cpu_online_mask)
> >  #define cpu_enabled_mask   ((const struct cpumask *)&__cpu_enabled_mask)
> >  #define cpu_present_mask  ((const struct cpumask *)&__cpu_present_mask)
> >  #define cpu_active_mask   ((const struct cpumask *)&__cpu_active_mask)
> >  #define cpu_dying_mask    ((const struct cpumask *)&__cpu_dying_mask)
> > +#define cpu_preferred_mask ((const struct cpumask *)&__cpu_preferred_mask)
> >  
> >  extern atomic_t __num_online_cpus;
> >  extern unsigned int __num_possible_cpus;
> 
> > diff --git a/kernel/cpu.c b/kernel/cpu.c
> > index bc4f7a9ba64e..d623a9c5554a 100644
> > --- a/kernel/cpu.c
> > +++ b/kernel/cpu.c
> > @@ -3107,6 +3107,11 @@ EXPORT_SYMBOL(__cpu_dying_mask);
> >  atomic_t __num_online_cpus __read_mostly;
> >  EXPORT_SYMBOL(__num_online_cpus);
> >  
> > +#ifdef CONFIG_PREFERRED_CPU
> > +struct cpumask __cpu_preferred_mask __read_mostly;
> > +EXPORT_SYMBOL(__cpu_preferred_mask);
> > +#endif
> 
> Precedent is definitely towards !GPL exports for this, but could we get
> away with making this one GPL?
> 
> 
> > @@ -3164,6 +3169,7 @@ void __init boot_cpu_init(void)
> >  	/* Mark the boot cpu "present", "online" etc for SMP and UP case */
> >  	set_cpu_online(cpu, true);
> >  	set_cpu_active(cpu, true);
> > +	set_cpu_preferred(cpu, true);
> 
> This sets active twice, which is harmless, but wasteful...

I think, the good criteria for correctness of this series would be the
identical binaries before the series, and when CONFIG_PREFERRED_CPU is
off. At least, as a mental model. This double-set chunk breaks that
model. 

Thanks,
Yury
 
> >  	set_cpu_present(cpu, true);
> >  	set_cpu_possible(cpu, true);
> >  
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 2f4530eb543f..9e16946c9d62 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -8685,6 +8685,9 @@ int sched_cpu_activate(unsigned int cpu)
> >  	 */
> >  	sched_set_rq_online(rq, cpu);
> >  
> > +	/* preferred is subset of active and follows its state */
> > +	set_cpu_preferred(cpu, true);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -8698,6 +8701,8 @@ int sched_cpu_deactivate(unsigned int cpu)
> >  	if (ret)
> >  		return ret;
> >  
> > +	set_cpu_preferred(cpu, false);
> > +
> >  	/*
> >  	 * Remove CPU from nohz.idle_cpus_mask to prevent participating in
> >  	 * load balancing when not active
> 
> But this one clears active earlier, is that not a problem?
> 
> Perhaps it is best if the modifier is a no-op when preferred mask does
> not exist?