Re: [1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact() - Patchwork

Re: [1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact() - Patchwork

[Joel Fernandes]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

Hello, Frederic,

On Tue, 1 Apr 2025 16:27:36 GMT, Frederic Weisbecker wrote:
> Le Mon, Mar 31, 2025 at 02:29:52PM -0700, Paul E. McKenney a écrit :
> > The disagreement is a feature, at least up to a point.  That feature
> > allows CPUs to go idle for long periods without RCU having to bother
> > them or to mess with their per-CPU data (give or take ->gpwrap).  It also
> > allows per-rcu_node-leaf locking, which is important on large systems.
> > 
> > Trying to make precisely globally agreed-on beginnings and ends of
> > RCU grace periods will not end well from performance, scalability,
> > or real-time-response viewpoints.  ;-)
> 
> The distributed disagreement is definetly a feature. The duplicate root
> is more debatable.
> 
> > But simplifications that don't hurt performance, scalability, and
> > real-time-response are of course welcome.
> 
> I'm not even sure my proposal is a simplification. Perhaps it is. Another
> hope is that it could avoid future accidents.
> 

Aside from the performance concerns:

Sorry if this is silly but could you provide a small hint as to how
unifying the global counter with the node affects QS reporting or hotplug?
It was not immediately obvious to me. Thanks for the help.

Thanks!

 - Joel




> > Indeed, this probably needs actual performance results showing that
> > it is needed.  My guess is that only systems with a single rcu_node
> > structure that is both leaf and root would have any chance of noticing.
> > And those tend to have few CPUs, so they might not care.
> 
> Do you have any idea for a benchmark to test here?
> 
> Thanks.
> 
> > 							Thanx, Paul

Re: [1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact() - Patchwork

[Frederic Weisbecker]

Le Tue, Apr 01, 2025 at 12:30:40PM -0400, Joel Fernandes a écrit :
> Hello, Frederic,
> 
> On Tue, 1 Apr 2025 16:27:36 GMT, Frederic Weisbecker wrote:
> > Le Mon, Mar 31, 2025 at 02:29:52PM -0700, Paul E. McKenney a écrit :
> > > The disagreement is a feature, at least up to a point.  That feature
> > > allows CPUs to go idle for long periods without RCU having to bother
> > > them or to mess with their per-CPU data (give or take ->gpwrap).  It also
> > > allows per-rcu_node-leaf locking, which is important on large systems.
> > > 
> > > Trying to make precisely globally agreed-on beginnings and ends of
> > > RCU grace periods will not end well from performance, scalability,
> > > or real-time-response viewpoints.  ;-)
> > 
> > The distributed disagreement is definetly a feature. The duplicate root
> > is more debatable.
> > 
> > > But simplifications that don't hurt performance, scalability, and
> > > real-time-response are of course welcome.
> > 
> > I'm not even sure my proposal is a simplification. Perhaps it is. Another
> > hope is that it could avoid future accidents.
> > 
> 
> Aside from the performance concerns:
> 
> Sorry if this is silly but could you provide a small hint as to how
> unifying the global counter with the node affects QS reporting or hotplug?
> It was not immediately obvious to me. Thanks for the help.

First of all rcu_seq_start() must be before the hotplug scan, otherwise
you run into this:

rcu_state.gp_seq = 4


CPU0/ rcu_gp_kthread()                                CPU 1                                                 CPU 2
-------------                                      ----------                                            -----------
//rcu_gp_init()
rcu_for_each_leaf_node(rnp) {
    raw_spin_lock_rcu_node(rnp);
    rnp->qsmaskinit = rnp->qsmaskinitnext
    raw_spin_unlock_rcu_node(rnp);
}
                                                   rcutree_report_cpu_starting()
                                                       raw_spin_lock_rcu_node(rnp);
                                                       rnp->qsmaskinitnext |= rdp->grpmask
                                                       raw_spin_unlock_rcu_node(rnp);
                                                   
                                                   rcu_read_lock()
                                                   r0 = *X
                                                                                                         r1 = *X
                                                                                                         X = NULL
                                                                                                         cookie = get_state_sychronize_rcu()
                                                                                                         //cookie = 8
rcu_seq_start(&rcu_state.gp_seq)
//rcu_state.gp_seq == 5

rcu_for_each_node_breadth_first(rnp)  {
    raw_spin_lock_rcu_node(rnp);
    // Ignore CPU 1
    rnp->qsmask = rnp->qsmaskinit;
    raw_spin_unlock_rcu_node(rnp);
}
[...]

//rcu_gp_cleanup()
rcu_seq_end(&rcu_state.gp_seq)
// rcu_state.gp_seq == 8
                                                                                                         poll_state_sychronize_rcu(cookie)
                                                                                                         kfree(r1)
                                                   r2 = *r0 // CRASH

                                                   
                                                                                                        

So the same applies if we convert rcu_state to use the root node.
But if we do rcu_seq_start() on the root node, then an update side
can call note_gp_changes() because of the state change (only if the
root node is also the unique leaf). But this then happens before
the loop that initializes all the ->qsmask

It's not a correctness problem because it won't make the rdp to
report a QS too early, since rnp->qsmask isn't intialized anyway,
but note_gp_changes() would needlessly lock the rnp lock to record
the state change in rdp->gp_seq.

This is why we need an intermediate state called RCU_SEQ_STARTED
during which note_gp_changes() can safely ignore the state change.

Then once the root's qsmask is initialized, the state can switch
to RCU_SEQ_WAIT_QS, after which calling note_gp_changes() becomes
useful.

Hope that helped.

Thanks.

Re: [1/3] rcu: Replace magic number with meaningful constant in rcu_seq_done_exact() - Patchwork

[Joel Fernandes]

Hi Frederic,

On 4/2/2025 10:22 AM, Frederic Weisbecker wrote:

>> On Tue, 1 Apr 2025 16:27:36 GMT, Frederic Weisbecker wrote:
>>> Le Mon, Mar 31, 2025 at 02:29:52PM -0700, Paul E. McKenney a écrit :
>>>> The disagreement is a feature, at least up to a point.  That feature
>>>> allows CPUs to go idle for long periods without RCU having to bother
>>>> them or to mess with their per-CPU data (give or take ->gpwrap).  It also
>>>> allows per-rcu_node-leaf locking, which is important on large systems.
>>>>
>>>> Trying to make precisely globally agreed-on beginnings and ends of
>>>> RCU grace periods will not end well from performance, scalability,
>>>> or real-time-response viewpoints.  ;-)
>>>
>>> The distributed disagreement is definetly a feature. The duplicate root
>>> is more debatable.
>>>
>>>> But simplifications that don't hurt performance, scalability, and
>>>> real-time-response are of course welcome.
>>>
>>> I'm not even sure my proposal is a simplification. Perhaps it is. Another
>>> hope is that it could avoid future accidents.
>>>
>>
>> Aside from the performance concerns:
>>
>> Sorry if this is silly but could you provide a small hint as to how
>> unifying the global counter with the node affects QS reporting or hotplug?
>> It was not immediately obvious to me. Thanks for the help.
> 
> First of all rcu_seq_start() must be before the hotplug scan, otherwise
> you run into this:
> 
> rcu_state.gp_seq = 4
> 
> 
> CPU0/ rcu_gp_kthread()                                CPU 1                                                 CPU 2
> -------------                                      ----------                                            -----------
> //rcu_gp_init()
> rcu_for_each_leaf_node(rnp) {
>     raw_spin_lock_rcu_node(rnp);
>     rnp->qsmaskinit = rnp->qsmaskinitnext
>     raw_spin_unlock_rcu_node(rnp);
> }
>                                                    rcutree_report_cpu_starting()
>                                                        raw_spin_lock_rcu_node(rnp);
>                                                        rnp->qsmaskinitnext |= rdp->grpmask
>                                                        raw_spin_unlock_rcu_node(rnp);
>                                                    
>                                                    rcu_read_lock()
>                                                    r0 = *X
>                                                                                                          r1 = *X
>                                                                                                          X = NULL
>                                                                                                          cookie = get_state_sychronize_rcu()
>                                                                                                          //cookie = 8
> rcu_seq_start(&rcu_state.gp_seq)
> //rcu_state.gp_seq == 5
> 
> rcu_for_each_node_breadth_first(rnp)  {
>     raw_spin_lock_rcu_node(rnp);
>     // Ignore CPU 1
>     rnp->qsmask = rnp->qsmaskinit;
>     raw_spin_unlock_rcu_node(rnp);
> }
> [...]
> 
> //rcu_gp_cleanup()
> rcu_seq_end(&rcu_state.gp_seq)
> // rcu_state.gp_seq == 8
>                                                                                                          poll_state_sychronize_rcu(cookie)
>                                                                                                          kfree(r1)
>                                                    r2 = *r0 // CRASH
> 
>                                                    
>                                                                                   

Makes sense! It will be too short GP if we did seq_start but missed the incoming
CPU which happened to start a reader before we did the seq_start.

> So the same applies if we convert rcu_state to use the root node.
> But if we do rcu_seq_start() on the root node, then an update side
> can call note_gp_changes() because of the state change (only if the
> root node is also the unique leaf). But this then happens before
> the loop that initializes all the ->qsmask
> 
> It's not a correctness problem because it won't make the rdp to
> report a QS too early, since rnp->qsmask isn't intialized anyway,
> but note_gp_changes() would needlessly lock the rnp lock to record
> the state change in rdp->gp_seq.
> 
> This is why we need an intermediate state called RCU_SEQ_STARTED
> during which note_gp_changes() can safely ignore the state change.

Ah ok, so it is not a correctness problem.. that's what I was wondering. It is
more of a performance optimization it sounds like.

> 
> Then once the root's qsmask is initialized, the state can switch
> to RCU_SEQ_WAIT_QS, after which calling note_gp_changes() becomes
> useful.

I agree, it is helpful to track the initialization through states and check for
that in code paths.

> Hope that helped.

It does, thanks!

 - Joel