* Re: [patch] Let smp_call_function_single return -EBUSY.
2007-05-14 9:23 [patch] Let smp_call_function_single return -EBUSY Heiko Carstens
@ 2007-05-14 19:11 ` Andrew Morton
2007-05-14 19:18 ` Andrew Morton
2007-05-14 23:24 ` Andi Kleen
2007-06-07 16:04 ` Satyam Sharma
1 sibling, 2 replies; 5+ messages in thread
From: Andrew Morton @ 2007-05-14 19:11 UTC (permalink / raw)
To: Heiko Carstens
Cc: linux-kernel, Ingo Molnar, randy.dunlap, paulus, benh,
David Miller, Andi Kleen
On Mon, 14 May 2007 11:23:17 +0200
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
>
> All architectures that have an implementation of smp_call_function_single
> let it return -EBUSY if it is asked to execute func on the current cpu.
> Therefore the UP version must always return -EBUSY.
>
smp_call_function_single() is a mess.
- it's unclear to me why smp_call_function_single(cpu, ...) doesn't just
call the darn function if cpu==smp_processor_id().
- it's unclear to me why smp_call_function_single(cpu, ...) doesn't just
call the darn function if CONFIG_SMP=n.
- it's unclear to me why smp_call_function_single(cpu, ...) isn't called
smp_call_function_on(cpu, ...)
- the x86_64 version doesn't return -EBUSY: it returns zero. Despite its
claim "Retrurns 0 on success, else a negative status code.".
- i386 _does_ return -EBUSY.
> This of course raises another question: it is not clear in which context
> the smp_call_function* functions are supposed to be called. Should it be
> with preemption disabled or is preemption enabled allowed as well?
> If calling with preemption enabled is allowed then the powerpc implementation
> is broken, since smp_processor_id() as well as num_online_cpus() may change
> while they are accessed.
These are all excellent questions. And important ones. I would suggest:
- May be called with preemption enabled. The implementation must take
care of that.
- May not be called under local_irq_disable() for the usual reasons.
- Will run the callback function under local_irq_disable() (this is
unimportant at present, but if we decide to later change it to run the
callback even on !SMP builds, we'll need to take care of that like we did
in on_each_cpu())
> include/linux/smp.h | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> Index: linux-2.6/include/linux/smp.h
> ===================================================================
> --- linux-2.6.orig/include/linux/smp.h
> +++ linux-2.6/include/linux/smp.h
> @@ -99,11 +99,9 @@ static inline void smp_send_reschedule(i
> #define num_booting_cpus() 1
> #define smp_prepare_boot_cpu() do {} while (0)
> static inline int smp_call_function_single(int cpuid, void (*func) (void *info),
> - void *info, int retry, int wait)
> + void *info, int retry, int wait)
> {
> - /* Disable interrupts here? */
> - func(info);
> - return 0;
> + return -EBUSY;
> }
>
> #endif /* !SMP */
Your patch seems appropriate given the present and proposed peculiar
semantics, but I think we need more work in there. x86_64 needs its return
value fixing asap, surely.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [patch] Let smp_call_function_single return -EBUSY.
2007-05-14 9:23 [patch] Let smp_call_function_single return -EBUSY Heiko Carstens
2007-05-14 19:11 ` Andrew Morton
@ 2007-06-07 16:04 ` Satyam Sharma
1 sibling, 0 replies; 5+ messages in thread
From: Satyam Sharma @ 2007-06-07 16:04 UTC (permalink / raw)
To: Heiko Carstens
Cc: Andrew Morton, linux-kernel, Ingo Molnar, randy.dunlap, paulus,
benh, David Miller, Alan Cox, Andi Kleen
Hi,
On 5/14/07, Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> All architectures that have an implementation of smp_call_function_single
> let it return -EBUSY if it is asked to execute func on the current cpu.
> Therefore the UP version must always return -EBUSY.
This logic is flawed, IMO. There is clearly a difference between trying to
send an IPI interrupt to oneself (which is what smp_call_function_single
on the current CPU could be thought to mean), and trying to send an IPI
interrupt to a processor that doesn't even exist (i.e. the case for both
smp_call_function() and smp_call_function_single() on !SMP). There are
other issues too ...
On 5/14/07, Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> This of course raises another question: it is not clear in which context
> the smp_call_function* functions are supposed to be called. Should it be
> with preemption disabled or is preemption enabled allowed as well?
> If calling with preemption enabled is allowed then the powerpc implementation
> is broken, since smp_processor_id() as well as num_online_cpus() may change
> while they are accessed.
and
On 5/15/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> erk, I see your point. If a caller is calling this with preemption enabled
> then the current thread might at any time migrate onto the target CPU,
> causing the smp_call_function_single() attempt to fail. So the effects of
> that call are basically a random crapshoot.
>
> Often but not always, any code which is hanging onto a variable called
> "cpu" while preemption enabled is buggy.
>
> So yes, I'd say that from a sanity-of-implementation POV and for general
> defensiveness, we should require that the called of
> smp_call_function_single() has disabled preemption.
Right, preemption must clearly be disabled. I have no experience with
non-x86 arch's, but i386 and x86_64 escape these issues due to the
call_lock spinlock or get_cpu()/put_cpu() pair, as discussed on the
other thread.
On 5/15/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> smp_call_function_single() is a mess.
There is a lot of inter-arch inconsistency, yes, but as far as the
semantics of smp_call_function() and smp_call_function_single() is
concerned, my understanding is simply what is documented in the
comments: we use smp_call_function when we wish to run given function
on _all_ *other* CPUs, and use smp_call_function_single when we wish
to run given function on _specified_ *other* CPU.
> - it's unclear to me why smp_call_function_single(cpu, ...) doesn't just
> call the darn function if cpu==smp_processor_id().
...
On 5/15/07, Andi Kleen <ak@suse.de> wrote:
> I always wondered that too.
This changes smp_call_function() semantics, does it not? Also, (stating
the obvious), we could simply /call/ the given function to run it on
current CPU. All the IPI infrastructure exists precisely for the case
when we want to run something on some _other_ CPU, doing otherwise
could horribly break things ...
A hypothetical example is when we wish to make another CPU halt (say
the passed function contains a "for (;;) ;") and then we wish to proceed
with some further work in-current-thread / on-current-CPU itself. If
we've jumped to target CPU by the time of execution of the call, and
then proceed to just execute the darn function as proposed ... and all
hell breaks loose.
I would strongly recommend not changing current semantics of both the
smp_call_function{_single} functions. [ Please do let me know if / where
I'm wrong, otherwise. ]
> - it's unclear to me why smp_call_function_single(cpu, ...) doesn't just
> call the darn function if CONFIG_SMP=n.
...
On 5/15/07, Andi Kleen <ak@suse.de> wrote:
> Yes.
Same reasoning as above?
> - it's unclear to me why smp_call_function_single(cpu, ...) isn't called
> smp_call_function_on(cpu, ...)
I'm fine with this proposed name change.
> - the x86_64 version doesn't return -EBUSY: it returns zero. Despite its
> claim "Retrurns 0 on success, else a negative status code.".
Needs to be fixed, clearly. I'll submit a patch for the same, shortly.
> - i386 _does_ return -EBUSY.
Which might be a reasonable thing to do if we've realized the target CPU
has somehow become the current CPU itself (after flagging a BUG / WARN
I would add). But like said earlier, this is not quite comparable to the
!SMP situation where the target CPU doesn't exist at all. I still feel both
of these should go BUG / WARNING on !SMP. The caller seems to have
gone wrong in logic somewhere, so we might as well flag it? [ For a
slightly more comparable case, we can refer to what powerpc does on
!cpu_online(target_cpu) and return -EINVAL; as suggested in my patch? ]
[ Clarifying / adding to smp_call_function{_single} semantics so that
various arch's can follow these without inconsistencies / mess. ]
On 5/15/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> - May be called with preemption enabled. The implementation must take
> care of that.
Agreed.
> - May not be called under local_irq_disable() for the usual reasons.
Agreed. We might also add what David Miller suggested earlier:
smp_call_function{_single} are illegal from _any_ asynchronous context,
such as hardirq, softirq, etc. [ s390 might need to be allowed to go
lax here, it contains a driver that needs to call these from softirq,
I think. ]
> - Will run the callback function under local_irq_disable() (this is
> unimportant at present, but if we decide to later change it to run the
> callback even on !SMP builds, we'll need to take care of that like we did
> in on_each_cpu())
on_each_cpu() is a different case, IMHO. The single processor in UP does
belong to the "each" CPUs of a system. Same can't be said about the
smp_call_function* for UP ...
Satyam
^ permalink raw reply [flat|nested] 5+ messages in thread