Re: [PATCH v2 4/5] tree/nocb: Improve readability of nocb_gp_wait()

[Joel Fernandes]

On Tue, Aug 29, 2023 at 10:45 AM Frederic Weisbecker
<frederic@xxxxxxxxxx> wrote:
>
> On Sat, Jul 29, 2023 at 02:27:34PM +0000, Joel Fernandes (Google) wrote:
> >  /*
> >   * No-CBs GP kthreads come here to wait for additional callbacks to show up
> >   * or for grace periods to end.
> >   */
> >  static void nocb_gp_wait(struct rcu_data *my_rdp)
> >  {
> > -     bool bypass = false;
> >       int __maybe_unused cpu = my_rdp->cpu;
> >       unsigned long cur_gp_seq;
> >       unsigned long flags;
> >       bool gotcbs = false;
> > -     unsigned long j = jiffies;
> > -     bool lazy = false;
> >       bool needwait_gp = false; // This prevents actual uninitialized use.
> >       bool needwake;
> >       bool needwake_gp;
> > +     int defer_wake_type = RCU_NOCB_WAKE_NOT;
> >       struct rcu_data *rdp, *rdp_toggling = NULL;
> >       struct rcu_node *rnp;
> >       unsigned long wait_gp_seq = 0; // Suppress "use uninitialized" warning.
> > @@ -712,44 +758,24 @@ static void nocb_gp_wait(struct rcu_data *my_rdp)
> >        * won't be ignored for long.
> >        */
> >       list_for_each_entry(rdp, &my_rdp->nocb_head_rdp, nocb_entry_rdp) {
> > -             long bypass_ncbs;
> > -             bool flush_bypass = false;
> > -             long lazy_ncbs;
> > +             int defer_wake_type_one = RCU_NOCB_WAKE_NOT;
>
> No need to initialize it, nocb_gp_flush_wake() always returns a value, and
> it will avoid mistakes in the future if nocb_gp_flush_wake() is moved and
> accidentally not called.

Compiler does not like that if it is not initialized:
tree_nocb.h:821:51: error: ‘defer_wake_type’ is used uninitialized
[-Werror=uninitialized]

> > +             bool flushed;
> > +             bool empty;
> >
> > -             trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("Check"));
> >               rcu_nocb_lock_irqsave(rdp, flags);
> > -             lockdep_assert_held(&rdp->nocb_lock);
> > -             bypass_ncbs = rcu_cblist_n_cbs(&rdp->nocb_bypass);
> > -             lazy_ncbs = READ_ONCE(rdp->lazy_len);
> > +             defer_wake_type_one = nocb_gp_flush_wake(rdp, &empty, &flushed);
> >
> > -             if (bypass_ncbs && (lazy_ncbs == bypass_ncbs) &&
> > -                 (time_after(j, READ_ONCE(rdp->nocb_bypass_first) + jiffies_till_flush) ||
> > -                  bypass_ncbs > 2 * qhimark)) {
> > -                     flush_bypass = true;
> > -             } else if (bypass_ncbs && (lazy_ncbs != bypass_ncbs) &&
> > -                 (time_after(j, READ_ONCE(rdp->nocb_bypass_first) + 1) ||
> > -                  bypass_ncbs > 2 * qhimark)) {
> > -                     flush_bypass = true;
> > -             } else if (!bypass_ncbs && rcu_segcblist_empty(&rdp->cblist)) {
> > -                     rcu_nocb_unlock_irqrestore(rdp, flags);
> > -                     continue; /* No callbacks here, try next. */
> > -             }
> > +             // We may need to do a deferred wakeup later for bypass/lazy
> > +             // So note down what we learnt from the rdp.
> > +             defer_wake_type = max(defer_wake_type_one, defer_wake_type);
> >
> > -             if (flush_bypass) {
> > -                     // Bypass full or old, so flush it.
> > -                     (void)rcu_nocb_try_flush_bypass(rdp, j);
> > -                     bypass_ncbs = rcu_cblist_n_cbs(&rdp->nocb_bypass);
> > -                     lazy_ncbs = READ_ONCE(rdp->lazy_len);
> > +             // Did we make any updates to main cblist? If not, no
> > +             // non-deferred wake up to do for this rdp.
> > +             if (!flushed && empty) {
>
> Can you ever have (flushed && empty) ? If not you should be able to remove the
> flushed parameter.

We can return 3 different states from the new nocb_gp_flush_wake():
1. flushed = true and empty = false
2. flushed = false and empty = true
3. flushed = false and empty = false

I guess you mean, we don't care about case #1 and #3 ?

That's a good point. What I will do then is pass a single bool
"wakeup" and and set it for case #2 from within the
nocb_gp_flush_wake().

Thanks!

 - Joel