lkml.org 
[lkml]   [2007]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] synchronize_irq needs a barrier
    Date
    On Friday 19 October 2007 12:32, Herbert Xu wrote:

    > First of all let's agree on some basic assumptions:
    >
    > * A pair of spin lock/unlock subsumes the effect of a full mb.

    Not unless you mean a pair of spin lock/unlock as in
    2 spin lock/unlock pairs (4 operations).

    *X = 10;
    spin_lock(&lock);
    /* *Y speculatively loaded here */
    /* store to *X leaves CPU store queue here */
    spin_unlock(&lock);
    y = *Y;

    > * A spin lock in general only equates to (SS/SL/LL).
    > * A spin unlock in general only equates to (SS/LS).

    I don't use the sparc barriers, so they don't come naturally to
    me ;)

    I think both loads and stores can pass into the critical section
    by having the spin_lock pass earlier ops, or by having spin_unlock
    be passed by later ones.


    > In particular, a load after a spin unlock may pass before the
    > spin unlock.
    >
    > Here is the race (with tg3 as the only example that I know of).
    > The driver attempts to quiesce interrupts such that after the
    > call to synchronize_irq it is intended that no further IRQ
    > handler calls for that device will do any work besides acking
    > the IRQ.
    >
    > Here is how it does it:
    >
    > CPU0 CPU1
    > spin lock
    > load irq_sync
    > irq_sync = 1
    > smp_mb
    > synchronize_irq()
    > while (IRQ_INPROGRESS)
    > wait
    > return
    > set IRQ_INPROGRESS
    > spin unlock
    > tg3_msi
    > ack IRQ
    > if (irq_sync)
    > return
    > do work
    >
    > The problem here is that load of irq_sync in the handler has
    > passed above the setting of IRQ_INPROGRESS.
    >
    > Linus's patch fixes it because this becomes:
    >
    > CPU0 CPU1
    > spin lock
    > load irq_sync
    > irq_sync = 1
    > smp_mb
    > synchronize_irq
    > set IRQ_INPROGRESS
    > spin unlock
    > spin lock
    > spin unlock
    > tg3_msi
    > ack IRQ
    > if (irq_sync)
    > return
    > do work
    > while (IRQ_INPROGRESS)
    > wait
    > spin lock
    > clear IRQ_INPROGRESS
    > spin unlock
    > return
    >
    > Even though we still do the work on the right we will now notice
    > the INPROGRESS flag on the left and wait.
    >
    > It's hard to fix this in the drivers because they'd either have
    > to access the desc lock or add a full mb to the fast path on the
    > right.
    >
    > Once this goes in we can also remove the smp_mb from tg3.c. BTW,
    > a lot of drivers (including the fusion example Ben quoted) call
    > synchronize_irq before free_irq. This is unnecessary because
    > the latter already calls it anyway.
    >
    > Cheers,
    -
    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/

    \
     
     \ /
      Last update: 2007-10-19 04:55    [W:0.024 / U:63.204 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site