Re: [PATCH v2 01/20] locking/rt: Use raw_spin_lock_irqsave() in __rwbase_read_unlock()

[Sebastian Andrzej Siewior]

On 2026-06-01 21:13:56 [+0200], Paolo Bonzini wrote:
> Il lun 1 giu 2026, 13:11 Sebastian Andrzej Siewior
> <bigeasy@xxxxxxxxxxxxx> ha scritto:
> > On 2026-06-01 10:40:45 [+0200], Peter Zijlstra wrote:
> > > > When raw_seqcount_try_begin() is used, the typical action when
> > > > the seqcount is busy is to take the same lock that the writer uses.
> > > > Another possibility, found in kernel/events/uprobes.c, is to delay
> > > > the work to a safe point using RCU.  Either way there is no need
> > > > to guarantee progress of the writer under CONFIG_PREEMPT_RT,
> > > > because the caller is not going to call raw_seqcount_try_begin() in a loop.
> >
> > This does not happen. It only happens if the associated lock is
> > preemptible. This is not the case for uprobes.c so there is no lock/
> > unlock.
> 
> The uprobes reference is just an example of a different try_begin
> pattern, that does not involve taking a lock but also doesn't need to
> guarantee progress of the writer. uprobes uses it without a lock but
> that doesn't have to be the case. I thought the commit message was
> reasonably clear but perhaps I was wrong?

I wasn't sure if you followed the code wrong or not. Based on the
example it does not happen. But I get the idea.

> > > > However, raw_seqcount_try_begin() currently uses seqprop_sequence()
> > > > via raw_read_seqcount(), and therefore does a lock/unlock of the
> > > > write-side lock on PREEMPT_RT kernel.  This prevents it from being
> > > > used in atomic context.  Use __seqprop_sequence() instead of
> > > > raw_read_seqcount() to allow it.
> >
> > So it would work if there is no lock associated or it is something like
> > raw_spinlock_t.
> 
> Yes, in which case the patch is a no-op. But in the KVM case that we
> are thinking about in this thread, raw spinlocks are not needed
> because the atomic-context read side would just punt if try_begin()
> fails; and unlike the uprobes case, there would be a lock involved.
> Since it'd be possible to use a regular spinlock, this patch would
> help.

Yes, I understand.

> > > Yeah, I think this makes sense. Thanks!
> >
> > Do I make sense or do I miss something obvious? On the other it would
> > make sense to have this even for spinlock_t assuming the try_begin
> > version does not try spin and expect to make progress (which it does not
> > for the current users).
> 
> That was my point. The lock/unlock is not causing any current bug in
> the kernel, other than maybe some minor inefficiencies, but it makes
> try_begin() subtly different for PREEMPT_RT kernels and the "obviously
> correct" way to do it isn't the right one. Having to use &s->seqcount
> is yucky.
> 
> The docs for try_begin() don't say it super explicitly, but still they
> heavily imply you're not meant to spin on it. At that point it's not a
> "try" anymore.

I don't mind the patch if the commit message would be clearer. This is
what confused me since it did not make sense based on the example.
The kernel doc says "w/o counter stabilization" but it is w/ on
PREEMPT_RT. Based on the API and the current users is not required.
Correct.
It could make sense to update the doc clarifying that a retry loop in
with raw_seqcount_try_begin() is not a valid pattern/ usage.

> Thanks,
> 
> Paolo

Sebastian