On 11/26, Paul E. McKenney wrote:
>
> Looks pretty good, actually. A few quibbles below. I need to review
> again after sleeping on it.
Thanks! Please also look at spinlock-based implementation,
http://marc.theaimsgroup.com/?l=linux-kernel&m=116457701231964
I must admit, Alan was right: it really looks simpler and the perfomance
penalty should be very low. I personally hate this additional spinlock
per se as a "unneeded complication", but probably you will like it more.
> > +int xxx_read_lock(struct xxx_struct *sp)
> > +{
> > + for (;;) {
> > + int idx = sp->completed & 0x1;
>
> Might need a comment saying why no rcu_dereference() needed on the
> preceding line. The reason (as I understand it) is that we are
> only doing atomic operations on the element being indexed.
My understanding is the same. Actually, smp_read_barrier_depends() can't
help because 'atomic_inc' and '->completed++' in synchronize_xxx() could
be re-ordered anyway, so we should rely on correctness of atomic_t.
>
> > + if (likely(atomic_inc_not_zero(sp->ctr + idx)))
> > + return idx;
> > + }
> > +}
>
> The loop seems absolutely necessary if one wishes to avoid a
> synchronize_sched() in synchronize_xxx() below (and was one of the things
> I was missing earlier). However, isn't there a possibility that a pile
> of synchronize_xxx() calls might indefinitely delay an unlucky reader?
Note that synchronize_xxx() does nothing when there are no readers under
xxx_read_lock(), so
for (;;)
synchronize_xxx();
can't suspend xxx_read_lock(). atomic_inc_not_zero() fails when something like
the events below happen between 'idx = sp->completed' and 'atomic_inc_not_zero'
- another reader does xxx_read_lock(), increments ->ctr.
- synchronize_xxx() notices it, goes to __wait_event()
- both the reader and writer manage to do atomic_dec()
This is possible in theory, but indefinite delay... Look, we have the same
"problem" with spinlocks: in theory synchronize_xxx() calls might indefinitely
delay an unlucky reader because synchronize_xxx() always wins
spin_lock(&sp->lock);
> > +
> > +void xxx_read_unlock(struct xxx_struct *sp, int idx)
> > +{
> > + if (unlikely(atomic_dec_and_test(sp->ctr + idx)))
> > + wake_up(&sp->wq);
> > +}
> > +
> > +void synchronize_xxx(struct xxx_struct *sp)
> > +{
> > + int idx;
> > +
> > + mutex_lock(&sp->mutex);
> > +
> > + idx = sp->completed & 0x1;
> > + if (!atomic_add_unless(sp->ctr + idx, -1, 1))
> > + goto out;
> > +
> > + atomic_inc(sp->ctr + (idx ^ 0x1));
> > + sp->completed++;
> > +
> > + __wait_event(sp->wq, !atomic_read(sp->ctr + idx));
> > +out:
> > + mutex_unlock(&sp->mutex);
> > +}
>
> Test code!!! Very good!!! (This is added to rcutorture, right?)
Yes, the whole patch goes to kernel/rcutorture.c, it is only for
testing/review.
Note: I suspect that Documentation/ lies about atomic_add_unless(), see
http://marc.theaimsgroup.com/?l=linux-kernel&m=116448966030359
so synchronize_xxx() should be
void synchronize_xxx(struct xxx_struct *sp)
{
int idx;
smp_mb();
mutex_lock(&sp->mutex);
idx = sp->completed & 0x1;
if (atomic_read(sp->ctr + idx) == 1)
goto out;
atomic_inc(sp->ctr + (idx ^ 0x1));
sp->completed++;
atomic_dec(sp->ctr + idx);
wait_event(sp->wq, !atomic_read(sp->ctr + idx));
out:
mutex_unlock(&sp->mutex);
}
Yes, Alan was right, spinlock_t makes the code simpler.
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
