Re: [PATCHv1 4/4] spinlock: add fair read-write locks

[Jennifer Herbert <Jennifer.Herbert@citrix.com>]

Hi Jan,

Thank you for your comments.  Sorry for the slow response - xmas and all...
Responses below...


On 22/12/15 13:54, Jan Beulich wrote:
>> +/**
>> + * rspin_until_writer_unlock - inc reader count & spin until writer is gone
> Stale comment - the function doesn't increment anything.
>
> Also throughout the file, with Linux coding style converted to Xen
> style, comment style should be made Xen-like too.

Oh yes, missed that - will fix.

>
>> +    /*
>> +     * Readers come here when they cannot get the lock without waiting
>> +     */
>> +    if ( unlikely(in_irq()) )
>> +    {
>> +        /*
>> +         * Readers in interrupt context will spin until the lock is
>> +         * available without waiting in the queue.
>> +         */
>> +        smp_rmb();
>> +        cnts = atomic_read(&lock->cnts);
>> +        rspin_until_writer_unlock(lock, cnts);
>> +        return;
>> +    }
> I can't immediately see the reason for this re-introduction of
> unfairness - can you say a word on this, or perhaps extend the
> comment?

We haven't found a reason this was introduced to Linux, but assume this 
was to reduce  interrupt latency.  I had thought to leave it there, in 
case we want to use it in the future, but now feel it would probably 
better to remove it, and deal with any irq related issues, if and when 
we use rw locks from irq handlers.


>> -        x = lock->lock & ~RW_WRITE_FLAG;
>> -    }
>> -    preempt_disable();
>> -}
>> -
>> -void _write_lock_irq(rwlock_t *lock)
>> -{
>> -    uint32_t x;
>> +        cnts = atomic_read(&lock->cnts);
>> +        if ( !(cnts & _QW_WMASK) &&
>> +             (atomic_cmpxchg(&lock->cnts, cnts,
>> +                             cnts | _QW_WAITING) == cnts) )
>> +            break;
> Considering that (at least on x86) cmpxchg is relatively expensive,
> and that atomic OR would be carried out by some cmpxchg-like
> mechanism on most other arches anyway, can't this be an atomic
> OR, followed by a read to check for another active writer?

I wanted to port know proven code.  We have now run this code in xen for 
quite a while, and while I think you may well be correct, I'd rather get 
this version in, and then consider further optimisation testing in 
future patches.

>> +unlock:
>> +    spin_unlock(&lock->lock);
> Labels indented by at least one space please.
>
> Also - are you using any of the static functions in spinlock.c? If not,
> putting the rwlock code in a new rwlock.c would help review quite a
> bit, since code removal and code addition would then be separate
> rather than intermixed.

Great idea. I think the reasons for keeping them together must just be 
historical.  I'll try and split that out.


>> +/*
>> + * Writer states & reader shift and bias
>> + */
>> +#define    _QW_WAITING  1               /* A writer is waiting     */
>> +#define    _QW_LOCKED   0xff            /* A writer holds the lock */
>> +#define    _QW_WMASK    0xff            /* Writer mask             */
>> +#define    _QR_SHIFT    8               /* Reader count shift      */
>> +#define    _QR_BIAS     (1U << _QR_SHIFT)
> Is there a particular reason for the 8-bit writer mask (a 2-bit one
> would seem to suffice)?

You picked up on optimisation in your later comment - I should add a 
comment here.

>> +static inline int _write_trylock(rwlock_t *lock)
>> +{
>> +    u32 cnts;
>> +
>> +    cnts = atomic_read(&lock->cnts);
>> +    if ( unlikely(cnts) )
>> +        return 0;
>> +
>> +    return likely(atomic_cmpxchg(&lock->cnts,
>> +                                 cnts, cnts | _QW_LOCKED) == cnts);
> The | is pointless here considering that cnts is zero.

I theorised that this was like this to aid the readability of the code, 
although I don't know if it does.  I'd happily change it over, and 
replace the cnts with 0s.

>> +static inline void _write_unlock(rwlock_t *lock)
>> +{
>> +    /*
>> +     * If the writer field is atomic, it can be cleared directly.
>> +     * Otherwise, an atomic subtraction will be used to clear it.
>> +     */
>> +    atomic_sub(_QW_LOCKED, &lock->cnts);
>> +}
> Ah, I guess the comment here is the explanation for the 8-bit
> write mask.

Yes. A comment on the declaration would no doubt help too.

>> +static inline int _rw_is_write_locked(rwlock_t *lock)
>> +{
>> +    return atomic_read(&lock->cnts) & _QW_WMASK;
>> +}
> This returns true for write-locked or writer-waiting - is this intended?

It was, but having thought about it a bit more, it would only have been 
"more useful" like this in unsafe (or at lest ugly) code, and so for 
that reason I should change it.  I don't think this would have affected 
the safe cases.
> Jan

Cheers

-jenny