Messages in this thread |  | | Subject | Re: [PATCH 3/7] barrier: a scalable synchonisation barrier | From | Peter Zijlstra <> | Date | Fri, 02 Feb 2007 13:01:29 +0100 |
| |
On Fri, 2007-02-02 at 14:56 +0300, Oleg Nesterov wrote: > On 02/01, Paul E. McKenney wrote: > > > > > > > > void synchronize_qrcu(struct qrcu_struct *qp) > > > > { > > > > int idx; > > > > > > > > smp_mb(); > > > > > > > > if (atomic_read(qp->ctr[0]) + atomic_read(qp->ctr[1]) <= 1) { > > > > smp_rmb(); > > > > if (atomic_read(qp->ctr[0]) + > > > > atomic_read(qp->ctr[1]) <= 1) > > > > goto out; > > > > } > > > > > > > > mutex_lock(&qp->mutex); > > > > idx = qp->completed & 0x1; > > > > atomic_inc(qp->ctr + (idx ^ 0x1)); > > > > /* Reduce the likelihood that qrcu_read_lock() will loop */ > > > > smp_mb__after_atomic_inc(); > > > > qp->completed++; > > > > > > > > atomic_dec(qp->ctr + idx); > > > > __wait_event(qp->wq, !atomic_read(qp->ctr + idx)); > > > > mutex_unlock(&qp->mutex); > > > > out: > > > > smp_mb(); > > > > } > > > > > > > > For the first "if" to give a false positive, a concurrent switch had > > > > to have happened. For example, qp->ctr[0] was zero and qp->ctr[1] > > > > was two at the time of the first atomic_read(), but then qp->completed > > > > switched so that both qp->ctr[0] and qp->ctr[1] were one at the time > > > > of the second atomic_read. The only way the second "if" can give us a > > > > false positive is if there was another change to qp->completed in the > > > > meantime -- but that means that all of the pre-existing qrcu_read_lock() > > > > holders must have gotten done, otherwise the second switch could not > > > > have happened. Yes, you do incur three memory barriers on the fast > > > > path, but the best you could hope for with your approach was two of them > > > > (unless I am confused about how you were using barrier_sync()). > > Yes. Without synchronize_qrcu() in between, one of the counters should be == 0, > another >= 1. == 1 means we have no active readers. So the false positive really > means a concurrent switch. And we can check twice - excellent idea! > > > > While doing qrcu, somehow I convinced myself we can't optimize out taking > > > qp->mutex. Now I think I was wrong. Good! > > > > Me, I didn't want to worry about it unless someone needed it. Which > > it now appears they do. ;-) > > No. I do remember I tried hard to optimize out taking qp->mutex, but failed. > So I decided it is not possible. And now you show that I just don't have enough > brains! (of course, I hate you :) > > > > Q: you deleted "if (atomic_read(qp->ctr + idx) == 1)" fastpath under ->mutex, > > > was this needed for this optimization to work? I am asking because I can't > > > understand how it can make any difference. > > > > Before, we held the lock, so we could just check the single current > > element. Now we don't hold the lock, so we need to check both elements. > > So I replaced the "if (atomic_read(qp->ctr + idx) == 1)" with the > > nested "if" statements that test both elements. > > Ah, my question was different. The current version of qrcu does > > mutex_lock(&qp->mutex); > > idx = qp->completed & 0x1; > if (atomic_read(qp->ctr + idx) == 1) // fast path > return; > > ... > > and it seems to me that we can retain this fastpath even with your optimization, > no? Surely, it is not so important, but it is nearly free. > > Paul, could you make a patch? (I'll do rcutorture test tomorrow, but I only have > P-4 ht). > > Peter, do you think you can use qrcu?
Yes, this looks very much like what I need. Awesome work!
- 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/
|  |