lkml.org 
[lkml]   [2008]   [Sep]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] [2/2] Don't complain about disabled irqs when the system has paniced
    Date
    On Wednesday 03 September 2008 16:59, Andi Kleen wrote:
    > On Wed, Sep 03, 2008 at 04:02:37PM +1000, Nick Piggin wrote:
    > > > > Neither remote cpu will handle the IPI due to irqs being disabled, so
    > > > > we'll wait at-infinitum for completion.
    > > >
    > > > First smp_send_stop does not wait (or at least not pass the
    > > > wait flag, it will still wait for the first ack like everyone else)
    > >
    > > It won't wait, but it may have to wait for an ack as you say (but now
    > > this is a very rare case when kmalloc fails rather than always having
    > > to wait for so long).
    >
    > kmalloc is actually not safe in panic. panic could happen inside
    > kmalloc(). Hmm I missed it does that unconditionally.
    > If yes the code is more broken than I thought.

    Hmm... that does pull in significantly more code yes...

    It should be fixed. I guess a quickfix is to add a new call in
    kernel/smp.c for use by panic code.

    smp_send_stop is not a very precise heuristic when used at panic time,
    though, so I prefer not to slow down anything just for that case.


    > > > I don't claim to understand the new kernel/smp.c code (it seems to me
    > > > quite overdesigned and complicated and I admit I got lost in it
    > > > somewhere), but I think your scenario would rely on a global lock and
    > > > presumably there is none in the new code?
    > >
    > > Overdesigned? Well the old code was horribly slow and synchronized, and
    > > made it useless for doing anything except things like smp_send_stop so
    > > I would say it was under designed ;)
    >
    > It just seems quite complicated. Do you really need four layers
    > of function calls just to ask the low level code to trigger
    > an IPI?

    Oh. I thought the actual function call structure itself should be the
    nice cleanup part that everyone would like...

    - call_function->call_function_mask pre existing code
    New:
    - arch IPI function allows most logic to be implemented generically
    - new API that allows deadlock-free operation with irqs disabled in
    some cases.

    Queueing for smp_call_function_mask I understand if it is less liked ;)


    > And are there really benchmarks that show the
    > queueing does actually improve something significantly?

    For the call_function_mask case, I did see a benefit of queueing when
    testing vmap scalability on a bigish system, yes. I had since done
    vunmap batching in the vmap layer so the IPI costs don't really show
    up at all there anymore. But the code was already implemented, and I
    still don't want a smp_call_function from CPU0 to CPUs 1 and 2 to hold
    up an smp_call_function from CPU4 to CPUs 5 and 6 for example.

    For call_function_single, queueing could really help with high interrupt
    loads like the block completion migration that Jens has been looking at.
    In that case, queueing will act like interrupt mitigation when the target
    CPU becomes saturated so it will help keep performance from degrading,
    and it also allows multiple source CPUs to target a single destination
    without losing too much scalability.

    Also there was some straight-line benefit of queueing due to the source
    CPU not having to wait for an ack from the destination before continuing.
    Basically that would previously put a hard upper limit of remote function
    call to the interrupt latency.

    Benchmarks for "real" things unfortunately not easy to come by yet,
    simply because it was so crap before that it was unusable for anything
    remotely performance oriented. After the rewrite, I hope to see some
    more interesting uses popping up slowly.


    > Ok I can see the
    > point of having distributed locks (did that on my own for the x86-64
    > TLB flusher), but that really doesn't need that much code !
    > And the queueing has bad side effects like breaking panic
    > and adding hard to test fallback paths.
    >
    > Perhaps I'm old fashioned, but somehow my feeling is noone tries
    > to keep code simple anymore :/

    It was certianly tried. It's not a simple problem to do this with any
    reasonable scalability, however.


    > > > I think for 2.6.27 at least this is the best fix. At least keeping
    > > > panic that broken is no option I would say.
    > >
    > > It is reasonable I think, but I don't like testing symbolic constants
    > > with inequalities like in your patch 2/2. Can you just make it
    > > system_state != SYSTEM_PANIC ?
    >
    > Well I like it.

    How is it better than system_state != SYSTEM_PANIC? It breaks if
    SYSTEM_PANIC gets something in front of it, and even when it is
    correct it makes you double check the definition of SYSTEM_PANIC
    to see what it does (is there any state above SYSTEM_PANIC?)


    > > Also... is it really a regression? The old code definitely had deadlock
    > > warnings too, but maybe they were made more complete in the new code?
    >
    > Yes 2.6.26 did panic without these endless warnings.

    OK that should be fixed I agree.


    \
     
     \ /
      Last update: 2008-09-03 11:41    [W:0.031 / U:0.372 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site