Re: [PATCH 4/4] rcutorture: Test the irqsave forms of atomic SRCU readers

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 2026-11-10 17:33, Paul E. McKenney wrote:
> On Tue, Nov 10, 2026 at 02:28:44PM -0500, Mathieu Desnoyers wrote:
[...]
> Cute trick with the per-CPU variable for the flags value!  ;-)
>
> A few questions and comments below.
>
> I am pulling this series in for testing and further review, but I do
> understand that it might need to be replaced by a later version.

Thanks!  A v2 will follow, rebased on v7.4-rc2: patch 3 no longer
applies there, because of the kzalloc_objs() conversion in rcutorture.

[...]
>> +torture_param(bool, atomic_irqsave, false, "Half the atomic SRCU readers disable irqs");
>
> What happens if there is only one reader?  Hey, you knew that this
> question was coming!  ;-)

The choice alternates per CPU, from one reader to the next.  So even a
single reader thread alternates between the two forms.  The description
says "half", which suggests a split between threads.  v2 says
"Atomic SRCU readers alternate per CPU with irqsave readers".

[...]
>> +#define SRCU_TORTURE_IRQSAVE	0x20	// The reader disabled interrupts.
>
> Is this supposed to slot into the RCUTORTURE_RDR_* bit positions?
[...]
> If this is independent of the RCUTORTURE_RDR_* bit positions, why
> the 0x20?

It is independent of them.  It is a bit of the index that
srcu_torture_read_lock() returns, so it lands in RCUTORTURE_RDR_MASK_1
or _2.  0x20 was the first bit above the five reader flavors.

[...]
> If so, it would be good to define SRCU_TORTURE_IRQSAVE somewhere near
> the definitions of RCUTORTURE_RDR_MASK_1 and RCUTORTURE_RDR_MASK_2.
[...]
> Might you therefore be willing to instead consider the value 0x80?

Yes to both.  v2 defines it as 0x80, next to the masks, with a comment
that it is a bit of the SRCU reader index.

[...]
>> +	cur_ops->extendables &= ~(RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH |
>> +				  RCUTORTURE_RDR_IRQ);
>
> All the "srcu*" torture types implicitly set ->extendables to zero, so
> you shouldn't need this.  The exception is the "busted_srcud" torture
> type, which is supposed to fail anyway.  Or is "busted_srcud" the case
> that this code is trying to handle?
>
> If so, an alternative approach would be to only do the _irqsave()
> readers when ->extendables is equal to zero.  Which is admittedly still
> a bit hacky.

It was only defensive.  I had not checked that every srcu type but
busted_srcud has no extendables.  v2 takes your approach: the irqsave
readers run only when ->extendables is zero.  It checks this once, at
init, and keeps the result in a bool that the reader tests.

[...]
>> +		ret += ((idx & 0x1) << 4) | (idx & SRCU_TORTURE_IRQSAVE);
>
> This bit manipulation is getting a bit ornate, but that is a separate
> issue and can be dealt with later.  And who knows?  This might be optimal.

Agreed, I left it as is.

> And it passes initial "smoke" tests.  ;-)

Good to hear!  Two more points for v2, both about synchronize_srcu_atomic()
rather than this patch.  I will raise them in its cover letter.

Thanks,

Mathieu

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com