On Wed, 16 Oct 2024 10:23:14 -0400, llong@xxxxxxxxxx wrote:
>> +static inline void rwsem_set_owner_upgrade(struct rw_semaphore *sem)
>> +{
>> + lockdep_assert_preemption_disabled();
>> + atomic_long_set(&sem->owner, (long)current | RWSEM_UPGRADING |
>> + RWSEM_READER_OWNED | RWSEM_NONSPINNABLE);
>> +}
>
>Because of possible racing between 2 competing upgraders, read lock
>owner setting has to be atomic to avoid one overwriting the others.
I did concurrent processing at the very beginning of the function
__upgrade_read(). Is it not necessary to do concurrent processing here?
The relevant code is as follows.
+static inline int __upgrade_read(struct rw_semaphore *sem)
+{
+ long tmp;
+
+ preempt_disable();
+
+ tmp = atomic_long_read(&sem->count);
+ do {
+ if (tmp & (RWSEM_WRITER_MASK | RWSEM_FLAG_UPGRADE_READ)) {
+ preempt_enable();
+ return -EBUSY;
+ }
+ } while (!atomic_long_try_cmpxchg(&sem->count, &tmp,
+ tmp + RWSEM_FLAG_UPGRADE_READ + RWSEM_WRITER_LOCKED - RWSEM_READER_BIAS));
>This new interface should have an API similar to a trylock. True if
>successful, false otherwise. I like the read_try_upgrade() name.
I can't agree more. I will add an read_try_upgrade() API in v2.
>Another alternative that I have been thinking about is a down_read()
>variant with intention to upgrade later. This will ensure that only one
>active reader is allowed to upgrade later. With this, upgrade_read()
>will always succeed, maybe with some sleeping, as long as the correct
>down_read() is used.
I haven't figured out how to do this yet. If the current upgrade_read
idea is also OK, should I continue to complete this patchset according
to this idea?
>Cheers,
>Longman
Thanks for your review!
