From: David Daney <[email protected]>
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 <[email protected]>
---
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
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.
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
On Thu, Aug 8, 2013 at 11:27 PM, David Daney <[email protected]> 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
[email protected]
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
On 08/09/2013 11:51 AM, Gilad Ben-Yossef wrote:
> On Thu, Aug 8, 2013 at 11:27 PM, David Daney <[email protected]> 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
On Fri, 9 Aug 2013 12:24:56 -0700 David Daney <[email protected]> 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 :(