lkml.org 
[lkml]   [2007]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/7] barrier: a scalable synchonisation barrier
On 01/31, Paul E. McKenney wrote:
>
> On Sun, Jan 28, 2007 at 04:24:35PM +0100, Ingo Molnar wrote:
> >
> > * Christoph Hellwig <hch@infradead.org> wrote:
> >
> > > On Sun, Jan 28, 2007 at 12:51:21PM +0100, Peter Zijlstra wrote:
> > > > This barrier thing is constructed so that it will not write in the
> > > > sync() condition (the hot path) when there are no active lock
> > > > sections; thus avoiding cacheline bouncing. -- I'm just not sure how
> > > > this will work out in relation to PI. We might track those in the
> > > > barrier scope and boost those by the max prio of the blockers.
> > >
> > > Is this really needed? We seem to grow new funky locking algorithms
> > > exponentially, while people already have a hard time understanding the
> > > existing ones.
> >
> > yes, it's needed.
>
> Would it be possible to come up with something common between this primitive
> and the one that Oleg Nesterov put together for Jens Axboe?
>
> http://lkml.org/lkml/2006/11/29/330
>
> Oleg's approach acquires a lock on the update side, which Peter would
> not want in the uncontended case -- but perhaps there is some way to
> make Oleg's approach be able to safely test both counters so as to
> avoid acquiring the lock if there are no readers.
>
> Oleg, any chance of this working? I believe it does, but have not
> thought it through fully.

I think no. From the quick reading, barrier_sync() and qrcu/srcu are
quite different. Consider:

barrier_lock()

barrier_sync();

barrier_unlock();
... wake up ...
barrier_lock();

schedule again

The last "schedule again" would be a BUG for qrcu/srcu, but probably
it is ok for barrier_sync(). It looks like barrier_sync() is more a
rw semaphore biased to readers.

A couple of minor off-topic notes,

+static inline void barrier_unlock(struct barrier *b)
+{
+ smp_wmb();
+ if (atomic_dec_and_test(&b->count))
+ __wake_up(&b->wait, TASK_INTERRUPTIBLE|TASK_UNINTERRUPTIBLE, 0, b);

This is wake_up_all(&b->wait), yes? I don't undestans why key == b, it could be NULL.

+static inline void barrier_sync(struct barrier *b)
+{
+ might_sleep();
+
+ if (unlikely(atomic_read(&b->count))) {
+ DEFINE_WAIT(wait);
+ prepare_to_wait(&b->wait, &wait, TASK_UNINTERRUPTIBLE);
+ while (atomic_read(&b->count))
+ schedule();
+ finish_wait(&b->wait, &wait);
+ }
+}

This should be open-coded wait_event(), but wrong! With the scenario above this
can hang forever! because the first wake_up removes the task from the &b->wait.

Oleg.

-
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-01-31 22:17    [W:0.116 / U:0.344 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site