On Thu, Feb 13, 2025 at 01:30:33PM -0500, Joel Fernandes wrote:
> +rcu for archives
>
> Hi,
>
> Following up about our gpwrap discussions, I did some tracing of rdp->gpwrap
> with just boot testing.
>
> I actually never see it set because rdp->gp_seq and rnp->gp_seq are usually
> very close.
>
> Example trace:
>
> # rnp->gp_seq starts with -1200 on boot and then right around the wrap:
>
> 178.096329: rcu_gpnum_ovf: CPU: 2, rnp->gp_seq: -3 (0xfffffffffffffffd),
> rdp->gp_seq: -3 (0xfffffffffffffffd), wrap before: 0
> 178.096330: rcu_gpnum_ovf: CPU: 2, rnp->gp_seq: -3 (0xfffffffffffffffd),
> rdp->gp_seq: -3 (0xfffffffffffffffd), wrap after: 0
> 178.100327: rcu_gpnum_ovf: CPU: 2, rnp->gp_seq: 1 (0x1), rdp->gp_seq: 1
> (0x1), wrap before: 0
> 178.100328: rcu_gpnum_ovf: CPU: 2, rnp->gp_seq: 1 (0x1), rdp->gp_seq: 1
> (0x1), wrap after: 0
>
> The wrap "before" and "after" are the value of gpwrap before and after
> calling rcu_gpnum_ovf().
>
> Closer look at the _ovf() shows it should be only set if rdp->gp_seq and
> rnp->gp_seq are quite a distance apart (say if the CPU was idle for too long
> and many GPs have passed. This can happen both with/without wrap. So imminent
> wrapping seems not necessary for ->gpwrap to be even set AFAICS.
>
> I think the problem is ULONG_CMP_LT is not really "<" so even after wrap, it
> can false. i.e if rdp->gpseq + ULONG/4 wraps, it could still return false.
Exactly, and all of that is in fact the intended behavior.
> >From a testing POV, the example shows it is not set even when around when a
> wrap actually happened. So may be, we are not testing gpwrap much, or at
> all, with rcutorture?
True enough, and that would be well worth fixing.
One simple (and maybe too simple) fix is to add a Kconfig option to
kernel/rcu/Kconfig.debug that makes rcu_gpnum_ovf() use something like
(32 << RCU_SEQ_CTR_SHIFT). This, or some similar value, should cause
rcutorture's stuttering and/or shuffling to trigger setting of ->gpwrap
within a few minutes.
And I would not be surprised if doing this located bugs.
> But before that, I am feeling it is not behaving correctly. I'd expect it to
> be set even if rnp and rdp values are close but we are actually about to
> wrap. So most likely I am missing something.
If I understand correctly, you would like ->gpwrap to be set
unconditionally at various points in the cycle, for example, at zero,
ULONG_MAX/4, ULONG_MAX/2, and 3*ULONG_MAX/4. But this could easily
have the effect of discarding each CPU's quiescent states that showed
up before the first call to rcu_watching_snap_recheck(), even if that
CPU was active. Our users might not consider this to be a friendly act.
In contrast, the current setup only discards quiescent states for CPUs
that just came back from idle prior to call to rcu_watching_snap_save().
In addition, use of the aforementioned Kconfig option has the benefit of
making any ->gpwrap-related bugs appear during testing, not in production.
On the same grounds, one could argue that absence of this Kconfig option
should also cause gp_seq to zero rather than the current;
.gp_seq = (0UL - 300UL) << RCU_SEQ_CTR_SHIFT,
One argument against is that production 32-bit systems can wrap ->gp_seq
in a relatively short time. Maybe moreso for ->expedited_sequence, so
perhaps leave ->gp_seq and modify ->expedited_sequence. Maybe similar
decisions for others as well. ;-)
Thanx, Paul
