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

[Waiman Long <waiman.long@hp.com>]

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