lkml.org 
[lkml]   [2015]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: qemu:arm test failure due to commit 8053871d0f7f (smp: Fix smp_call_function_single_async() locking)
On Mon, Apr 20, 2015 at 07:39:55AM +0200, Ingo Molnar wrote:
>
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > On Sun, Apr 19, 2015 at 11:01 AM, Ingo Molnar <mingo@kernel.org> wrote:
> > >
> > > That's all fine and good, but why is an IPI sent to a non-existent
> > > CPU? It's not like we don't know which CPU is up and down.
> >
> > I agree that the qemu-arm behavior smells like a bug, plain and
> > simple. Nobody sane should send random IPI's to CPU's that they
> > don't even know are up or not.
>
> Yeah, and a warning would have caught that bug a bit earlier, at the
> cost of restricting the API:
>
> > That said, I could imagine that we would have some valid case where
> > we just do a cross-cpu call to (for example) do lock wakeup, and the
> > code would use some optimistic algorithm that prods the CPU after
> > the lock has been released, and there could be some random race
> > where the lock data structure has already been released (ie imagine
> > the kind of optimistic unlocked racy access to "sem->owner" that we
> > discussed as part of the rwsem_spin_on_owner() thread recently).
> >
> > So I _could_ imagine that somebody would want to do optimistic "prod
> > other cpu" calls that in all normal cases are for existing cpus, but
> > could be racy in theory.
>
> Yes, and I don't disagree with such optimizations in principle (it
> allows less references to be taken in the fast path), but is it really
> safe?
>
> If a CPU is going down and we potentially race against that, and send
> off an IPI, the IPI might be 'in flight' for an indeterminate amount
> of time, especially on wildly non-deterministic hardware like virtual
> platforms.
>
> So if the CPU goes down during that time, the typical way a CPU goes
> down is that it ignores all IPIs that arrive after that. End result:
> the sender of the IPI may hang indefinitely (for the synchronous API),
> waiting for the CSD lock to be released...
>
> For the non-wait API we could also hang, but in an even more
> interesting way: we won't hang trying to send to the downed CPU, we'd
> hang on the _next_ cross-call call, possibly sending to another CPU,
> because the CSD_FLAG_LOCK of the sender CPU is never released by the
> offlined CPU which was supposed to unlock it.
>
> Also note the existing warning we already have in
> flush_smp_call_function_queue():
>
> /* There shouldn't be any pending callbacks on an offline CPU. */
> if (unlikely(warn_cpu_offline && !cpu_online(smp_processor_id()) &&
> !warned && !llist_empty(head))) {
> warned = true;
> WARN(1, "IPI on offline CPU %d\n", smp_processor_id());
>
> Couldn't this trigger in the opportunistic, imprecise IPI case, if
> IRQs are ever enabled when or after the CPU is marked offline?
>
> My suggested warning would simply catch this kind of unsafe looking
> race a bit sooner, instead of silently ignoring the cross-call
> attempt.
>
> Now your suggestion could be made to work by:
>
> - polling for CPU status in the CSD-lock code as well. (We don't do
> this currently.)
>
> - making sure that if a late IPI arrives to an already-down CPU it
> does not attempt to execute CSD functions. (I.e. silence the
> above, already existing warning.)
>
> - auditing the CPU-down path to make sure it does not get surprised
> by late external IPIs. (I think this should already be so, given
> the existing warning.)
>
> - IPIs can be pending indefinitely, so make sure a pending IPI won't
> confuse the machinery after a CPU has been onlined again.
>
> - pending IPIs may consume hardware resources when not received
> properly. For example I think x86 will keep trying to resend it.
> Not sure there's any timeout mechanism on the hardware level - the
> sending APIC might even get stuck? Audit all hardware for this
> kind of possibility.
>
> So unless I'm missing something, to me it looks like that the current
> code is only safe to be used on offline CPUs if the 'offline' CPU is
> never ever brought online in the first place.
>
> > It doesn't sound like the qemu-arm case is that kind of situation,
> > though. That one just sounds like a stupid "let's send an ipi to a
> > cpu whether it exists or not".
> >
> > But Icommitted it without any new warning, because I could in theory
> > see it as being a valid use.
>
> So my point is that we already have a 'late' warning, and I think it
> actively hid the qemu-arm bug, because the 'offline' CPU was so
> offline (due to being non-existent) that it never had a chance to
> print a warning.
>
> With an 'early' warning we could flush out such bugs (or code
> uncleanlinesses: clearly the ARM code was at least functional before)
> a bit sooner.
>
> But I don't have strong feelings about it, SMP cross-call users are a
> lot less frequent than say locking facility users, so the strength of
> debugging isn't nearly as important and it's fine to me either way!

In the long term, support of "send an IPI to a CPU that might or might
not be leaving" will probably need an API that returns a success or
failure indication. This will probably simplify things a bit, because
currently the caller needs to participate in the IPI's synchronization.
With a conditional primitive, callers could instead simply rely on the
return value.

Thanx, Paul



\
 
 \ /
  Last update: 2015-04-20 14:21    [W:0.063 / U:0.724 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site