Message-ID: <CAHk-=wjozC9_JCdEW9K_uruJqzTLzhtcVpgDk1OuqErNRUS7Mg@mail.gmail.com>
Date: Wed, 8 Oct 2025 09:05:17 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Alexander Viro <viro@...iv.linux.org.uk>, Boqun Feng <boqun.feng@...il.com>, 
	David Howells <dhowells@...hat.com>, Ingo Molnar <mingo@...hat.com>, 
	Li RongQing <lirongqing@...du.com>, Peter Zijlstra <peterz@...radead.org>, 
	Waiman Long <longman@...hat.com>, Will Deacon <will@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] seqlock: introduce scoped_seqlock_read() and scoped_seqlock_read_irqsave()

Bah.

Now I'm reading this again, and going through it more carefully, and
now I hate your helper.

Why?

Because I think that with the new organization, you can do so much better.

So the old code used "seq" as two different values:

 - the first loop around, it's the sequence count for the lockless
attempt and is always even because of how read_seqbegin() works (and
because the original code had that whole 'nextseq' hackery.

 - the second loop around, it's just a flag that now we hold the lock

and you converted the new scheme to that same model.

Which works, but in the new scheme, it's just all *pointless*.

Why do I say that?

Because in the new scheme, you have that explicit "lockless" flag
which is so much more legible in the first place.

You use it in the for-loop, but you don't use it in the helper function.

So all the games with

        if (*seq & 1) {
       ...
                *seq = 1;

are horribly non-intuitive and shouldn't exist because it would be
much more logical to just use the 'lockless' boolean instead.

Those games only make the code harder to understand, and I think they
also make the compiler unable to just until the "loop" twice.

So instead of this:

  static inline bool
  __scoped_seqlock_read_retry(seqlock_t *lock, int *seq, unsigned long *flags)
  {
         bool retry = false;

         if (*seq & 1) {
                 if (flags)
                         read_sequnlock_excl_irqrestore(lock, *flags);
                 else
                         read_sequnlock_excl(lock);
         } else if (read_seqretry(lock, *seq)) {
                 *seq = 1;
                 retry = true;
                 if (flags)
                         read_seqlock_excl_irqsave(lock, *flags);
                 else
                         read_seqlock_excl(lock);
         }

         return retry;
  }

I think that you should make that helper function be a macro - so that
it can take the unnamed union type as its argument - and then make it
do something like this instead:

  #define __scoped_seqlock_read_retry(s) ({ s.lockless ?       \
        (s.lockless = false) || read_seqretry(lock, s.seq) ?   \
                ({ read_seqlock_excl(lock); true }) : false :  \
        ({ read_sequnlock_excl(lock); false }) })

Ok, that hasn't even been close to a compiler and may be completely
buggy, but basically the logic is

 - if lockless: set lockless to false (unconditionally to make it real
easy for the compiler), then do the seq_retry, and if that says we
should retry, it does the locking and returns true (so that the loop
is re-done)

 - if not lockless, just unlock the lock and return false to say we're done

And yes, you'd need the irqsafe version too, and yes, you could do it
with the "if (flags)" thing or you could duplicate that macro, but you
could also just pass the lock/unlock sequence as an argument, and now
that macro looks even simpler

  #define __scoped_seqlock_retry(s, lock, unlock) ({ s.lockless ?  \
        (s.lockless = false) || read_seqretry(lock, s.seq) ?       \
                ({ lock ; true; }) : false :                       \
        ({ unlock ; false }) })

although then the users or that macro will be a bit uglier (maybe do
that with another level of macro to make each step simpler).

And I think the compiler will be able to optimize this better, because
the compiler actually can just follow the 's.lockless' tests and turn
all those tests into static jumps when it unrolls that loop twice
(first the lockless = true, then the lockless = false).

Again: I DID NOT FEED THIS TO A COMPILER. It may have syntax errors.
It may have logic errors,

Or it may be me having a complete breakdown and spouting nonsense. But
when re-reading your __scoped_seqlock_read_retry() helper, I really
really hated how it played those games with even/odd *seq, and I think
it's unnecessary.

Am I making sense? Who knows?

             Linus

Powered by blists - more mailing lists