Re: [PATCH nf-next 3/9] netfilter: nft_set_pipapo: prepare destroy function for on-demand clone

[Stefano Brivio <sbrivio@redhat.com>]

On Wed,  3 Apr 2024 10:41:03 +0200
Florian Westphal <fw@strlen.de> 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@strlen.de>
> ---
>  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