Re: [PATCH 1/3] smp/ipi: Remove redundant cfd->cpumask_ipi mask
Hi Wang,
On 07/06/2013 11:33 AM, Wang YanQing wrote:
> On Sat, Jul 06, 2013 at 10:59:39AM +0530, Preeti U Murthy wrote:
>> 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?
>
> Assume we have three CPUs: A,B,C
>
> A call smp_call_function_many to notify C do something,
> and current it execute on finished below codes:
>
> "for_each_cpu(cpu, cfd->cpumask) {
> struct call_single_data *csd = per_cpu_ptr(cfd->csd, cpu);
> struct call_single_queue *dst =
> &per_cpu(call_single_queue, cpu);
> unsigned long flags;
>
> csd_lock(csd);
> csd->func = func;
> csd->info = info;
>
> raw_spin_lock_irqsave(&dst->lock, flags);
> list_add_tail(&csd->list, &dst->list);
> raw_spin_unlock_irqrestore(&dst->lock, flags);
> }
> "
> You see "list_add_tail(&csd->list, &dst->list);", it pass the address of csd,
> and A stop before call arch_send_call_function_ipi_mask due interrupt.
>
> At this time B send ipi to C also, then C will see the csd passed by A,
> then C will clear itself in the cfd->cpumask.
Ah ok! Thank you very much for this clarification :)
Regards
Preeti U Murthy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] smp/ipi: Remove redundant cfd->cpumask_ipi mask
On Sat, Jul 06, 2013 at 10:59:39AM +0530, Preeti U Murthy wrote:
> 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?
Assume we have three CPUs: A,B,C
A call smp_call_function_many to notify C do something,
and current it execute on finished below codes:
"for_each_cpu(cpu, cfd->cpumask) {
struct call_single_data *csd = per_cpu_ptr(cfd->csd, cpu);
struct call_single_queue *dst =
&per_cpu(call_single_queue, cpu);
unsigned long flags;
csd_lock(csd);
csd->func = func;
csd->info = info;
raw_spin_lock_irqsave(&dst->lock, flags);
list_add_tail(&csd->list, &dst->list);
raw_spin_unlock_irqrestore(&dst->lock, flags);
}
"
You see "list_add_tail(&csd->list, &dst->list);", it pass the address of csd,
and A stop before call arch_send_call_function_ipi_mask due interrupt.
At this time B send ipi to C also, then C will see the csd passed by A,
then C will clear itself in the cfd->cpumask.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] smp/ipi: Remove redundant cfd->cpumask_ipi mask
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] smp/ipi: Remove redundant cfd->cpumask_ipi mask
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.
|
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] smp/ipi: Remove redundant cfd->cpumask_ipi mask
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.
Signed-off-by: Preeti U Murthy
Cc: Peter Zijlstra
Cc: Ingo Molnar
Cc: Xiao Guangrong
Cc: srivatsa.b...@linux.vnet.ibm.com
Cc: Paul E. McKenney
Cc: Steven Rostedt
Cc: Rusty Russell
---
kernel/smp.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/kernel/smp.c b/kernel/smp.c
index 4dba0f7..89be6e6 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -23,7 +23,6 @@ enum {
struct call_function_data {
struct call_single_data __percpu *csd;
cpumask_var_t cpumask;
- cpumask_var_t cpumask_ipi;
};
static DEFINE_PER_CPU_SHARED_ALIGNED(struct call_function_data, cfd_data);
@@ -47,9 +46,6 @@ hotplug_cfd(struct notifier_block *nfb, unsigned long action,
void *hcpu)
if (!zalloc_cpumask_var_node(&cfd->cpumask, GFP_KERNEL,
cpu_to_node(cpu)))
return notifier_from_errno(-ENOMEM);
- if (!zalloc_cpumask_var_node(&cfd->cpumask_ipi, GFP_KERNEL,
- cpu_to_node(cpu)))
- return notifier_from_errno(-ENOMEM);
cfd->csd = alloc_percpu(struct call_single_data);
if (!cfd->csd) {
free_cpumask_var(cfd->cpumask);
@@ -64,7 +60,6 @@ hotplug_cfd(struct notifier_block *nfb, unsigned long action,
void *hcpu)
case CPU_DEAD:
case CPU_DEAD_FROZEN:
free_cpumask_var(cfd->cpumask);
- free_cpumask_var(cfd->cpumask_ipi);
free_percpu(cfd->csd);
break;
#endif
@@ -410,13 +405,6 @@ void smp_call_function_many(const struct cpumask *mask,
if (unlikely(!cpumask_weight(cfd->cpumask)))
return;
- /*
-* After we put an entry into the list, cfd->cpumask may be cleared
-* again when another CPU sends another IPI for a SMP function call, so
-* cfd->cpumask will be zero.
-*/
- cpumask_copy(cfd->cpumask_ipi, cfd->cpumask);
-
for_each_cpu(cpu, cfd->cpumask) {
struct call_single_data *csd = per_cpu_ptr(cfd->csd, cpu);
struct call_single_queue *dst =
@@ -433,7 +421,7 @@ void smp_call_function_many(const struct cpumask *mask,
}
/* Send a message to all CPUs in the map */
- arch_send_call_function_ipi_mask(cfd->cpumask_ipi);
+ arch_send_call_function_ipi_mask(cfd->cpumask);
if (wait) {
for_each_cpu(cpu, cfd->cpumask) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
