lkml.org 
[lkml]   [2010]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: blk-throttle: Correct the placement of smp_rmb()
    On 12/09, Paul E. McKenney wrote:
    >
    > On Thu, Dec 09, 2010 at 10:26:59AM +0100, Oleg Nesterov wrote:
    > >
    > > update_object(obj)
    > > {
    > > modify_obj(obj);
    > >
    > > wmb();
    > >
    > > obj->was_changed = true;
    > > }
    > >
    > > It can be called many times. Sooner or later, we will call
    > >
    > > process_object(obj)
    > > {
    > > if (!obj->was_changed)
    > > return;
    >
    > Ah, and if you have a huge number of CPUs executing update_object()
    > at just this point, we have the scenario you showed my in your initial
    > email.

    Yes.

    > update_object(obj)
    > {
    > modify_obj(obj);
    >
    > wmb();
    >
    > atomic_set(&obj->was_changed, true);
    > }
    >
    > process_object(obj)
    > {
    > if (!atomic_read(&obj->was_changed))
    > return;
    >
    > if (!atomic_xchg(&obj->was_changed, false))
    > return;
    >
    > /* mb(); implied by atomic_xchg(), so no longer needed. */
    >
    > do_process_object(obj);
    > }

    This is what we were going to do initially. Except, I think the
    plain bool/xchg can be used instead of atomic_t/atomic_xchg ?

    But then we decided to discuss the alternatives. Partly because
    this looked like the interesting question, but mostly to keep
    you busy ;)

    > One caution: The wmb() in update_object() means that modify_object()
    > might read some variable and get a -newer- value for that variable than
    > would a subsequent read from that same variable in do_process_object().
    > If this would cause a problem, the wmb() should instead be an mb().

    Yes. And in this case I even _seem_ to understand why we need
    s/wmb/mb/ change.

    But the original code (I mean, the code we are trying to fix/change)
    doesn't have the load-load dependency, so I think wmb() is enough.

    > The reason that I say that this should not take much additional
    > overhead is that all of the writes were taking cache-miss latencies,
    > and you had lots of memory barriers that make it difficult for the
    > CPUs' store buffers to hide that latency. The only added overhead
    > is from the atomic instruction, but given that you already had a
    > cache miss from the original write and a memory barrier, I would not
    > expect it to be noticeable.
    >
    > But enough time on my soapbox... Would this do what you need it to?
    > If so, hopefully it really does what I think it does. ;-)

    OK, thanks Paul.

    So I guess it would be safer to return to initial plan and use xchg().

    > (See http://paulmck.livejournal.com/20312.html for explanation.)

    Oh. Very interesting. Transitive memory barriers.

    You know, I always wanted to understand this aspect. May be you can
    look at

    http://marc.info/?l=linux-kernel&m=118944341320665

    starting from "To simplify the example above". This pseudo-code tries
    to resemble the real-life code we discussed, that is why it uses the
    pointers (dereference lack read_barrier_depends(), please ignore).

    And no, I can't understand why foo_1() needs the full barrier :/
    Or may be I can? Suppose that CPU 0 and CPU 1 share the store-buffer
    (no, no, I do not pretend I _really_ understand what this actually
    means;). In this case, perhaps we can forget abou CPU 0 and rewrite
    this code as

    void foo_1(void)
    {
    X = 1; /* it was actually written by CPU 0 */

    r1 = x;
    smp_rmb(); /* The only change. */
    r2 = y;
    }

    void foo_2(void)
    {
    y = 1;
    smp_mb();
    r3 = x;
    }

    In this case smp_rmb() obviously can't help. Does it make any sense?


    But, when I look at the link I sent you again, I feel I am totatlly
    confused. Nothing new, I always knew that memory barriers were specially
    designed to drive me crazy.

    Oleg.



    \
     
     \ /
      Last update: 2010-12-10 17:55    [W:2.416 / U:0.212 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site