Re: [PATCH v3 10/45] smp: Use get/put_online_cpus_atomic() to prevent CPU offline

On 07/02/2013 02:17 PM, Michael Wang wrote:

On 07/02/2013 04:25 PM, Srivatsa S. Bhat wrote:

quoted

Hi Michael,

On 07/02/2013 11:02 AM, Michael Wang wrote:

quoted

Hi, Srivatsa

On 06/28/2013 03:54 AM, Srivatsa S. Bhat wrote:
[snip]

quoted

@@ -625,8 +632,9 @@ EXPORT_SYMBOL(on_each_cpu_mask);
  * The function might sleep if the GFP flags indicates a non
  * atomic allocation is allowed.
  *
- * Preemption is disabled to protect against CPUs going offline but not online.
- * CPUs going online during the call will not be seen or sent an IPI.
+ * We use get/put_online_cpus_atomic() to protect against CPUs going
+ * offline but not online. CPUs going online during the call will
+ * not be seen or sent an IPI.

I was a little confused about this comment, if the offline-cpu still
have chances to become online, then there is chances that we will pick
it from for_each_online_cpu(), isn't it? Did I miss some point?

Whether or not the newly onlined CPU is observed in our for_each_online_cpu()
loop, is dependent on timing. On top of that, there are 2 paths in the code:
one which uses a temporary cpumask and the other which doesn't. In the former
case, if a CPU comes online _after_ we populate the temporary cpumask, then
we won't send an IPI to that cpu, since the temporary cpumask doesn't contain
that CPU. Whereas, if we observe the newly onlined CPU in the for_each_online_cpu()
loop itself (either in the former or the latter case), then yes, we will send
the IPI to that CPU.

So it is not 'during the call' but 'during the call of
on_each_cpu_mask()', correct?

Well, as I said, its timing dependent. We might miss the newly onlined CPU in
the for_each_online_cpu() loop itself, based on when exactly the CPU was added
to the cpu_online_mask. So you can't exactly pin-point the places where you'll
miss the CPU and where you won't. Besides, is it _that_ important? It is after
all unpredictable..

The comment position seems like it declaim that during the call of this
func, online-cpu won't be seem and send IPI...

Doesn't matter, AFAICS. The key take-away from that whole comment is: nothing is
done to prevent CPUs from coming online while the function is running, whereas
the online CPUs are guaranteed to remain online throughout the function. In other
words, its a weaker form of get_online_cpus()/put_online_cpus(), providing a
one-way synchronization (CPU offline).

As long as that idea is conveyed properly, the purpose of that comment is served,
IMHO.

Regards,
Srivatsa S. Bhat

quoted

quoted

quoted

  *
  * You must not call this function with disabled interrupts or
  * from a hardware interrupt handler or from a bottom half handler.

@@ -641,26 +649,26 @@ void on_each_cpu_cond(bool (*cond_func)(int cpu, void *info),
 	might_sleep_if(gfp_flags & __GFP_WAIT);

 	if (likely(zalloc_cpumask_var(&cpus, (gfp_flags|__GFP_NOWARN)))) {
-		preempt_disable();
+		get_online_cpus_atomic();
 		for_each_online_cpu(cpu)
 			if (cond_func(cpu, info))
 				cpumask_set_cpu(cpu, cpus);
 		on_each_cpu_mask(cpus, func, info, wait);
-		preempt_enable();
+		put_online_cpus_atomic();
 		free_cpumask_var(cpus);
 	} else {
 		/*
 		 * No free cpumask, bother. No matter, we'll
 		 * just have to IPI them one by one.
 		 */
-		preempt_disable();
+		get_online_cpus_atomic();
 		for_each_online_cpu(cpu)
 			if (cond_func(cpu, info)) {
 				ret = smp_call_function_single(cpu, func,
 								info, wait);
 				WARN_ON_ONCE(!ret);
 			}
-		preempt_enable();
+		put_online_cpus_atomic();
 	}
 }
 EXPORT_SYMBOL(on_each_cpu_cond);