Re: [PATCH 2/6] sched: add function to execute a function synchronously on a physical cpu

[Juergen Gross]

On 11/03/16 13:19, Peter Zijlstra wrote:
> On Fri, Mar 11, 2016 at 12:59:30PM +0100, Juergen Gross wrote:
>> +int call_sync_on_phys_cpu(unsigned cpu, int (*func)(void *), void *par)
>> +{
>> +	cpumask_var_t old_mask;
>> +	int ret;
>> +
>> +	if (cpu >= nr_cpu_ids)
>> +		return -EINVAL;
>> +
>> +	if (!alloc_cpumask_var(&old_mask, GFP_KERNEL))
>> +		return -ENOMEM;
>> +
>> +	cpumask_copy(old_mask, &current->cpus_allowed);
>> +	ret = set_cpus_allowed_ptr(current, cpumask_of(cpu));
>> +	if (ret)
>> +		goto out;
> 
> So what happens if someone does sched_setaffinity() right about here?

Aah, okay. Seems I should add preempt_disable() in this patch already
and retry the set_cpus_allowed_ptr() in case cpus_allowed has changed.

> 
>> +
>> +	ret = func(par);
>> +
>> +	set_cpus_allowed_ptr(current, old_mask);
>> +
>> +out:
>> +	free_cpumask_var(old_mask);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(call_sync_on_phys_cpu);
> 
> This is disgusting, and you're adding this to !Xen kernels too.

Sure. It is called on !Xen kernels too. Without Xen it is just omitting
the vcpu pinning. I've copied above logic from dcdbas/i8k (it was open
coded twice).

BTW: I tried to get rid of the complete logic to call a function on cpu
0 only. It was rejected by the Dell maintainers.


Juergen