* The rdp->gpwrap behavior
@ 2025-02-13 18:30 Joel Fernandes
2025-02-14 8:20 ` Paul E. McKenney
0 siblings, 1 reply; 10+ messages in thread
From: Joel Fernandes @ 2025-02-13 18:30 UTC (permalink / raw)
To: rcu, paulmck
+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.
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?
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.
thanks,
- Joel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: The rdp->gpwrap behavior
2025-02-13 18:30 The rdp->gpwrap behavior Joel Fernandes
@ 2025-02-14 8:20 ` Paul E. McKenney
2025-02-14 18:03 ` Joel Fernandes
0 siblings, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2025-02-14 8:20 UTC (permalink / raw)
To: Joel Fernandes; +Cc: rcu
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: The rdp->gpwrap behavior
2025-02-14 8:20 ` Paul E. McKenney
@ 2025-02-14 18:03 ` Joel Fernandes
2025-02-15 10:28 ` Paul E. McKenney
0 siblings, 1 reply; 10+ messages in thread
From: Joel Fernandes @ 2025-02-14 18:03 UTC (permalink / raw)
To: paulmck; +Cc: rcu
Hi Paul,
On 2/14/2025 3:20 AM, Paul E. McKenney wrote:
> 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.
Ah I think I get it now, so the wrap isn't really about the counter wrapping
causing an issue, but about the design of the ULONG_CMP APIs which are designed
for comparisons that wrap around. Due to this design, there is a side effect
that the numbers being compared have to be at a distance that is less than half
the number wheel. Otherwise the comparisons of numbers yields incorrect results.
Is that accurate to say? IOW, ->gpwrap == true means, don't compare the numbers
anymore till we sort these huge deltas out.
>
>> >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.
>
Ok cool! So I will work on this next then. I am curious to see how often it
triggers both with/without such a change.
>> 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.
Got it.
> 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().
Do you mean the check of ->gpwrap in rcu_report_qs_rdp() which returns early?
> 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,
Hm, I think it still make sense to the do -300 even without debug options, just
so we have a true wrap early on, regardless of which APIs we use for comparison.
thanks,
- Joel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: The rdp->gpwrap behavior
2025-02-14 18:03 ` Joel Fernandes
@ 2025-02-15 10:28 ` Paul E. McKenney
2025-02-16 12:23 ` Joel Fernandes
0 siblings, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2025-02-15 10:28 UTC (permalink / raw)
To: Joel Fernandes; +Cc: rcu
On Fri, Feb 14, 2025 at 01:03:40PM -0500, Joel Fernandes wrote:
> Hi Paul,
>
> On 2/14/2025 3:20 AM, Paul E. McKenney wrote:
> > 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.
>
> Ah I think I get it now, so the wrap isn't really about the counter wrapping
> causing an issue, but about the design of the ULONG_CMP APIs which are designed
> for comparisons that wrap around. Due to this design, there is a side effect
> that the numbers being compared have to be at a distance that is less than half
> the number wheel. Otherwise the comparisons of numbers yields incorrect results.
> Is that accurate to say? IOW, ->gpwrap == true means, don't compare the numbers
> anymore till we sort these huge deltas out.
But "incorrect" is such a harsh and judgmental word. ;-)
Somewhat more seriously, ULONG_CMP_{GE,LT}() are defined to return the
sign (or its complement) of differences between a pair of 32-bit or 64-bit
numbers, as sizeof(unsigned long) dictates. This is, mathematically,
how you do signed differences. The result coming out negative isn't
incorrect, but instead the expected result given the numbers in question.
Current RCU use cases might interpret a negative result as "no one did
it for us yet", as in rcu_barrier(), "this CPU has not completed the
current grace period", as in __note_gp_changes(), and so on.
Now, these can get a false-positive/-negative result. In the case of
rcu_barrier(), we get a needless rcu_barrier() computation. In the case
of __note_gp_changes(), the false negative is OK, but a false positive
from wrapping all the way around, could be fatal for a CPU that was idle
for a long time (hence ->gpwrap). And so on.
> >> >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.
>
> Ok cool! So I will work on this next then. I am curious to see how often it
> triggers both with/without such a change.
If you can make it trigger without this change, you are far more patient
than I am. ;-)
> >> 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.
>
> Got it.
>
> > 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().
>
> Do you mean the check of ->gpwrap in rcu_report_qs_rdp() which returns early?
Well, it also resets the CPU to collect quiescent states for the current
grace period, but yes, it does return early.
> > 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,
>
> Hm, I think it still make sense to the do -300 even without debug options, just
> so we have a true wrap early on, regardless of which APIs we use for comparison.
It has certainly been as it is for a long time, so agreed.
Thanx, Paul
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: The rdp->gpwrap behavior
2025-02-15 10:28 ` Paul E. McKenney
@ 2025-02-16 12:23 ` Joel Fernandes
2025-02-18 13:17 ` Paul E. McKenney
0 siblings, 1 reply; 10+ messages in thread
From: Joel Fernandes @ 2025-02-16 12:23 UTC (permalink / raw)
To: paulmck; +Cc: rcu
On 2/15/2025 5:28 AM, Paul E. McKenney wrote:
> On Fri, Feb 14, 2025 at 01:03:40PM -0500, Joel Fernandes wrote:
>>>> 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.
>>
>> Ah I think I get it now, so the wrap isn't really about the counter wrapping
>> causing an issue, but about the design of the ULONG_CMP APIs which are designed
>> for comparisons that wrap around. Due to this design, there is a side effect
>> that the numbers being compared have to be at a distance that is less than half
>> the number wheel. Otherwise the comparisons of numbers yields incorrect results.
>> Is that accurate to say? IOW, ->gpwrap == true means, don't compare the numbers
>> anymore till we sort these huge deltas out.
>
> But "incorrect" is such a harsh and judgmental word. ;-)
>
> Somewhat more seriously, ULONG_CMP_{GE,LT}() are defined to return the
> sign (or its complement) of differences between a pair of 32-bit or 64-bit
> numbers, as sizeof(unsigned long) dictates. This is, mathematically,
> how you do signed differences. The result coming out negative isn't
> incorrect, but instead the expected result given the numbers in question.
>
> Current RCU use cases might interpret a negative result as "no one did
> it for us yet", as in rcu_barrier(), "this CPU has not completed the
> current grace period", as in __note_gp_changes(), and so on.
>
> Now, these can get a false-positive/-negative result. In the case of
> rcu_barrier(), we get a needless rcu_barrier() computation. In the case
> of __note_gp_changes(), the false negative is OK, but a false positive
> from wrapping all the way around, could be fatal for a CPU that was idle
> for a long time (hence ->gpwrap). And so on.
Understood and agreed! I guess what I was getting at was that ->gpwrap can be
true and false-negatives start getting reported well before a full wrap /
false-positive situation arises, I believe. And a false-negative may not be
desirable in all scenarios. In my other patch, I posted a scenario where I was
concerned about missed wakeups on a false-negative situation.
In srcutree.c
swait_event_interruptible_exclusive(
rnp->nocb_gp_wq[rcu_seq_ctr(wait_gp_seq) & 0x1],
rcu_seq_done(&rnp->gp_seq, wait_gp_seq) ||
!READ_ONCE(my_rdp->nocb_gp_sleep));
But I shamelessly admit I don't understand this code well enough to say if this
is an issue :/.
I agree depending on usecase, false-negative is a non-issue. But I am not sure
if false-negatives in all usecases is non-fatal.
>
>>>> >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.
Since minutes was too long for me :-D, I am trying just a count of 4, and I do
see 15 total wraps in just 2 minutes, on 36 CPU system.
I am trying this same patch now for 10 minute run on all scenarios:
https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/commit/?h=rcu/next&id=984c47eeb7728829901b33462baae2771af80ea0
Will try longer runs after.
Are you interested in an rcutorture patch where we force this? Would this be a
default setting where we always run it, or do we want to set it only under
torture command line options? Do we default to say a high value like 32, but
still provide an option to lower it to say 4, like I am doing?
>>>
>>> And I would not be surprised if doing this located bugs.
>>
>> Ok cool! So I will work on this next then. I am curious to see how often it
>> triggers both with/without such a change.
>
> If you can make it trigger without this change, you are far more patient
> than I am. ;-)
>
>>>> 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.
Got it, thanks.
>>
>> Got it.
>>
>>> 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().
>>
>> Do you mean the check of ->gpwrap in rcu_report_qs_rdp() which returns early?
>
> Well, it also resets the CPU to collect quiescent states for the current
> grace period, but yes, it does return early.
Got it, makes sense!
thanks,
- Joel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: The rdp->gpwrap behavior
2025-02-16 12:23 ` Joel Fernandes
@ 2025-02-18 13:17 ` Paul E. McKenney
2025-02-18 22:50 ` Joel Fernandes
0 siblings, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2025-02-18 13:17 UTC (permalink / raw)
To: Joel Fernandes; +Cc: rcu
On Sun, Feb 16, 2025 at 07:23:48AM -0500, Joel Fernandes wrote:
> On 2/15/2025 5:28 AM, Paul E. McKenney wrote:
> > On Fri, Feb 14, 2025 at 01:03:40PM -0500, Joel Fernandes wrote:
> >>>> 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.
> >>
> >> Ah I think I get it now, so the wrap isn't really about the counter wrapping
> >> causing an issue, but about the design of the ULONG_CMP APIs which are designed
> >> for comparisons that wrap around. Due to this design, there is a side effect
> >> that the numbers being compared have to be at a distance that is less than half
> >> the number wheel. Otherwise the comparisons of numbers yields incorrect results.
> >> Is that accurate to say? IOW, ->gpwrap == true means, don't compare the numbers
> >> anymore till we sort these huge deltas out.
> >
> > But "incorrect" is such a harsh and judgmental word. ;-)
> >
> > Somewhat more seriously, ULONG_CMP_{GE,LT}() are defined to return the
> > sign (or its complement) of differences between a pair of 32-bit or 64-bit
> > numbers, as sizeof(unsigned long) dictates. This is, mathematically,
> > how you do signed differences. The result coming out negative isn't
> > incorrect, but instead the expected result given the numbers in question.
> >
> > Current RCU use cases might interpret a negative result as "no one did
> > it for us yet", as in rcu_barrier(), "this CPU has not completed the
> > current grace period", as in __note_gp_changes(), and so on.
> >
> > Now, these can get a false-positive/-negative result. In the case of
> > rcu_barrier(), we get a needless rcu_barrier() computation. In the case
> > of __note_gp_changes(), the false negative is OK, but a false positive
> > from wrapping all the way around, could be fatal for a CPU that was idle
> > for a long time (hence ->gpwrap). And so on.
>
> Understood and agreed! I guess what I was getting at was that ->gpwrap can be
> true and false-negatives start getting reported well before a full wrap /
> false-positive situation arises, I believe. And a false-negative may not be
> desirable in all scenarios. In my other patch, I posted a scenario where I was
> concerned about missed wakeups on a false-negative situation.
>
> In srcutree.c
> swait_event_interruptible_exclusive(
> rnp->nocb_gp_wq[rcu_seq_ctr(wait_gp_seq) & 0x1],
> rcu_seq_done(&rnp->gp_seq, wait_gp_seq) ||
> !READ_ONCE(my_rdp->nocb_gp_sleep));
>
> But I shamelessly admit I don't understand this code well enough to say if this
> is an issue :/.
This looks like RCU NOCB code rather than SRCU code? Could you please
tell me what function this code lives in? The closest match I see is
in nocb_gp_wait()?
> I agree depending on usecase, false-negative is a non-issue. But I am not sure
> if false-negatives in all usecases is non-fatal.
Agreed, which is exactly why this fatality (or lack thereof) must be
evaluated on a use-case-by-use-case basis.
> >>>> >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.
>
> Since minutes was too long for me :-D, I am trying just a count of 4, and I do
> see 15 total wraps in just 2 minutes, on 36 CPU system.
Very good, and thank you for taking this on!
> I am trying this same patch now for 10 minute run on all scenarios:
> https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/commit/?h=rcu/next&id=984c47eeb7728829901b33462baae2771af80ea0
> Will try longer runs after.
A good prototype!
> Are you interested in an rcutorture patch where we force this? Would this be a
> default setting where we always run it, or do we want to set it only under
> torture command line options? Do we default to say a high value like 32, but
> still provide an option to lower it to say 4, like I am doing?
Something like this would be good, but only under options. (In order to
more easily debug something that only happens with hyperactive ->gpwrap
setting.) For example, defined with yet another "rcutree." module_param()
that is also recorded in kernel-parameters.txt. And printed out at boot,
perhaps in the rcu_bootup_announce_oddness() function.
We need a default setting that causes it to happen (very roughly) every
ten minutes on a given CPU, or perhaps better, every minute or so across
the system, by scaling the default by the number of CPUs. After all,
we want to test both the ->gpwrap path *and* the non-->gpwrap path.
Then that parameter's value would of course take the place of the
hard-coded "4" in your new ULONG_CMP_LT() call in rcu_gpnum_ovf().
For example, the "pr_alert("%s%s total_gpwrap_count...);" works well
for prototyping, but for production needs to take its place elsewhere,
most likely later in that same series of pr_cont() invocations.
The get_gp_wrap_count() should sum up the per-CPU ->gpwrap_count fields,
right? Ah, but you are calling it from a per-CPU loop anyway, so either
way should be fine. But either way, get_gp_wrap_count() does need a
header comment saying why. Trust me, it won't necessarily be obvious
five years from now. ;-)
And that "atomic_inc(&rdp->gpwrap_count)" could be this_cpu_inc(), or
even "WRITE_ONCE(..., ... + 1)", given that we are not worried about
losing counts once in awhile.
I *think* that it is OK to have the tree.[ch] and rcutorture.c portions
in the same patch, but whoever is pushing this to mainline might well ask
for it to be split if we happen to end up with merge conflicts. Or maybe
just get more practice handling merge conflicts, but it is their call.
Thanx, Paul
> >>> And I would not be surprised if doing this located bugs.
> >>
> >> Ok cool! So I will work on this next then. I am curious to see how often it
> >> triggers both with/without such a change.
> >
> > If you can make it trigger without this change, you are far more patient
> > than I am. ;-)
> >
> >>>> 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.
>
> Got it, thanks.
>
> >>
> >> Got it.
> >>
> >>> 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().
> >>
> >> Do you mean the check of ->gpwrap in rcu_report_qs_rdp() which returns early?
> >
> > Well, it also resets the CPU to collect quiescent states for the current
> > grace period, but yes, it does return early.
>
> Got it, makes sense!
>
> thanks,
>
> - Joel
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: The rdp->gpwrap behavior
2025-02-18 13:17 ` Paul E. McKenney
@ 2025-02-18 22:50 ` Joel Fernandes
2025-02-19 13:54 ` Paul E. McKenney
0 siblings, 1 reply; 10+ messages in thread
From: Joel Fernandes @ 2025-02-18 22:50 UTC (permalink / raw)
To: paulmck; +Cc: rcu
Hi Paul,
On 2/18/2025 8:17 AM, Paul E. McKenney wrote:
> On Sun, Feb 16, 2025 at 07:23:48AM -0500, Joel Fernandes wrote:
>> On 2/15/2025 5:28 AM, Paul E. McKenney wrote:
>>> On Fri, Feb 14, 2025 at 01:03:40PM -0500, Joel Fernandes wrote:
>>>>>> 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.
>>>>
>>>> Ah I think I get it now, so the wrap isn't really about the counter wrapping
>>>> causing an issue, but about the design of the ULONG_CMP APIs which are designed
>>>> for comparisons that wrap around. Due to this design, there is a side effect
>>>> that the numbers being compared have to be at a distance that is less than half
>>>> the number wheel. Otherwise the comparisons of numbers yields incorrect results.
>>>> Is that accurate to say? IOW, ->gpwrap == true means, don't compare the numbers
>>>> anymore till we sort these huge deltas out.
>>>
>>> But "incorrect" is such a harsh and judgmental word. ;-)
>>>
>>> Somewhat more seriously, ULONG_CMP_{GE,LT}() are defined to return the
>>> sign (or its complement) of differences between a pair of 32-bit or 64-bit
>>> numbers, as sizeof(unsigned long) dictates. This is, mathematically,
>>> how you do signed differences. The result coming out negative isn't
>>> incorrect, but instead the expected result given the numbers in question.
>>>
>>> Current RCU use cases might interpret a negative result as "no one did
>>> it for us yet", as in rcu_barrier(), "this CPU has not completed the
>>> current grace period", as in __note_gp_changes(), and so on.
>>>
>>> Now, these can get a false-positive/-negative result. In the case of
>>> rcu_barrier(), we get a needless rcu_barrier() computation. In the case
>>> of __note_gp_changes(), the false negative is OK, but a false positive
>>> from wrapping all the way around, could be fatal for a CPU that was idle
>>> for a long time (hence ->gpwrap). And so on.
>>
>> Understood and agreed! I guess what I was getting at was that ->gpwrap can be
>> true and false-negatives start getting reported well before a full wrap /
>> false-positive situation arises, I believe. And a false-negative may not be
>> desirable in all scenarios. In my other patch, I posted a scenario where I was
>> concerned about missed wakeups on a false-negative situation.
>>
>> In srcutree.c
>> swait_event_interruptible_exclusive(
>> rnp->nocb_gp_wq[rcu_seq_ctr(wait_gp_seq) & 0x1],
>> rcu_seq_done(&rnp->gp_seq, wait_gp_seq) ||
>> !READ_ONCE(my_rdp->nocb_gp_sleep));
>>
>> But I shamelessly admit I don't understand this code well enough to say if this
>> is an issue :/.
>
> This looks like RCU NOCB code rather than SRCU code? Could you please
> tell me what function this code lives in? The closest match I see is
> in nocb_gp_wait()?
Oops, sorry I was looking at too many things.
Yes, it is in nocb_gp_wait(). I meant the rcu_seq_done() in that snippet in
'tree_nocb.h'.
>>>>>> >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.
>>
>> Since minutes was too long for me :-D, I am trying just a count of 4, and I do
>> see 15 total wraps in just 2 minutes, on 36 CPU system.
>
> Very good, and thank you for taking this on!
>
>> I am trying this same patch now for 10 minute run on all scenarios:
>> https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/commit/?h=rcu/next&id=984c47eeb7728829901b33462baae2771af80ea0
>> Will try longer runs after.
>
> A good prototype!
Thanks, so far I could not produce any issue with doing this. Which could mean
that the code is robust to any wrap issues, but I guess no harm in testing it
for future-proofing?
>
>> Are you interested in an rcutorture patch where we force this? Would this be a
>> default setting where we always run it, or do we want to set it only under
>> torture command line options? Do we default to say a high value like 32, but
>> still provide an option to lower it to say 4, like I am doing?
>
> Something like this would be good, but only under options. (In order to
> more easily debug something that only happens with hyperactive ->gpwrap
> setting.) For example, defined with yet another "rcutree." module_param()
> that is also recorded in kernel-parameters.txt. And printed out at boot,
> perhaps in the rcu_bootup_announce_oddness() function.
>
> We need a default setting that causes it to happen (very roughly) every
> ten minutes on a given CPU, or perhaps better, every minute or so across
And that default is certainly not ULONG_MAX/2 but rather something that I
empirically calculate to cause a half-way wrap in 10 minutes?
> the system, by scaling the default by the number of CPUs. After all,
> we want to test both the ->gpwrap path *and* the non-->gpwrap path.
Agreed. But do we want to test both these paths only under rcutorture testing or
were you considering we do it for regular operation as well? i.e. wrap in 10
minutes in regular operation. I am guessing you want it only for rcutorture
_and_ have a 10 minute default.
If that is the case, we would then need rcu_gpnum_ovf() to consult rcutorture,
if it is enabled, about what what the threshold is.
> Then that parameter's value would of course take the place of the
> hard-coded "4" in your new ULONG_CMP_LT() call in rcu_gpnum_ovf().
>
> For example, the "pr_alert("%s%s total_gpwrap_count...);" works well
> for prototyping, but for production needs to take its place elsewhere,
> most likely later in that same series of pr_cont() invocations.
>
> The get_gp_wrap_count() should sum up the per-CPU ->gpwrap_count fields,
> right? Ah, but you are calling it from a per-CPU loop anyway, so either
> way should be fine. But either way, get_gp_wrap_count() does need a
> header comment saying why. Trust me, it won't necessarily be obvious
> five years from now. ;-)
>
> And that "atomic_inc(&rdp->gpwrap_count)" could be this_cpu_inc(), or
> even "WRITE_ONCE(..., ... + 1)", given that we are not worried about
> losing counts once in awhile.
Agreed to all these, I will make these changes.
> I *think* that it is OK to have the tree.[ch] and rcutorture.c portions
> in the same patch, but whoever is pushing this to mainline might well ask
> for it to be split if we happen to end up with merge conflicts. Or maybe
> just get more practice handling merge conflicts, but it is their call.
Ok, will see how it looks and consider splitting.
Thanks!
- Joel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: The rdp->gpwrap behavior
2025-02-18 22:50 ` Joel Fernandes
@ 2025-02-19 13:54 ` Paul E. McKenney
2025-02-19 15:57 ` Joel Fernandes
0 siblings, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2025-02-19 13:54 UTC (permalink / raw)
To: Joel Fernandes; +Cc: rcu
On Tue, Feb 18, 2025 at 05:50:27PM -0500, Joel Fernandes wrote:
> Hi Paul,
>
> On 2/18/2025 8:17 AM, Paul E. McKenney wrote:
> > On Sun, Feb 16, 2025 at 07:23:48AM -0500, Joel Fernandes wrote:
> >> On 2/15/2025 5:28 AM, Paul E. McKenney wrote:
> >>> On Fri, Feb 14, 2025 at 01:03:40PM -0500, Joel Fernandes wrote:
> >>>>>> 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.
> >>>>
> >>>> Ah I think I get it now, so the wrap isn't really about the counter wrapping
> >>>> causing an issue, but about the design of the ULONG_CMP APIs which are designed
> >>>> for comparisons that wrap around. Due to this design, there is a side effect
> >>>> that the numbers being compared have to be at a distance that is less than half
> >>>> the number wheel. Otherwise the comparisons of numbers yields incorrect results.
> >>>> Is that accurate to say? IOW, ->gpwrap == true means, don't compare the numbers
> >>>> anymore till we sort these huge deltas out.
> >>>
> >>> But "incorrect" is such a harsh and judgmental word. ;-)
> >>>
> >>> Somewhat more seriously, ULONG_CMP_{GE,LT}() are defined to return the
> >>> sign (or its complement) of differences between a pair of 32-bit or 64-bit
> >>> numbers, as sizeof(unsigned long) dictates. This is, mathematically,
> >>> how you do signed differences. The result coming out negative isn't
> >>> incorrect, but instead the expected result given the numbers in question.
> >>>
> >>> Current RCU use cases might interpret a negative result as "no one did
> >>> it for us yet", as in rcu_barrier(), "this CPU has not completed the
> >>> current grace period", as in __note_gp_changes(), and so on.
> >>>
> >>> Now, these can get a false-positive/-negative result. In the case of
> >>> rcu_barrier(), we get a needless rcu_barrier() computation. In the case
> >>> of __note_gp_changes(), the false negative is OK, but a false positive
> >>> from wrapping all the way around, could be fatal for a CPU that was idle
> >>> for a long time (hence ->gpwrap). And so on.
> >>
> >> Understood and agreed! I guess what I was getting at was that ->gpwrap can be
> >> true and false-negatives start getting reported well before a full wrap /
> >> false-positive situation arises, I believe. And a false-negative may not be
> >> desirable in all scenarios. In my other patch, I posted a scenario where I was
> >> concerned about missed wakeups on a false-negative situation.
> >>
> >> In srcutree.c
> >> swait_event_interruptible_exclusive(
> >> rnp->nocb_gp_wq[rcu_seq_ctr(wait_gp_seq) & 0x1],
> >> rcu_seq_done(&rnp->gp_seq, wait_gp_seq) ||
> >> !READ_ONCE(my_rdp->nocb_gp_sleep));
> >>
> >> But I shamelessly admit I don't understand this code well enough to say if this
> >> is an issue :/.
> >
> > This looks like RCU NOCB code rather than SRCU code? Could you please
> > tell me what function this code lives in? The closest match I see is
> > in nocb_gp_wait()?
>
> Oops, sorry I was looking at too many things.
>
> Yes, it is in nocb_gp_wait(). I meant the rcu_seq_done() in that snippet in
> 'tree_nocb.h'.
OK, same as any other use case. Can wrap happen, either across ULONG_MIN
or zero, and if so, what are the consequences? Are things better or worse
with rcu_seq_done_exact()? If a change is in order, how do we test it?
Plus, given that this is nocb code, we should get Frederic's opinion.
> >>>>>> >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.
> >>
> >> Since minutes was too long for me :-D, I am trying just a count of 4, and I do
> >> see 15 total wraps in just 2 minutes, on 36 CPU system.
> >
> > Very good, and thank you for taking this on!
> >
> >> I am trying this same patch now for 10 minute run on all scenarios:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/commit/?h=rcu/next&id=984c47eeb7728829901b33462baae2771af80ea0
> >> Will try longer runs after.
> >
> > A good prototype!
>
> Thanks, so far I could not produce any issue with doing this. Which could mean
> that the code is robust to any wrap issues, but I guess no harm in testing it
> for future-proofing?
As long as we have proper control and proper default values, as outlined
in my earlier message. ;-)
> >> Are you interested in an rcutorture patch where we force this? Would this be a
> >> default setting where we always run it, or do we want to set it only under
> >> torture command line options? Do we default to say a high value like 32, but
> >> still provide an option to lower it to say 4, like I am doing?
> >
> > Something like this would be good, but only under options. (In order to
> > more easily debug something that only happens with hyperactive ->gpwrap
> > setting.) For example, defined with yet another "rcutree." module_param()
> > that is also recorded in kernel-parameters.txt. And printed out at boot,
> > perhaps in the rcu_bootup_announce_oddness() function.
> >
> > We need a default setting that causes it to happen (very roughly) every
> > ten minutes on a given CPU, or perhaps better, every minute or so across
>
> And that default is certainly not ULONG_MAX/2 but rather something that I
> empirically calculate to cause a half-way wrap in 10 minutes?
Exactly!
Maybe also use negative numbers to allow scaling by the number of CPUs,
but that can be separate, when/if needed. (Mentioning this now to avoid
the negative numbers being used unwittingly.)
> > the system, by scaling the default by the number of CPUs. After all,
> > we want to test both the ->gpwrap path *and* the non-->gpwrap path.
>
> Agreed. But do we want to test both these paths only under rcutorture testing or
> were you considering we do it for regular operation as well? i.e. wrap in 10
> minutes in regular operation. I am guessing you want it only for rcutorture
> _and_ have a 10 minute default.
>
> If that is the case, we would then need rcu_gpnum_ovf() to consult rcutorture,
> if it is enabled, about what what the threshold is.
The rcu_gp_slow() function does something similar for the grace-period
kthread's injected delays, so that can work. Probably better than putting
the module parameter in tree.c, because putting it in rcutorture makes
in more clear that it is for debugging.
> > Then that parameter's value would of course take the place of the
> > hard-coded "4" in your new ULONG_CMP_LT() call in rcu_gpnum_ovf().
> >
> > For example, the "pr_alert("%s%s total_gpwrap_count...);" works well
> > for prototyping, but for production needs to take its place elsewhere,
> > most likely later in that same series of pr_cont() invocations.
> >
> > The get_gp_wrap_count() should sum up the per-CPU ->gpwrap_count fields,
> > right? Ah, but you are calling it from a per-CPU loop anyway, so either
> > way should be fine. But either way, get_gp_wrap_count() does need a
> > header comment saying why. Trust me, it won't necessarily be obvious
> > five years from now. ;-)
> >
> > And that "atomic_inc(&rdp->gpwrap_count)" could be this_cpu_inc(), or
> > even "WRITE_ONCE(..., ... + 1)", given that we are not worried about
> > losing counts once in awhile.
>
> Agreed to all these, I will make these changes.
>
> > I *think* that it is OK to have the tree.[ch] and rcutorture.c portions
> > in the same patch, but whoever is pushing this to mainline might well ask
> > for it to be split if we happen to end up with merge conflicts. Or maybe
> > just get more practice handling merge conflicts, but it is their call.
>
> Ok, will see how it looks and consider splitting.
Sounds good, looking forward to seeing what you come up with.
Thanx, Paul
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: The rdp->gpwrap behavior
2025-02-19 13:54 ` Paul E. McKenney
@ 2025-02-19 15:57 ` Joel Fernandes
2025-02-19 17:02 ` Paul E. McKenney
0 siblings, 1 reply; 10+ messages in thread
From: Joel Fernandes @ 2025-02-19 15:57 UTC (permalink / raw)
To: paulmck; +Cc: rcu
On 2/19/2025 8:54 AM, Paul E. McKenney wrote:
>>> the system, by scaling the default by the number of CPUs. After all,
>>> we want to test both the ->gpwrap path *and* the non-->gpwrap path.
>> Agreed. But do we want to test both these paths only under rcutorture testing or
>> were you considering we do it for regular operation as well? i.e. wrap in 10
>> minutes in regular operation. I am guessing you want it only for rcutorture
>> _and_ have a 10 minute default.
>>
>> If that is the case, we would then need rcu_gpnum_ovf() to consult rcutorture,
>> if it is enabled, about what what the threshold is.
> The rcu_gp_slow() function does something similar for the grace-period
> kthread's injected delays, so that can work. Probably better than putting
> the module parameter in tree.c, because putting it in rcutorture makes
> in more clear that it is for debugging.
Ok thanks, just to clarify:
Normal operation is still ULONG/2 threshold.
rcutorture operation will be 10 minutes (overridable by rcutorture module
option, not module option in non-test kernel code itself).
If we are in disagreement about this, please let me know. Otherwise I will work
on a patch as a next step on this.
- Joel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: The rdp->gpwrap behavior
2025-02-19 15:57 ` Joel Fernandes
@ 2025-02-19 17:02 ` Paul E. McKenney
0 siblings, 0 replies; 10+ messages in thread
From: Paul E. McKenney @ 2025-02-19 17:02 UTC (permalink / raw)
To: Joel Fernandes; +Cc: rcu
On Wed, Feb 19, 2025 at 10:57:29AM -0500, Joel Fernandes wrote:
> On 2/19/2025 8:54 AM, Paul E. McKenney wrote:
> >>> the system, by scaling the default by the number of CPUs. After all,
> >>> we want to test both the ->gpwrap path *and* the non-->gpwrap path.
> >> Agreed. But do we want to test both these paths only under rcutorture testing or
> >> were you considering we do it for regular operation as well? i.e. wrap in 10
> >> minutes in regular operation. I am guessing you want it only for rcutorture
> >> _and_ have a 10 minute default.
> >>
> >> If that is the case, we would then need rcu_gpnum_ovf() to consult rcutorture,
> >> if it is enabled, about what what the threshold is.
> > The rcu_gp_slow() function does something similar for the grace-period
> > kthread's injected delays, so that can work. Probably better than putting
> > the module parameter in tree.c, because putting it in rcutorture makes
> > in more clear that it is for debugging.
>
> Ok thanks, just to clarify:
>
> Normal operation is still ULONG/2 threshold.
Yes, please keep this be the default.
> rcutorture operation will be 10 minutes (overridable by rcutorture module
> option, not module option in non-test kernel code itself).
That sounds good also.
> If we are in disagreement about this, please let me know. Otherwise I will work
> on a patch as a next step on this.
Very good, looking forward to seeing what you come up with!
Thanx, Paul
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-02-19 17:02 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-13 18:30 The rdp->gpwrap behavior Joel Fernandes
2025-02-14 8:20 ` Paul E. McKenney
2025-02-14 18:03 ` Joel Fernandes
2025-02-15 10:28 ` Paul E. McKenney
2025-02-16 12:23 ` Joel Fernandes
2025-02-18 13:17 ` Paul E. McKenney
2025-02-18 22:50 ` Joel Fernandes
2025-02-19 13:54 ` Paul E. McKenney
2025-02-19 15:57 ` Joel Fernandes
2025-02-19 17:02 ` Paul E. McKenney
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.