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: <339a4e0b-f323-4d48-8a1a-b7459aec53a2@redhat.com>
Date: Wed, 1 Oct 2025 15:24:21 -0400
From: Waiman Long <llong@...hat.com>
To: Oleg Nesterov <oleg@...hat.com>, Waiman Long <llong@...hat.com>
Cc: Boqun Feng <boqun.feng@...il.com>, David Howells <dhowells@...hat.com>,
 Ingo Molnar <mingo@...hat.com>, Li RongQing <lirongqing@...du.com>,
 Linus Torvalds <torvalds@...ux-foundation.org>,
 Peter Zijlstra <peterz@...radead.org>, Will Deacon <will@...nel.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] documentation: seqlock: fix the wrong documentation
 of read_seqbegin_or_lock/need_seqretry

On 10/1/25 3:06 PM, Oleg Nesterov wrote:
> On 10/01, Waiman Long wrote:
>> On 9/28/25 12:20 PM, Oleg Nesterov wrote:
>>> --- a/Documentation/locking/seqlock.rst
>>> +++ b/Documentation/locking/seqlock.rst
>>> @@ -218,13 +218,14 @@ Read path, three categories:
>>>      according to a passed marker. This is used to avoid lockless readers
>>>      starvation (too much retry loops) in case of a sharp spike in write
>>>      activity. First, a lockless read is tried (even marker passed). If
>>> -   that trial fails (odd sequence counter is returned, which is used as
>>> -   the next iteration marker), the lockless read is transformed to a
>>> -   full locking read and no retry loop is necessary::
>>> +   that trial fails (sequence counter doesn't match), make the marker
>>> +   odd for the next iteration, the lockless read is transformed to a
>>> +   full locking read and no retry loop is necessary, for example::
>>>   	/* marker; even initialization */
>>> -	int seq = 0;
>>> +	int seq = 1;
>>>   	do {
>>> +		seq++; /* 2 on the 1st/lockless path, otherwise odd */
>>>   		read_seqbegin_or_lock(&foo_seqlock, &seq);
>>>   		/* ... [[read-side critical section]] ... */
>> It is kind of odd to initialize the sequence to 1 and add an sequence
>> increment inside the loop.
> Sure. But a) in this patch my only point is that the current documentation is
> wrong, and b) the pseudo-code after this change becomes correct and the new
> pattern already have the users. For example, do_io_accounting() and more.
Thank for letting me know, but I believe my suggested change will work 
with this modified loop iteration.
>
>> Perhaps we can do something like:
> Perhaps. But could you please read the "RFC 2/1" thread? To me it is kind of
> odd that the simple loops like this example have to even touch the sequence
> counter inside the loop.
>
>> +static inline int need_seqretry_once(seqlock_t *lock, int *seq)
>> +{
>> +       int ret = !(*seq & 1) && read_seqretry(lock, *seq);
>> +
>> +       if (ret)
>> +               *seq = 1;       /* Enforce locking in next iteration */
>> +       return ret;
>> +}
> And this is exactly what I tried to propose in "RFC 2/1". Plus more...

I had read that. You used _xxx() suffix which I think a good choice will 
be to use _once() to indicate that we only want one retry.

Note that I believe the current read_seqbegin_or_lock() API was made to 
be consistent with how the read_seqbegin() or read_seqcount_begin() APIs 
are being used. I am not against your suggested single helper function, 
but it may cause confusion because it differs from the others. So it 
must be clearly documented to highlight its difference from the other 
similar APIs.

Cheers,
Longman


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ