Re: [PATCH v6 04/12] smp: Use task-local IPI cpumask in smp_call_function_many_cond()

[Chuyi Zhou]

On 2026-06-03 6:54 p.m., Sebastian Andrzej Siewior wrote:
> On 2026-05-28 23:13:30 [+0800], Chuyi Zhou wrote:
>> --- a/include/linux/smp.h
>> +++ b/include/linux/smp.h
>> @@ -167,6 +167,11 @@ void smp_call_function_many(const struct cpumask *mask,
>>   int smp_call_function_any(const struct cpumask *mask,
>>   			  smp_call_func_t func, void *info, int wait);
>>   
>> +#ifdef CONFIG_PREEMPTION
>> +int smp_task_ipi_mask_alloc(struct task_struct *task);
>> +void smp_task_ipi_mask_free(struct task_struct *task);
>> +#endif
>> +
>>   void kick_all_cpus_sync(void);
>>   void wake_up_all_idle_cpus(void);
>>   bool cpus_peek_for_pending_ipi(const struct cpumask *mask);
>> @@ -310,4 +315,14 @@ bool csd_lock_is_stuck(void);
>>   static inline bool csd_lock_is_stuck(void) { return false; }
>>   #endif
>>   
>> +#if !defined(CONFIG_SMP) || !defined(CONFIG_PREEMPTION)
>> +static inline int smp_task_ipi_mask_alloc(struct task_struct *task)
>> +{
>> +	return 0;
>> +}
>> +static inline void smp_task_ipi_mask_free(struct task_struct *task)
>> +{
>> +}
>> +#endif
>> +
> 

Yes, that looks cleaner.

> It might make sense to move them closer together after
> CONFIG_UP_LATE_INIT so you have
> 
> #if defined(CONFIG_PREEMPTION) && defined(CONFIG_SMP)
> int smp_task_ipi_mask_alloc(struct task_struct *task);
> void smp_task_ipi_mask_free(struct task_struct *task);
> 
> #else
> static inline int smp_task_ipi_mask_alloc(struct task_struct *task)
> {
> 	return 0;
> }
> static inline void smp_task_ipi_mask_free(struct task_struct *task) { }
> #endif
> 
> …
> 
>> --- a/kernel/smp.c
>> +++ b/kernel/smp.c
>> @@ -1010,6 +1061,9 @@ EXPORT_SYMBOL(nr_cpu_ids);
>>   void __init setup_nr_cpu_ids(void)
>>   {
>>   	set_nr_cpu_ids(find_last_bit(cpumask_bits(cpu_possible_mask), NR_CPUS) + 1);
>> +
>> +	if (IS_ENABLED(CONFIG_PREEMPTION) && cpumask_size() <= sizeof(unsigned long))
>> +		static_branch_enable(&ipi_mask_inlined);
> Nitpicks:
> We restrict the inline case to a large enough size.
> smp_call_function_many_cond() can deal with with a NULL pointer and will
> use the per-CPU mask in this cases and not enable PREEMPTION early.
> 
> This happens for instance on !PREEMPT kernels and request issued by the
> init task which is defined at compile at init/init_task.c. Not sure if
> this is a problem as it did not trigger is testing.
> Just to let you know.
> 
> The inline condition is based on cpumask_size() which uses
> large_cpumask_bits. This is used by cpumask_copy() and cpumask_clear()
> because the underlying operation can be optimized if the size is a
> constant.
> large_cpumask_bits remains a constant as long as CONFIG_NR_CPUS is <=
> 256 on 64bit mirroring CONFIG_NR_CPUS. That means if you boot this on a
> 4 core CPU then it will not inline the operation if CONFIG_NR_CPUS is
> say 128 to cope with larger machines. Debian for instance uses here
> NR_CPUS=8192 so it will inline it.
> 
> All the cpumask operation that are used by smp_call_function_many_cond()
> for this task_mask are based on small_cpumask_bits. It could be used to
> allow the inline case if CONFIG_NR_CPUS=256 but boot with 8 CPUs. It
> would risk breakage if the code changes one does
> cpumask_clear(task_mask).
> 
> Just two things that I noticed while looking at it.
> 

Thanks for pointing this out.

The NULL fallback in smp_call_function_many_cond() is intentional for 
cases where no task-local mask is available, such as !PREEMPT kernels or 
static/early tasks. In those cases it falls back to the per-CPU mask and 
keeps the existing preemption-disabled behavior.

Using cpumask_size() is conservative, but it ensures that the inline 
storage is large enough for cpumask operations which may use 
large_cpumask_bits, such as cpumask_clear() or cpumask_copy(), and does 
not rely on this path continuing to only use small_cpumask_bits based 
operations. So I think keeping the cpumask_size() check is the safer 
tradeoff here.


> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> 
>>   }
>>   
>>   /* Called by boot processor to activate the rest. */
> 
> Sebastian