All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().
@ 2013-08-08 18:25 David Daney
  2013-08-08 19:25 ` Christoph Lameter
  0 siblings, 1 reply; 6+ messages in thread
From: David Daney @ 2013-08-08 18:25 UTC (permalink / raw)
  To: Andrew Morton, Gilad Ben-Yossef, linux-kernel
  Cc: Christoph Lameter, Chris Metcalf, Peter Zijlstra, 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().
  2013-08-08 18:25 [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask() David Daney
@ 2013-08-08 19:25 ` Christoph Lameter
  2013-08-08 20:27   ` David Daney
  0 siblings, 1 reply; 6+ messages in thread
From: Christoph Lameter @ 2013-08-08 19:25 UTC (permalink / raw)
  To: David Daney
  Cc: Andrew Morton, Gilad Ben-Yossef, linux-kernel, Chris Metcalf,
	Peter Zijlstra, David Daney

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().
  2013-08-08 19:25 ` Christoph Lameter
@ 2013-08-08 20:27   ` David Daney
  2013-08-09 18:51     ` Gilad Ben-Yossef
  0 siblings, 1 reply; 6+ messages in thread
From: David Daney @ 2013-08-08 20:27 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: David Daney, Andrew Morton, Gilad Ben-Yossef, linux-kernel,
	Chris Metcalf, Peter Zijlstra, 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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().
  2013-08-08 20:27   ` David Daney
@ 2013-08-09 18:51     ` Gilad Ben-Yossef
  2013-08-09 19:24       ` David Daney
  0 siblings, 1 reply; 6+ messages in thread
From: Gilad Ben-Yossef @ 2013-08-09 18:51 UTC (permalink / raw)
  To: David Daney
  Cc: Christoph Lameter, David Daney, Andrew Morton, linux-kernel,
	Chris Metcalf, Peter Zijlstra, David Daney

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().
  2013-08-09 18:51     ` Gilad Ben-Yossef
@ 2013-08-09 19:24       ` David Daney
  2013-08-12 22:16         ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: David Daney @ 2013-08-09 19:24 UTC (permalink / raw)
  To: Gilad Ben-Yossef
  Cc: Christoph Lameter, David Daney, Andrew Morton, linux-kernel,
	Chris Metcalf, Peter Zijlstra, 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().
  2013-08-09 19:24       ` David Daney
@ 2013-08-12 22:16         ` Andrew Morton
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2013-08-12 22:16 UTC (permalink / raw)
  To: David Daney
  Cc: Gilad Ben-Yossef, Christoph Lameter, David Daney, linux-kernel,
	Chris Metcalf, Peter Zijlstra, David Daney

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 :(

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-08-12 22:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-08 18:25 [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask() David Daney
2013-08-08 19:25 ` Christoph Lameter
2013-08-08 20:27   ` David Daney
2013-08-09 18:51     ` Gilad Ben-Yossef
2013-08-09 19:24       ` David Daney
2013-08-12 22:16         ` Andrew Morton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.