Re: [RFC 1/2] rwsem: introduce upgrade_read interface

[lizhe . 67]

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!