Re: [v2 PATCH] rhashtable: Add rhashtable_flush_and_free helper

[Florian Westphal <fw@strlen.de>]

Herbert Xu <herbert@gondor.apana.org.au> wrote:
> This patch adds the helper rhashtable_flush_and_free and its
> rhltable counter-part.  The intended user is netfilter:
> 
> https://sashiko.dev/#/patchset/20260828152256.8759-1-fw%40strlen.de

Thanks for working on this.

> The hash table will briefly become NULL during the flush call,
> in order to avoid race conditions against concurrent insertions
> and removals which may schedule new rehashes.

When you send next version, could you elaborate a bit why this is
required?  rht_deferred_worker grabs the ht mutex, so I don't understand
this problem.  Worst case is a single,
"useless" rehash of a most likely empty (or at least mostly empty) table.

I don't see why the

mutex_lock(&ht->mutex);
old_tbl = rcu_replace_pointer(ht->tbl, new_tbl, lockdep_rht_mutex_is_held(ht));

is bad.  It avoids the NULL checks and concurrent insertions will work.

From your previous comments, there are two cases:

1. rehash gets queued, then flush was called (before worker runs).
   This result in a "useless" rehash of a mostly empty table, but
   I don't see the problem with this (flushes are rare).

2. flush -> table replaced with empty one; *then* a rehash gets queued
   (e.g. because of parallel mass insert into the new, now
    not-so-empty-anymore table).

In this second case, the behaviour is the same as if no flush would
have happened.

-EBUSY is always possible regardless of a flush happening or not. Also,
a possible "wrong" -EBUSY is certainly better than guaranteed
-EBUSY during the synchronize_rcu() call?

That said, if it is really needed then this "guarenteed -EBUSY" is fine
for my use case: for insertions from kernel/(iptables -j SET target),
failure is already tolerated / acceptable, and insertions from userspace
serialize on the same netlink mutex as the flush request.