Re: [PATCH 1/3] smp/ipi: Remove redundant cfd->cpumask_ipi mask

[Preeti U Murthy <preeti@linux.vnet.ibm.com>]

Hi Wang,

On 07/06/2013 08:43 AM, Wang YanQing wrote:
> On Fri, Jul 05, 2013 at 09:57:01PM +0530, Preeti U Murthy wrote:
>> cfd->cpumask_ipi is used only in smp_call_function_many().The existing
>> comment around it says that this additional mask is used because
>> cfd->cpumask can get overwritten.
>>
>> There is no reason why the cfd->cpumask can be overwritten, since this
>> is a per_cpu mask; nobody can change it but us and we are
>> called with preemption disabled.
> 
> The ChangeLog for f44310b98ddb7f0d06550d73ed67df5865e3eda5
> which import cfd->cpumask_ipi saied the reason why we need
> it:
> 
> "    As explained by Linus as well:
>     
>      |
>      | Once we've done the "list_add_rcu()" to add it to the
>      | queue, we can have (another) IPI to the target CPU that can
>      | now see it and clear the mask.
>      |
>      | So by the time we get to actually send the IPI, the mask might
>      | have been cleared by another IPI.

I am unable to understand where the cfd->cpumask of the source cpu is
getting cleared. Surely not by itself, since it is preempt disabled.
Also why should it get cleared?

The idea behind clearing a source CPU's cfd->cpumask AFAICS, could be
that the source cpu should not send an IPI to the target if the target
has already received an IPI from another CPU. The reason being that the
target would execute the already queued csds, hence would not need
another IPI to see its queue.

If the above is the intention of clearing the cfd->cpumask of the source
cpu, why is the mechanism not consistent with what happens in
generic_exec_single(), where in an ipi is decided to be sent if there
are no previous queued csds on the target?

Also why is it that in the wait condition under
smp_call_function_many(), cfd->cpumask continues to be used and not
cfd->cpumask_ipi ?

>      |
>     
>     This patch also fixes a system hang problem, if the data->cpumask
>     gets cleared after passing this point:
>     
>             if (WARN_ONCE(!mask, "empty IPI mask"))
>                     return;
>     
>     then the problem in commit 83d349f35e1a ("x86: don't send an IPI to
>     the empty set of CPU's") will happen again.
> "
> So this patch is wrong.
> 
> And you should cc linus and Jan Beulich who give acked-by tag to
> the commit.
> 
> Thanks.
> 

Thank you

Regards
Preeti U Murthy