Re: [PATCH RFC 1/2] qrwlock: A queue read/write lock implementation

[Waiman Long]

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@xxxxxx>
> > ---
> >
> > +/*
> > + * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/