Re: [PATCH] rcu: Merge rcu_seq_done_exact() logic into rcu_seq_done()

[Joel Fernandes]

On Mon, Jan 27, 2025 at 7:07 PM Joel Fernandes (Google)
<joel@xxxxxxxxxxxxxxxxx> wrote:
>
> The rcu_seq_done() API has a large "false-negative" windows of size
> ULONG_MAX/2, where after wrap around, it is possible that it will think
> that a GP has not completed if a wrap around happens and the delta is
> large.
>
> rcu_seq_done_exact() is more accurate avoiding this wrap around issue,
> by making the window of false-negativity by only 3 GPs. Use this logic
> for rcu_seq_done() which is a nice negative code delta and could
> potentially avoid issues in the future where rcu_seq_done() was
> reporting false-negatives for too long.
>
> rcutorture runs of all scenarios for 15 minutes passed. Code inspection
> was done of all users to convince the change would work.
>
> Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>

I am leaving a 60 minute overnight run of all scenarios on my personal
server for further testing.

thanks,

 - Joel




> ---
>  kernel/rcu/rcu.h  | 13 ++-----------
>  kernel/rcu/tree.c |  6 +++---
>  2 files changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index eed2951a4962..c2ca196907cb 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -146,19 +146,10 @@ static inline bool rcu_seq_started(unsigned long *sp, unsigned long s)
>
>  /*
>   * Given a snapshot from rcu_seq_snap(), determine whether or not a
> - * full update-side operation has occurred.
> + * full update-side operation has occurred while also handling
> + * wraparounds that exceed the (ULONG_MAX / 2) safety-factor/guard-band.
>   */
>  static inline bool rcu_seq_done(unsigned long *sp, unsigned long s)
> -{
> -       return ULONG_CMP_GE(READ_ONCE(*sp), s);
> -}
> -
> -/*
> - * Given a snapshot from rcu_seq_snap(), determine whether or not a
> - * full update-side operation has occurred, but do not allow the
> - * (ULONG_MAX / 2) safety-factor/guard-band.
> - */
> -static inline bool rcu_seq_done_exact(unsigned long *sp, unsigned long s)
>  {
>         unsigned long cur_s = READ_ONCE(*sp);
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index b77ccc55557b..835600cec9ba 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -4300,7 +4300,7 @@ EXPORT_SYMBOL_GPL(start_poll_synchronize_rcu_full);
>  bool poll_state_synchronize_rcu(unsigned long oldstate)
>  {
>         if (oldstate == RCU_GET_STATE_COMPLETED ||
> -           rcu_seq_done_exact(&rcu_state.gp_seq_polled, oldstate)) {
> +           rcu_seq_done(&rcu_state.gp_seq_polled, oldstate)) {
>                 smp_mb(); /* Ensure GP ends before subsequent accesses. */
>                 return true;
>         }
> @@ -4347,9 +4347,9 @@ bool poll_state_synchronize_rcu_full(struct rcu_gp_oldstate *rgosp)
>
>         smp_mb(); // Order against root rcu_node structure grace-period cleanup.
>         if (rgosp->rgos_norm == RCU_GET_STATE_COMPLETED ||
> -           rcu_seq_done_exact(&rnp->gp_seq, rgosp->rgos_norm) ||
> +           rcu_seq_done(&rnp->gp_seq, rgosp->rgos_norm) ||
>             rgosp->rgos_exp == RCU_GET_STATE_COMPLETED ||
> -           rcu_seq_done_exact(&rcu_state.expedited_sequence, rgosp->rgos_exp)) {
> +           rcu_seq_done(&rcu_state.expedited_sequence, rgosp->rgos_exp)) {
>                 smp_mb(); /* Ensure GP ends before subsequent accesses. */
>                 return true;
>         }
> --
> 2.34.1
>