On Sun, Jun 09, 2024 at 10:57:00PM -0400, Kent Overstreet wrote:
> On Sun, Jun 09, 2024 at 07:10:53PM -0700, Paul E. McKenney wrote:
> > On Sun, Jun 09, 2024 at 08:52:14PM -0400, Kent Overstreet wrote:
> > > On Sun, Jun 09, 2024 at 09:55:30AM -0700, Paul E. McKenney wrote:
> > > > On Sun, Jun 09, 2024 at 11:37:45AM -0400, Kent Overstreet wrote:
> > > > > On Sat, Jun 08, 2024 at 08:25:53PM -0700, Paul E. McKenney wrote:
> > > > > > Could you please try something like this just before the call to
> > > > > > cleanup_srcu_struct()?
> > > > > >
> > > > > >
> > > > > > WARN_ON_ONCE(poll_state_synchronize_srcu(&c->btree_trans_barrier,
> > > > > > ck->btree_trans_barrier_seq);
> > > > >
> > > > > Which seq was this supposed to be? All keys have been freed by this
> > > > > point...
> > > >
> > > > Or, alternatively, where in the code is this supposed to be?
> > > >
> > > > If there is no convenient point in the code to grab the most recent
> > > > return value from start_poll_synchronize_srcu(), another thing to do
> > > > is to invoke either synchronize_srcu() or synchronize_srcu_expedited()
> > > > just before the call to cleanup_srcu_struct().
> > > >
> > > > Another approach is to use get_state_synchronize_srcu() instead of
> > > > start_poll_synchronize_srcu(), and have a self-reposting SRCU callback
> > > > to keep the grace periods going. Then you would set a flag that
> > > > stopped it from self-posting, then do srcu_barrier(). With careful
> > > > memory ordering.
> > > >
> > > > There are quite a few techniques to shut down the self-reposting SRCU
> > > > callback when there is nothing for it to do and to restart it if need
> > > > be.
> > > >
> > > > But just doing a synchronize_srcu() or synchronize_srcu_expedited() is
> > > > a lot simpler and probably does the job.
> > >
> > > synchronize_srcu_expedited() seems like the simplest solution, yeah.
> > >
> > > Thanks, I think I'm starting (hazily) to get an idea of how the RCU code
> > > is structured, but I'll have to dig more when I have more time, this is
> > > interesting :)
> > >
> > > I am wondering why you couldn't just have cleanup_srcu_struct() do the
> > > appropriate cleanup (synchronize_srcu_expedited?) in this instance; if
> > > the caller is tearing down the srcu struct they don't need srcu
> > > synchronization anymore, I would think the only safety issue that would
> > > need a warning would be leaked read locks.
> >
> > Starting a grace period and then invoking cleanup_srcu_struct() before
> > it has had a chance to finish seems worth a warning. And preferable to
> > having something like poll_state_synchronize_rcu() segfault later on,
> > for example.
>
> That seems to me like it'd be entirely on the calling code for using an
> object it destroyed.
I like the way you think, but the hard cold fact would be that the
segfault would be within a function whose name contained "rcu". ;-)
But I do need to update that comment. Would the following have
helped?
> > > Another question for you: is there a limit to the number of pending
> > > sequence numbers from start_poll_synchronize_srcu()? (e.g. 2?)
> > >
> > > That affects the data structure I use for redoing this "track pending
> > > frees" code.
> >
> > Yes, there is, and you are right, the number is two. Would something
> > like the patch shown below help?
>
> Yeah, that clarifies things - thanks!
Very well, I expect to push that into the upcoming merge window. Now to
make rcutorture do the right thing with it.
Thanx, Paul
------------------------------------------------------------------------
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 9a8b7ecd2ff45..bc0d28fdc0146 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -668,7 +668,10 @@ void cleanup_srcu_struct(struct srcu_struct *ssp)
pr_info("%s: Active srcu_struct %p read state: %d gp state:
%lu/%lu\n",
__func__, ssp,
rcu_seq_state(READ_ONCE(sup->srcu_gp_seq)),
rcu_seq_current(&sup->srcu_gp_seq),
sup->srcu_gp_seq_needed);
- return; /* Caller forgot to stop doing call_srcu()? */
+ return; // Caller forgot to stop doing call_srcu()?
+ // Or caller invoked start_poll_synchronize_srcu()
+ // and then cleanup_srcu_struct() before that grace
+ // period ended?
}
kfree(sup->node);
sup->node = NULL;
