[PATCH] locking/rwlocks: clean up of qrwlock

[PATCH] locking/rwlocks: clean up of qrwlock

[Baoquan He]

In queue_read_lock_slowpath, when writer count becomes 0, we need
increment the read count and get the lock. Then need call
rspin_until_writer_unlock to check again if an incoming writer
steals the lock in the gap. But in rspin_until_writer_unlock
it only checks the writer count, namely low 8 bit of lock->cnts,
no need to subtract the reader count unit specifically. So remove
that subtraction to make it clearer, rspin_until_writer_unlock
just takes the actual lock->cnts as the 2nd argument.

And also change the code comment in queue_write_lock_slowpath to
make it more exact and explicit.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 kernel/locking/qrwlock.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
index f956ede..ae66c10 100644
--- a/kernel/locking/qrwlock.c
+++ b/kernel/locking/qrwlock.c
@@ -76,7 +76,7 @@ void queue_read_lock_slowpath(struct qrwlock *lock)
 	while (atomic_read(&lock->cnts) & _QW_WMASK)
 		cpu_relax_lowlatency();
 
-	cnts = atomic_add_return(_QR_BIAS, &lock->cnts) - _QR_BIAS;
+	cnts = atomic_add_return(_QR_BIAS, &lock->cnts);
 	rspin_until_writer_unlock(lock, cnts);
 
 	/*
@@ -97,14 +97,14 @@ void queue_write_lock_slowpath(struct qrwlock *lock)
 	/* Put the writer into the wait queue */
 	arch_spin_lock(&lock->lock);
 
-	/* Try to acquire the lock directly if no reader is present */
+	/* Try to acquire the lock directly if no reader and writer is present */
 	if (!atomic_read(&lock->cnts) &&
 	    (atomic_cmpxchg(&lock->cnts, 0, _QW_LOCKED) == 0))
 		goto unlock;
 
 	/*
-	 * Set the waiting flag to notify readers that a writer is pending,
-	 * or wait for a previous writer to go away.
+	 * Wait for a previous writer to go away, then set the waiting flag to
+	 * notify readers that a writer is pending.
 	 */
 	for (;;) {
 		cnts = atomic_read(&lock->cnts);
-- 
1.8.5.3

Re: [PATCH] locking/rwlocks: clean up of qrwlock

[Peter Zijlstra]

On Tue, Dec 16, 2014 at 02:00:40PM +0800, Baoquan He wrote:
> In queue_read_lock_slowpath, when writer count becomes 0, we need
> increment the read count and get the lock. Then need call
> rspin_until_writer_unlock to check again if an incoming writer
> steals the lock in the gap. But in rspin_until_writer_unlock
> it only checks the writer count, namely low 8 bit of lock->cnts,
> no need to subtract the reader count unit specifically. So remove
> that subtraction to make it clearer, rspin_until_writer_unlock
> just takes the actual lock->cnts as the 2nd argument.
> 
> And also change the code comment in queue_write_lock_slowpath to
> make it more exact and explicit.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  kernel/locking/qrwlock.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
> index f956ede..ae66c10 100644
> --- a/kernel/locking/qrwlock.c
> +++ b/kernel/locking/qrwlock.c
> @@ -76,7 +76,7 @@ void queue_read_lock_slowpath(struct qrwlock *lock)
>  	while (atomic_read(&lock->cnts) & _QW_WMASK)
>  		cpu_relax_lowlatency();
>  
> -	cnts = atomic_add_return(_QR_BIAS, &lock->cnts) - _QR_BIAS;
> +	cnts = atomic_add_return(_QR_BIAS, &lock->cnts);
>  	rspin_until_writer_unlock(lock, cnts);

Did you actually look at the ASM generated? I suspect your change makes
it bigger.

Re: [PATCH] locking/rwlocks: clean up of qrwlock

[Baoquan He]

On 12/16/14 at 10:01am, Peter Zijlstra wrote:
> On Tue, Dec 16, 2014 at 02:00:40PM +0800, Baoquan He wrote:
> > In queue_read_lock_slowpath, when writer count becomes 0, we need
> > increment the read count and get the lock. Then need call
> > rspin_until_writer_unlock to check again if an incoming writer
> > steals the lock in the gap. But in rspin_until_writer_unlock
> > it only checks the writer count, namely low 8 bit of lock->cnts,
> > no need to subtract the reader count unit specifically. So remove
> > that subtraction to make it clearer, rspin_until_writer_unlock
> > just takes the actual lock->cnts as the 2nd argument.
> > 
> > And also change the code comment in queue_write_lock_slowpath to
> > make it more exact and explicit.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  kernel/locking/qrwlock.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
> > index f956ede..ae66c10 100644
> > --- a/kernel/locking/qrwlock.c
> > +++ b/kernel/locking/qrwlock.c
> > @@ -76,7 +76,7 @@ void queue_read_lock_slowpath(struct qrwlock *lock)
> >  	while (atomic_read(&lock->cnts) & _QW_WMASK)
> >  		cpu_relax_lowlatency();
> >  
> > -	cnts = atomic_add_return(_QR_BIAS, &lock->cnts) - _QR_BIAS;
> > +	cnts = atomic_add_return(_QR_BIAS, &lock->cnts);
> >  	rspin_until_writer_unlock(lock, cnts);
> 
> Did you actually look at the ASM generated? I suspect your change makes
> it bigger.


It does make it bigger. But it doesn't matter. Because in
rspin_until_writer_unlock it only compqre (cnts & _QW_WMASK) 
with _QW_LOCKED. So using incremented reader count doesn't impact
the result. Anyway it will get the actual lock->cnts in
rspin_until_writer_unlock in next loop. I can't see why we need
subtract that reader count increment specifically.

When I read this code, thought there's some special usage. Finally I
realized it doesn't have special usage, and doesn't have to do that. 

Re: [PATCH] locking/rwlocks: clean up of qrwlock

[Waiman Long]

On 12/16/2014 10:36 AM, Baoquan He wrote:
> On 12/16/14 at 10:01am, Peter Zijlstra wrote:
>> On Tue, Dec 16, 2014 at 02:00:40PM +0800, Baoquan He wrote:
>>> In queue_read_lock_slowpath, when writer count becomes 0, we need
>>> increment the read count and get the lock. Then need call
>>> rspin_until_writer_unlock to check again if an incoming writer
>>> steals the lock in the gap. But in rspin_until_writer_unlock
>>> it only checks the writer count, namely low 8 bit of lock->cnts,
>>> no need to subtract the reader count unit specifically. So remove
>>> that subtraction to make it clearer, rspin_until_writer_unlock
>>> just takes the actual lock->cnts as the 2nd argument.
>>>
>>> And also change the code comment in queue_write_lock_slowpath to
>>> make it more exact and explicit.
>>>
>>> Signed-off-by: Baoquan He<bhe@redhat.com>
>>> ---
>>>   kernel/locking/qrwlock.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
>>> index f956ede..ae66c10 100644
>>> --- a/kernel/locking/qrwlock.c
>>> +++ b/kernel/locking/qrwlock.c
>>> @@ -76,7 +76,7 @@ void queue_read_lock_slowpath(struct qrwlock *lock)
>>>   	while (atomic_read(&lock->cnts)&  _QW_WMASK)
>>>   		cpu_relax_lowlatency();
>>>
>>> -	cnts = atomic_add_return(_QR_BIAS,&lock->cnts) - _QR_BIAS;
>>> +	cnts = atomic_add_return(_QR_BIAS,&lock->cnts);
>>>   	rspin_until_writer_unlock(lock, cnts);
>> Did you actually look at the ASM generated? I suspect your change makes
>> it bigger.
>
> It does make it bigger. But it doesn't matter. Because in
> rspin_until_writer_unlock it only compqre (cnts&  _QW_WMASK)
> with _QW_LOCKED. So using incremented reader count doesn't impact
> the result. Anyway it will get the actual lock->cnts in
> rspin_until_writer_unlock in next loop. I can't see why we need
> subtract that reader count increment specifically.
>
> When I read this code, thought there's some special usage. Finally I
> realized it doesn't have special usage, and doesn't have to do that.

The "- _QR_BIAS" expression was added to simulate xadd() which is 
present in x86, but not in some other architectures. There is no 
equivalent functionality in the set of atomic helper functions. Anyway, 
I have no objection to the change as it is in the slowpath.

Acked-by: Waiman Long <Waiman.Long@hp.com>

Re: [PATCH] locking/rwlocks: clean up of qrwlock

[Baoquan He]

Hi all,

Thanks for reviewing. 

Do you have any other concerns about this small change?

Thanks
Baoquan


On 12/17/14 at 04:16pm, Waiman Long wrote:
> On 12/16/2014 10:36 AM, Baoquan He wrote:
> >On 12/16/14 at 10:01am, Peter Zijlstra wrote:
> >>On Tue, Dec 16, 2014 at 02:00:40PM +0800, Baoquan He wrote:
> >>>In queue_read_lock_slowpath, when writer count becomes 0, we need
> >>>increment the read count and get the lock. Then need call
> >>>rspin_until_writer_unlock to check again if an incoming writer
> >>>steals the lock in the gap. But in rspin_until_writer_unlock
> >>>it only checks the writer count, namely low 8 bit of lock->cnts,
> >>>no need to subtract the reader count unit specifically. So remove
> >>>that subtraction to make it clearer, rspin_until_writer_unlock
> >>>just takes the actual lock->cnts as the 2nd argument.
> >>>
> >>>And also change the code comment in queue_write_lock_slowpath to
> >>>make it more exact and explicit.
> >>>
> >>>Signed-off-by: Baoquan He<bhe@redhat.com>
> >>>---
> >>>  kernel/locking/qrwlock.c | 8 ++++----
> >>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>>diff --git a/kernel/locking/qrwlock.c b/kernel/locking/qrwlock.c
> >>>index f956ede..ae66c10 100644
> >>>--- a/kernel/locking/qrwlock.c
> >>>+++ b/kernel/locking/qrwlock.c
> >>>@@ -76,7 +76,7 @@ void queue_read_lock_slowpath(struct qrwlock *lock)
> >>>  	while (atomic_read(&lock->cnts)&  _QW_WMASK)
> >>>  		cpu_relax_lowlatency();
> >>>
> >>>-	cnts = atomic_add_return(_QR_BIAS,&lock->cnts) - _QR_BIAS;
> >>>+	cnts = atomic_add_return(_QR_BIAS,&lock->cnts);
> >>>  	rspin_until_writer_unlock(lock, cnts);
> >>Did you actually look at the ASM generated? I suspect your change makes
> >>it bigger.
> >
> >It does make it bigger. But it doesn't matter. Because in
> >rspin_until_writer_unlock it only compqre (cnts&  _QW_WMASK)
> >with _QW_LOCKED. So using incremented reader count doesn't impact
> >the result. Anyway it will get the actual lock->cnts in
> >rspin_until_writer_unlock in next loop. I can't see why we need
> >subtract that reader count increment specifically.
> >
> >When I read this code, thought there's some special usage. Finally I
> >realized it doesn't have special usage, and doesn't have to do that.
> 
> The "- _QR_BIAS" expression was added to simulate xadd() which is
> present in x86, but not in some other architectures. There is no
> equivalent functionality in the set of atomic helper functions.
> Anyway, I have no objection to the change as it is in the slowpath.
> 
> Acked-by: Waiman Long <Waiman.Long@hp.com>
> 
> --
> 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/

Re: [PATCH] locking/rwlocks: clean up of qrwlock

[Peter Zijlstra]

On Tue, Jan 06, 2015 at 05:25:42PM +0800, Baoquan He wrote:
> Hi all,
> 
> Thanks for reviewing. 
> 
> Do you have any other concerns about this small change?

I just don't see any point in making the code worse for the 1 arch that
actually uses it. If there were other archs using it and for them the
code generation would be negatively impacted (they'd actually generate
more core because of it) it might be worth looking at.

Re: [PATCH] locking/rwlocks: clean up of qrwlock

[Baoquan He]

On 01/06/15 at 11:13am, Peter Zijlstra wrote:
> On Tue, Jan 06, 2015 at 05:25:42PM +0800, Baoquan He wrote:
> > Hi all,
> > 
> > Thanks for reviewing. 
> > 
> > Do you have any other concerns about this small change?
> 
> I just don't see any point in making the code worse for the 1 arch that
> actually uses it. If there were other archs using it and for them the
> code generation would be negatively impacted (they'd actually generate
> more core because of it) it might be worth looking at.

Hi Peter,

It's OK if you don't like it. I am just a little confused about 2
things:

1) Waiman said "- _QR_BIAS" is used to simulate the xaddr which is only
in x86. So removing it to make it be consistent with other ARCHs, why is
it not good? And you said making the code worse, could you be more
specific?

2) Previously you said this change will make it bigger, I thought you
means the cnts. Am I correct? Or you mean other thing? 

Just want to understand it clearly, please point out what's wrong about
my understanding.