Re: [PATCH 17/20] SMP: Implement on_cpu()

[Satyam Sharma]

On 7/10/07, Avi Kivity <avi@xxxxxxxxxxxx> wrote:
> Satyam Sharma wrote:
> >
> >
> > On 7/9/07, Andi Kleen <andi@xxxxxxxxxxxxxx> 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()?

More importantly, what's wrong with it in the first place (to "fix")?

> > On 7/9/07, Andi Kleen <andi@xxxxxxxxxxxxxx> 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.

> > 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.

[ This was w.r.t. existing smp_call_function{_single} semantics. ]

> 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.

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/