Re: [PATCH nf-next v4 01/13] rhashtable: add rhashtable_flush_and_free helper

[Florian Westphal <fw@strlen.de>]

Herbert Xu <herbert@gondor.apana.org.au> wrote:
> Florian Westphal <fw@strlen.de> wrote:
> >
> > +void rhashtable_flush_and_free(struct rhashtable *ht,
> > +                              void (*free_fn)(void *ptr, void *arg),
> > +                              void *arg)
> > +{
> > +       struct bucket_table *tbl, *old_tbl, *last_tbl, *new_tbl;
> > +       struct rhashtable_walker *walker;
> > +       unsigned int i;
> > +
> > +       new_tbl = bucket_table_alloc(ht, rounded_hashtable_size(&ht->p),
> > +                                    GFP_KERNEL);
> > +       if (!new_tbl)
> > +               new_tbl = bucket_table_alloc(ht, ht->p.min_size,
> > +                                            GFP_KERNEL | __GFP_NOFAIL);
> > +
> > +       mutex_lock(&ht->mutex);
> 
> Before starting the work you need to stop all existing rehashes.
> 
> This is where it helps if you impose restrictions on the caller.
> For example, if you could guarantee that no insertions or removals
> occur during the call to rhashtable_flush_and_free (the duration of
> the call does not include the actual freeing, which can occur later),
> then it's much easier since you could just call cancel_work_sync on
> the rehashes.
> 
> If you can't guarantee that, then we'll need some sort of a flag to
> stop the rehashes manually.

Hmm. why?  AFAICS the rehash worker holds ht->mutex, i.e. a rehash
might be pending, but it cannot run in parallel.

We called 'rcu_assign_pointer(ht->tbl, new_tbl);' before unlocking the
mutex, so a pending work (resize or rehash) will only see elements
that have been added right after that point.

Am I misreading anything here?

> > +       /* Splice the new, empty table onto the tail of the live table ... */
> > +       old_tbl = rht_dereference(ht->tbl, ht);
> > +       do {
> > +               last_tbl = rhashtable_last_table(ht, old_tbl);
> > +       } while (rhashtable_rehash_attach(ht, last_tbl, new_tbl));
> 
> This loop is only needed if you impose no restrictions on the caller.

Yes, no restrictions are imposed on the caller, at least thats the idea.

> In fact I think this could become a lot simpler since the two
> tables don't need to mix at all.
> 
> Just add an old_tbl field to struct rhashtable, then move the
> old table into it and directly write the new table to tbl.  That
> way none of the complications from overlapping insertions matter
> anymore.

Hmm. What do you think of this version?

+void rhashtable_flush_and_free(struct rhashtable *ht,
+                              void (*free_fn)(void *ptr, void *arg),
+                              void *arg)
+{
+       struct bucket_table *tbl, *old_tbl, *new_tbl;
+
+       new_tbl = bucket_table_alloc(ht, rounded_hashtable_size(&ht->p),
+                                    GFP_KERNEL);
+       if (!new_tbl)
+               new_tbl = bucket_table_alloc(ht, ht->p.min_size,
+                                            GFP_KERNEL | __GFP_NOFAIL);
+
+       /* Make sure we won't race with rhashtable_rehash_table() */
+       mutex_lock(&ht->mutex);
+       old_tbl = rcu_replace_pointer(ht->tbl, new_tbl, lockdep_rht_mutex_is_held(ht));
+       mutex_unlock(&ht->mutex);
+
+       /* Make sure all other CPUs no longer access the old table */
+       synchronize_rcu();

Rest as before.  This way, we don't need to worry about concurrency,
no readers or writers can reach old_tbl anymore.

I'll give this a try.