[PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().

[PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().

[David Daney]

From: David Daney <david.daney@cavium.com>

As in f21afc25f9ed (smp.h: Use local_irq_{save,restore}() in !SMP
version of on_each_cpu().), we don't want to enable irqs if they are
not already enabled.

I don't know of any bugs currently caused by this unconditional
local_irq_enable(), but I want to use this function in MIPS/OCTEON
early boot (when we have early_boot_irqs_disabled).  This also makes
this function have similar semantics to on_each_cpu() which is good in
itself.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 kernel/smp.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 91c52ab..97edbbe 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -578,8 +578,10 @@ EXPORT_SYMBOL(on_each_cpu);
  *
  * If @wait is true, then returns once @func has returned.
  *
- * You must not call this function with disabled interrupts or
- * from a hardware interrupt handler or from a bottom half handler.
+ * You must not call this function with disabled interrupts or from a
+ * hardware interrupt handler or from a bottom half handler.  The
+ * exception is that it may be used during early boot while
+ * early_boot_irqs_disabled is set.
  */
 void on_each_cpu_mask(const struct cpumask *mask, smp_call_func_t func,
 			void *info, bool wait)
@@ -588,9 +590,10 @@ void on_each_cpu_mask(const struct cpumask *mask, smp_call_func_t func,
 
 	smp_call_function_many(mask, func, info, wait);
 	if (cpumask_test_cpu(cpu, mask)) {
-		local_irq_disable();
+		unsigned long flags;
+		local_irq_save(flags);
 		func(info);
-		local_irq_enable();
+		local_irq_restore(flags);
 	}
 	put_cpu();
 }
-- 
1.7.11.7

Re: [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().

[Christoph Lameter]

On Thu, 8 Aug 2013, David Daney wrote:

> I don't know of any bugs currently caused by this unconditional
> local_irq_enable(), but I want to use this function in MIPS/OCTEON
> early boot (when we have early_boot_irqs_disabled).  This also makes
> this function have similar semantics to on_each_cpu() which is good in
> itself.

smp_call_function_many() wants interrupts enabled.

Re: [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().

[David Daney]

On 08/08/2013 12:25 PM, Christoph Lameter wrote:
> On Thu, 8 Aug 2013, David Daney wrote:
>
>> I don't know of any bugs currently caused by this unconditional
>> local_irq_enable(), but I want to use this function in MIPS/OCTEON
>> early boot (when we have early_boot_irqs_disabled).  This also makes
>> this function have similar semantics to on_each_cpu() which is good in
>> itself.
>
> smp_call_function_many() wants interrupts enabled.

That's what the comments say, but it isn't actually true.

The usage introduced by the patch is no different than the existing 
usage in on_each_cpu() 30 line up in the file.

David Daney

Re: [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().

[Gilad Ben-Yossef]

On Thu, Aug 8, 2013 at 11:27 PM, David Daney <ddaney@caviumnetworks.com> wrote:
>
> On 08/08/2013 12:25 PM, Christoph Lameter wrote:
>>
>> On Thu, 8 Aug 2013, David Daney wrote:
>>
>>> I don't know of any bugs currently caused by this unconditional
>>> local_irq_enable(), but I want to use this function in MIPS/OCTEON
>>> early boot (when we have early_boot_irqs_disabled).  This also makes
>>> this function have similar semantics to on_each_cpu() which is good in
>>> itself.
>>
>>
>> smp_call_function_many() wants interrupts enabled.
>
>
> That's what the comments say, but it isn't actually true.
>
> The usage introduced by the patch is no different than the existing usage in on_each_cpu() 30 line up in the file.
>

Regardless of the question of how smp_call_function_many() should be
called, the IRQ disable/enable pair is actually there to make sure the
provided function runs on the current CPU at the same conditions as it
would get called via the IPI.

I would at least consider putting a test there to make sure IRQs
really are disabled when entering the function, otherwise the bugs
stemming from incorrect use can be tricky to catch.

Just my 0.00002 BTC
Gilad



--
Gilad Ben-Yossef
Chief Coffee Drinker
gilad@benyossef.com
Israel Cell: +972-52-8260388
US Cell: +1-973-8260388
http://benyossef.com

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().

[David Daney]

On 08/09/2013 11:51 AM, Gilad Ben-Yossef wrote:
> On Thu, Aug 8, 2013 at 11:27 PM, David Daney <ddaney@caviumnetworks.com> wrote:
>>
>> On 08/08/2013 12:25 PM, Christoph Lameter wrote:
>>>
>>> On Thu, 8 Aug 2013, David Daney wrote:
>>>
>>>> I don't know of any bugs currently caused by this unconditional
>>>> local_irq_enable(), but I want to use this function in MIPS/OCTEON
>>>> early boot (when we have early_boot_irqs_disabled).  This also makes
>>>> this function have similar semantics to on_each_cpu() which is good in
>>>> itself.
>>>
>>>
>>> smp_call_function_many() wants interrupts enabled.
>>
>>
>> That's what the comments say, but it isn't actually true.
>>
>> The usage introduced by the patch is no different than the existing usage in on_each_cpu() 30 line up in the file.
>>
>
> Regardless of the question of how smp_call_function_many() should be
> called, the IRQ disable/enable pair is actually there to make sure the
> provided function runs on the current CPU at the same conditions as it
> would get called via the IPI.

Yes, I am aware of that.

You will notice that my patch doesn't change in any way the state of the 
system while running the provided function, irqs are still disabled, 
just as they are in the current implementation.

>
> I would at least consider putting a test there to make sure IRQs
> really are disabled when entering the function,

I think this may be a false predicate.  on_each_cpu_mask() is usually 
called with IRQs enabled...

> otherwise the bugs
> stemming from incorrect use can be tricky to catch.
>

... all my patch does is allow on_each_cpu_mask() to be called with IRQs 
disabled if we are in Early Boot.  This is already the case with 
smp_call_function(), smp_call_function_many() and on_each_cpu().  I am 
arguing that for the sake of consistency and the principle that function 
behavior shouldn't be surprising, that we make on_each_cpu_mask() work 
the same way.

Any required preconditions on the state of the system when calling 
on_each_cpu_mask() are already verified as it unconditionally calls 
smp_call_function_many() which has a lot of conditions it warns on. 
Additional tests in the callers of smp_call_function_many() would only 
be redundant.


David Daney

Re: [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().

[Andrew Morton]

On Fri, 9 Aug 2013 12:24:56 -0700 David Daney <ddaney@caviumnetworks.com> wrote:

> > otherwise the bugs
> > stemming from incorrect use can be tricky to catch.
> >
> 
> ... all my patch does is allow on_each_cpu_mask() to be called with IRQs 
> disabled if we are in Early Boot.  This is already the case with 
> smp_call_function(), smp_call_function_many() and on_each_cpu().  I am 
> arguing that for the sake of consistency and the principle that function 
> behavior shouldn't be surprising, that we make on_each_cpu_mask() work 
> the same way.

Yup, the check in smp_call_function_many() will tell us if anyone calls
on_each_cpu_mask() with interrupts disabled any time after boot.

The whole early_boot_irqs_disabled thing is of course totally vile :(