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

[Thomas Gleixner <tglx@kernel.org>]

On Tue, Jun 16 2026 at 19:11, Chuyi Zhou wrote:
> This patch prepares the task-local IPI cpumask during thread creation, and
> uses the local cpumask to replace the percpu cfd cpumask in
> smp_call_function_many_cond(). We will enable preemption during
> csd_lock_wait() later, and this can prevent concurrent access to the
> cfd->cpumask from other tasks on the current CPU. For cases where
> cpumask_size() is smaller than or equal to the pointer size, it tries to
> stash the cpumask in the pointer itself to avoid extra memory allocations.

This one fails the comprehensible test and also does not match the rules of
how change logs should be written.

> +#if defined(CONFIG_SMP) && defined(CONFIG_PREEMPTION)
> +	union {
> +		cpumask_t                       *ipi_mask_ptr;
> +		unsigned long			ipi_mask_val;

Indentation of the variable name wants TABs not spaces

> @@ -933,10 +934,14 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
>  #endif
>  	account_kernel_stack(tsk, 1);
>  
> -	err = scs_prepare(tsk, node);
> +	err = smp_task_ipi_mask_alloc(tsk);

Hrm. So we unconditionally allocate another per task CPU mask. How many
task actually utilize it?

We keep making task_struct and the related things larger every other
release without actually looking at the resulting overall memory
consumption.

> +static DEFINE_STATIC_KEY_FALSE(ipi_mask_inlined);
> +
> +#ifdef CONFIG_PREEMPTION
> +
> +int smp_task_ipi_mask_alloc(struct task_struct *task)
> +{
> +	if (static_branch_unlikely(&ipi_mask_inlined))
> +		return 0;
> +
> +	task->ipi_mask_ptr = kmalloc(cpumask_size(), GFP_KERNEL);
> +	if (!task->ipi_mask_ptr)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +void smp_task_ipi_mask_free(struct task_struct *task)
> +{
> +	if (static_branch_unlikely(&ipi_mask_inlined))
> +		return;
> +
> +	kfree(task->ipi_mask_ptr);
> +}
> +
> +static cpumask_t *smp_task_ipi_mask(struct task_struct *cur)
> +{
> +	/*
> +	 * If cpumask_size() is smaller than or equal to the pointer
> +	 * size, it stashes the cpumask in the pointer itself to
> +	 * avoid extra memory allocations.
> +	 */
> +	if (static_branch_unlikely(&ipi_mask_inlined))
> +		return (cpumask_t *)&cur->ipi_mask_val;
> +
> +	return cur->ipi_mask_ptr;
> +}
> +#else
> +static cpumask_t *smp_task_ipi_mask(struct task_struct *cur)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  /*
>   * Flags to be used as scf_flags argument of smp_call_function_many_cond().
>   *
> @@ -811,11 +855,19 @@ static void smp_call_function_many_cond(const struct cpumask *mask,
>  	int cpu, last_cpu, this_cpu = smp_processor_id();
>  	struct call_function_data *cfd;
>  	bool wait = scf_flags & SCF_WAIT;
> +	struct cpumask *cpumask, *task_mask;
>  	int nr_cpus = 0;
>  	bool run_remote = false;

While at it please fix up the variable declaration according to
Documentation so it becomes reverse fir tree layout.

>  
>  	lockdep_assert_preemption_disabled();
>  
> +	task_mask = smp_task_ipi_mask(current);
> +	cfd = this_cpu_ptr(&cfd_data);
> +	if (task_mask)
> +		cpumask = task_mask;
> +	else
> +		cpumask = cfd->cpumask;

Glueing the cfd initialization between task_mask and the conditional is
pointlessly hard to follow. Keep related things together.

Thanks,

        tglx