linux-kernel - Re: [RFC 1/2] rwsem: introduce upgrade

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241017064632.82771-1-lizhe.67@bytedance.com>
Date: Thu, 17 Oct 2024 14:46:32 +0800
From: lizhe.67@...edance.com
To: llong@...hat.com
Cc: akpm@...ux-foundation.org,
	boqun.feng@...il.com,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org,
	lizhe.67@...edance.com,
	mingo@...hat.com,
	peterz@...radead.org,
	will@...nel.org
Subject: Re: [RFC 1/2] rwsem: introduce upgrade_read interface

On Wed, 16 Oct 2024 10:23:14 -0400, llong@...hat.com 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!