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

From: Avi Kivity
Date: Tue Jul 10 2007 - 07:03:10 EST


Satyam Sharma wrote:
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()?


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

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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/