From: Jiang Liu <[email protected]>
Commit f44310b98ddb7 "smp: Fix SMP function call empty cpu mask race"
introduced field call_function_data->cpumask_ipi to resolve a race
condition in smp_call_function_many().
Later commit 9a46ad6d6df3 "smp: make smp_call_function_many() use logic
similar to smp_call_function_single()" fixed the same issue in another
way when optimizing smp_call_function_many(), which then obsoletes
changes introduced by commit f44310b98ddb7. So revert it.
We may also keep call_function_data->cpumask_ipi field and use it to
optimize smp_call_function_many() as below:
diff --git a/kernel/smp.c b/kernel/smp.c
index fe9f773..dd852fb 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -428,6 +428,8 @@ void smp_call_function_many(const struct cpumask *mask,
csd->info = info;
raw_spin_lock_irqsave(&dst->lock, flags);
+ if (list_empty(&dst->list))
+ cpumask_clear_cpu(cpu, cfd->cpumask_ipi);
list_add_tail(&csd->list, &dst->list);
raw_spin_unlock_irqrestore(&dst->lock, flags);
}
But I have no platforms to verify the optimization effect,
so simpify the code first.
Signed-off-by: Jiang Liu <[email protected]>
Cc: Jiang Liu <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Wang YanQing <[email protected]>
Cc: Shaohua Li <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Paul Gortmaker <[email protected]>
Cc: liguang <[email protected]>
Cc: [email protected]
---
kernel/smp.c | 16 ++--------------
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/kernel/smp.c b/kernel/smp.c
index fe9f773..a034712 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
@@ -255,9 +250,9 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
csd = &__get_cpu_var(csd_data);
csd_lock(csd);
-
csd->func = func;
csd->info = info;
+
generic_exec_single(cpu, csd, wait);
} else {
err = -ENXIO; /* CPU not online */
@@ -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) {
--
1.8.1.2
From: Jiang Liu <[email protected]>
Now the call_single_data data structure is always locked by invoking
csd_lock() before inserting it into corresponding call_single_queue
list, so no need to save and check csd->flags in function
generic_smp_call_function_single_interrupt().
Signed-off-by: Jiang Liu <[email protected]>
Cc: Jiang Liu <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Wang YanQing <[email protected]>
Cc: Shaohua Li <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Paul Gortmaker <[email protected]>
Cc: liguang <[email protected]>
Cc: [email protected]
---
kernel/smp.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/kernel/smp.c b/kernel/smp.c
index a034712..baa2a65 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -181,25 +181,11 @@ void generic_smp_call_function_single_interrupt(void)
while (!list_empty(&list)) {
struct call_single_data *csd;
- unsigned int csd_flags;
csd = list_entry(list.next, struct call_single_data, list);
list_del(&csd->list);
-
- /*
- * 'csd' can be invalid after this call if flags == 0
- * (when called through generic_exec_single()),
- * so save them away before making the call:
- */
- csd_flags = csd->flags;
-
csd->func(csd->info);
-
- /*
- * Unlocked CSDs are valid through generic_exec_single():
- */
- if (csd_flags & CSD_FLAG_LOCK)
- csd_unlock(csd);
+ csd_unlock(csd);
}
}
--
1.8.1.2
From: Jiang Liu <[email protected]>
Function smpboot_thread_schedule() is never defined or used,
so remove it from smp_boot.h.
Signed-off-by: Jiang Liu <[email protected]>
Cc: Jiang Liu <[email protected]>
Cc: Jiang Liu <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Wang YanQing <[email protected]>
Cc: Shaohua Li <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Paul Gortmaker <[email protected]>
Cc: liguang <[email protected]>
Cc: [email protected]
---
include/linux/smpboot.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/linux/smpboot.h b/include/linux/smpboot.h
index 13e9296..d600afb 100644
--- a/include/linux/smpboot.h
+++ b/include/linux/smpboot.h
@@ -47,6 +47,5 @@ struct smp_hotplug_thread {
int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread);
void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread);
-int smpboot_thread_schedule(void);
#endif
--
1.8.1.2
On 2013/9/8 23:22, Jiang Liu wrote:
> From: Jiang Liu <[email protected]>
>
> Now the call_single_data data structure is always locked by invoking
> csd_lock() before inserting it into corresponding call_single_queue
> list, so no need to save and check csd->flags in function
> generic_smp_call_function_single_interrupt().
>
> Signed-off-by: Jiang Liu <[email protected]>
> Cc: Jiang Liu <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Wang YanQing <[email protected]>
> Cc: Shaohua Li <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Paul Gortmaker <[email protected]>
> Cc: liguang <[email protected]>
> Cc: [email protected]
Hi, I've sent a same patch before and it has been applied by Ingo.
See http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?id=46591962cb5bfd2bfb0baf42497119c816503598
> ---
> kernel/smp.c | 16 +---------------
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/kernel/smp.c b/kernel/smp.c
> index a034712..baa2a65 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -181,25 +181,11 @@ void generic_smp_call_function_single_interrupt(void)
>
> while (!list_empty(&list)) {
> struct call_single_data *csd;
> - unsigned int csd_flags;
>
> csd = list_entry(list.next, struct call_single_data, list);
> list_del(&csd->list);
> -
> - /*
> - * 'csd' can be invalid after this call if flags == 0
> - * (when called through generic_exec_single()),
> - * so save them away before making the call:
> - */
> - csd_flags = csd->flags;
> -
> csd->func(csd->info);
> -
> - /*
> - * Unlocked CSDs are valid through generic_exec_single():
> - */
> - if (csd_flags & CSD_FLAG_LOCK)
> - csd_unlock(csd);
> + csd_unlock(csd);
> }
> }
>
>
On Sun, Sep 08, 2013 at 11:22:23PM +0800, Jiang Liu wrote:
> From: Jiang Liu <[email protected]>
>
> Commit f44310b98ddb7 "smp: Fix SMP function call empty cpu mask race"
> introduced field call_function_data->cpumask_ipi to resolve a race
> condition in smp_call_function_many().
>
> Later commit 9a46ad6d6df3 "smp: make smp_call_function_many() use logic
> similar to smp_call_function_single()" fixed the same issue in another
> way when optimizing smp_call_function_many(), which then obsoletes
> changes introduced by commit f44310b98ddb7. So revert it.
>
Yes, you are right, after commit 9a46ad6d6df3, we can revert f44310b98ddb7.
Maybe use "Revert smp: Fix SMP function call empty cpu mask race" is a better
subject.
> We may also keep call_function_data->cpumask_ipi field and use it to
> optimize smp_call_function_many() as below:
> diff --git a/kernel/smp.c b/kernel/smp.c
> index fe9f773..dd852fb 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -428,6 +428,8 @@ void smp_call_function_many(const struct cpumask *mask,
> csd->info = info;
>
> raw_spin_lock_irqsave(&dst->lock, flags);
> + if (list_empty(&dst->list))
> + cpumask_clear_cpu(cpu, cfd->cpumask_ipi);
> list_add_tail(&csd->list, &dst->list);
> raw_spin_unlock_irqrestore(&dst->lock, flags);
> }
>
Your optimization don't need to keep cpumask_ipi, just clear cfd->cpumask,
and test whether cfd->cpumask is empty after "for_each_cpu(cpu, cfd->cpumask)".
Acked-by: Wang YanQing <[email protected]>
Thanks.