On Wed, 3 Apr 2024 10:41:03 +0200
Florian Westphal <fw@xxxxxxxxx> wrote:
> Once priv->clone can be NULL in case no insertions/removals occurred
> in the last transaction we need to drop set elements from priv->match
> if priv->clone is NULL.
>
> While at it, condense this function by reusing the pipapo_free_match
> helper instead of open-coded version.
>
> The rcu_barrier() is removed, its not needed: old call_rcu instances
> for pipapo_reclaim_match do not access struct nft_set.
True, pipapo_reclaim_match() won't, but nft_set_pipapo_match_destroy()
will, right? That is:
>
> Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
> ---
> net/netfilter/nft_set_pipapo.c | 27 ++++++---------------------
> 1 file changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
> index 48d5600f8836..d2ac2d5560e4 100644
> --- a/net/netfilter/nft_set_pipapo.c
> +++ b/net/netfilter/nft_set_pipapo.c
> @@ -2323,33 +2323,18 @@ static void nft_pipapo_destroy(const struct nft_ctx *ctx,
> {
> struct nft_pipapo *priv = nft_set_priv(set);
> struct nft_pipapo_match *m;
> - int cpu;
>
> m = rcu_dereference_protected(priv->match, true);
> - if (m) {
> - rcu_barrier();
...before b0e256f3dd2b ("netfilter: nft_set_pipapo: release elements in
clone only from destroy path"), this rcu_barrier() was needed because we'd
call nft_set_pipapo_match_destroy() on 'm'.
That call is now gone, and we could have dropped it at that point, but:
> -
> - for_each_possible_cpu(cpu)
> - pipapo_free_scratch(m, cpu);
> - free_percpu(m->scratch);
> - pipapo_free_fields(m);
> - kfree(m);
> - priv->match = NULL;
> - }
>
> if (priv->clone) {
> - m = priv->clone;
> -
> - nft_set_pipapo_match_destroy(ctx, set, m);
> -
> - for_each_possible_cpu(cpu)
> - pipapo_free_scratch(priv->clone, cpu);
> - free_percpu(priv->clone->scratch);
> -
> - pipapo_free_fields(priv->clone);
> - kfree(priv->clone);
> + nft_set_pipapo_match_destroy(ctx, set, priv->clone);
> + pipapo_free_match(priv->clone);
> priv->clone = NULL;
> + } else {
> + nft_set_pipapo_match_destroy(ctx, set, m);
now it's back, so we should actually move rcu_barrier() before this
call? I think it's needed because nft_set_pipapo_match_destroy() does
access nft_set data.
> }
> +
> + pipapo_free_match(m);
> }
>
> /**
--
Stefano
