lkml.org 
[lkml]   [2013]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH RFC 1/2] qrwlock: A queue read/write lock implementation
    On 07/15/2013 10:39 AM, Steven Rostedt wrote:
    > On Fri, 2013-07-12 at 21:34 -0400, Waiman Long wrote:
    >
    >> Signed-off-by: Waiman Long<Waiman.Long@hp.com>
    >> ---
    >>
    >> +/*
    >> + * The queue read/write lock data structure
    >> + * The reader stealing flag, if sea,t will enable reader at the head of the
    > "sea,t"?

    Should be "if set,". Thank for spotting the typo. It will be fixed in
    the next version.

    >> +/**
    >> + * wait_in_queue - Add to queue and wait until it is at the head
    >> + * @lock: Pointer to queue read/writer lock structure
    >> + * @node: Node pointer to be added to the queue
    >> + */
    >> +static __always_inline void
    >> +wait_in_queue(struct qrwlock *lock, struct qrwnode *node)
    >> +{
    >> + struct qrwnode *prev;
    >> +
    >> + node->next = NULL;
    >> + node->wait = true;
    >> + barrier();
    >> + prev = xchg(&lock->waitq, node);
    > "barrier()" isn't needed, as xchg() is a full blown smp_mb(), it also
    > acts as a compiler barrier.

    Will remove barrier().

    >> +/*
    >> + * queue_read_trylock - try to acquire read lock of a queue read/write lock
    >> + * @lock : Pointer to queue read/writer lock structure
    >> + * Return: 1 if lock acquired, 0 if failed
    >> + */
    >> +int queue_read_trylock(struct qrwlock *lock)
    >> +{
    >> + struct qrwlock old, new;
    >> +
    >> + old.rw = ACCESS_ONCE(lock->rw);
    >> + if (unlikely(old.writer))
    >> + return 0;
    >> + new.rw = old.rw;
    >> + new.readers++;
    >> +
    >> + if (cmpxchg(&lock->rw, old.rw, new.rw) == old.rw)
    >> + return 1;
    >> + cpu_relax();
    > What's the cpu_relax() for? It's not in a loop.

    I put a cpu_relax() after each cacheline contention event. You are right
    that we don't need a cpu_relax() in the trylock() function here.

    >
    >> + return 0;
    >> +}
    >> +EXPORT_SYMBOL(queue_read_trylock);
    >> +
    >> +/**
    >> + * queue_write_lock - acquire write lock of a queue read/write lock
    >> + * @lock : Pointer to queue read/writer lock structure
    >> + */
    >> +void queue_write_lock(struct qrwlock *lock)
    >> +{
    >> + struct qrwnode node, *next;
    >> +
    >> + if (likely(!ACCESS_ONCE(lock->writer))) {
    >> + /*
    >> + * Atomically set the writer to 1, then wait until reader
    >> + * count goes to 0.
    >> + */
    >> + if (xchg(&lock->writer, 1) == 0) {
    >> + while (ACCESS_ONCE(lock->readers))
    >> + cpu_relax();
    >> + return;
    >> + }
    >> + cpu_relax();
    > Another cpu_relax() outside of a loop.

    I can remove that one too.

    >> +
    >> +/**
    >> + * queue_write_trylock - try to acquire write lock of a queue read/write lock
    >> + * @lock : Pointer to queue read/writer lock structure
    >> + * Return: 1 if lock acquired, 0 if failed
    >> + */
    >> +int queue_write_trylock(struct qrwlock *lock)
    >> +{
    >> + struct qrwlock old, new;
    >> +
    >> + old.rw = ACCESS_ONCE(lock->rw);
    >> + if (!old.rw) {
    >> + /*
    >> + * Atomically set the writer to 1 if readers = 0
    >> + */
    >> + new.rw = old.rw;
    >> + new.writer = 1;
    >> + if (cmpxchg(&lock->rw, old.rw, new.rw) == old.rw)
    >> + return 1;
    >> + cpu_relax();
    > Again the cpu_relax with no loop.

    Ditto.

    >> + }
    >> + return 0;
    >> +}
    >> +EXPORT_SYMBOL(queue_write_trylock);
    > I haven't seen anything bad about this with a quick review. But it
    > should have a more thorough review to check all corner cases.
    >
    > -- Steve
    >

    Thank for your time.

    Regards,
    Longman


    \
     
     \ /
      Last update: 2013-07-15 23:21    [W:5.244 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site