Messages in this thread | | | Date | Tue, 10 Jul 2007 14:03:02 +0300 | From | Avi Kivity <> | Subject | Re: [PATCH 17/20] SMP: Implement on_cpu() |
| |
Satyam Sharma wrote: > On 7/10/07, Avi Kivity <avi@qumranet.com> wrote: >> Satyam Sharma wrote: >> > >> > >> > On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote: >> >> [...] >> >> on_each_cpu() was imho always a mistake. It would have been better >> >> to just fix smp_call_function() directly >> > >> > I'm not sure what you mean by "fix" here, but if you're proposing >> > that we change smp_call_function() semantics to _include_ the >> > current CPU (and just run the given function locally also along >> > with the others -- and hence get rid of on_each_cpu) then I'm sorry >> > but I'll have to *violently* disagree with that. Please remember that >> > the current CPU _must_ be treated specially in a whole *lot* of >> > usage scenarios ... >> >> I imagine that by "fix" Andi means also updating all callers. Otherwise >> he would just have said "break". > > But that's the point. How do you plan / intend to update > smp_send_stop()? >
Well, I don't plan to do anything to smp_call_function(). I imagine you can add a flag, or compare smp_processor_id() to the cpu that's not stopping, or use smp_call_function_mask().
> More importantly, what's wrong with it in the first place (to "fix")?
If most use cases want to run a function on all cpus, they shouldn't need to open code it.
> >> > On 7/9/07, Andi Kleen <andi@firstfloor.org> wrote: >> >> > I think it would be better to fix smp_call_function_single to just >> >> > handle this case transparently. There aren't that many callers yet >> >> > because it is >> >> > fairly new. >> > >> > Take the same example here -- let's say we want to send a >> > "for (;;) ;" kind of function to a specified CPU. Now let's say >> > by the time we've called smp_call_function_single() on that >> > target CPU, we're preempted out and then get rescheduled >> > on the target CPU itself. There, we begin executing the >> > smp_call_function_single() (as modified by Avi here with your >> > proposed changed semantics) and notice that we've landed >> > on the target CPU itself, execute the suicidal function >> > _locally_ *in current thread* itself, and ... well, I hope you >> > get the picture. >> >> So you disable preemption before calling smp_call_function_single(). > > Which is what on_cpu() and which is why I like that. > > And which is *not* what Andi's proposal (or your later patch > implementing that proposal) does, and which is why I *don't* > like that.
It does disable preemption. Look more carefully.
> >> > So my opinion is to go with the get_cpu() / put_cpu() wrapper >> > Avi is proposing here and keep smp_call_function{_single} >> > semantics unchanged. [ Also please remember that for >> > *correctness*, preemption needs to be disabled by the >> > _caller_ of smp_call_function{_single} functions, doing so >> > inside them is insufficient. ] >> >> That's not correct. kvm has two places where you can call the new >> smp_call_function_single() (or on_cpu()) without disabling preemption. > > on_cpu() _is_ the wrapper that does the necessary get_cpu() > (i.e. preemption-disabling wrap over smp_call_function_single). > > Obviously a caller of on_cpu() does not need to disable preemption.
Neither does the caller of the new smp_call_function_single(). Look at the code.
> >> There are also a couple of existing places that don't need to disable >> preemption with the new semantics (see mtrr_save_state(), do_cpuid(), >> _rdmsr_on_cpu(), all in arch/i386 for examples). In fact I think more >> places can take advantage of the new semantics than not. > > I presume you mean these are places where we just specify the CPU > to execute the function on, and don't really care if by that time we've > gone over to that CPU itself -- so the new semantics are fine too? > So these are places where you can use on_cpu(). But why change > existing semantics of smp_call_function_single is what I can't quite > understand, when there are perfectly legitimate usage cases where we > _don't_ want the function to get executed locally.
Most (all?) do. And there's not harm done if they don't. Look at the code.
-- error compiling committee.c: too many arguments to function
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |