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
