On Wed, Nov 17, 2021 at 10:47:50PM +0100, Frederic Weisbecker wrote:
> On Wed, Nov 17, 2021 at 10:53:41AM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 17, 2021 at 04:56:32PM +0100, Frederic Weisbecker wrote:
> > > pr_info("Offloading %d\n", rdp->cpu);
> > > +
> > > + /*
> > > + * Iterate this CPU on nocb_gp_wait(). We do it before locking nocb_gp_lock,
> > > + * resetting nocb_gp_sleep and waking up the related "rcuog". Since nocb_gp_wait()
> > > + * in turn locks nocb_gp_lock before setting nocb_gp_sleep again, we are guaranteed
> > > + * to iterate this new rdp before "rcuog" goes to sleep again.
> >
> > Just to make sure that I understand...
> >
> > The ->nocb_entry_rdp list is RCU-protected, with an odd flavor of RCU.
> > The list_add_tail_rcu() handles list insertion. For list deletion,
> > on the one hand, the rcu_data structures are never freed, so their
> > lifetime never ends. But one could be removed during an de-offloading
> > operation, then re-added by a later offloading operation. It would be
> > bad if a reader came along for that ride because that reader would end
> > up skipping all the rcu_data structures that were in the list after the
> > initial position of the one that was removed and added back.
>
> How did I miss that :-(
>
> >
> > The trick seems to be that the de-offloading process cannot complete
> > until the relevant rcuog kthread has acknowledged the de-offloading,
> > which it cannot do until it has completed the list_for_each_entry_rcu()
> > loop. And the rcuog kthread is the only thing that traverses the list,
> > except for the show_rcu_nocb_state() function, more on which later.
> >
> > Therefore, it is not possible for the rcuog kthread to come along for
> > that ride.
>
> Actually it's worse than that: the node is removed _after_ the kthread
> acknowledges deoffloading and added _before_ the kthread acknowledges
> offloading. So a single rcuog loop can well run through a deletion/re-add
> pair altogether.
>
> Now since we force another loop with the new add guaranteed visible, the new
> loop should handle the missed rdp's that went shortcut by the race.
>
> Let's hope I'm not missing something else... And yes that definetly needs
> a fat comment.
>
> >
> > On to show_rcu_nocb_state()...
> >
> > This does not actually traverse the list, but instead looks at the ->cpu
> > field of the next structure. Because the ->next pointer is never NULLed,
> > the worst that can happen is that a confusing ->cpu field is printed,
> > for example, the one that was in effect prior to the de-offloading
> > operation. But that number would have been printed anyway had the
> > show_rcu_nocb_state() function not been delayed across the de-offloading
> > and offloading.
> >
> > So no harm done.
>
> Exactly, that part is racy by nature.
>
> >
> > Did I get it right? If so, the comment might need help. If not,
> > what am I missing?
>
> You got it right!
Next question... What things are the two of us put together missing? ;-)
Thanx, Paul
