Re: [PATCH nf-next 8/9] netfilter: nft_set_pipapo: move cloning of match info to insert/removal path

[Stefano Brivio <sbrivio@redhat.com>]

On Wed,  3 Apr 2024 10:41:08 +0200
Florian Westphal <fw@strlen.de> wrote:

> This set type keeps two copies of the sets' content,
>    priv->match (live version, used to match from packet path)
>    priv->clone (work-in-progress version of the 'future' priv->match).
> 
> All additions and removals are done on priv->clone.  When transaction
> completes, priv->clone becomes priv->match and a new clone is allocated
> for use by next transaction.
> 
> Problem is that the cloning requires GFP_KERNEL allocations but we
> cannot fail at either commit or abort time.
> 
> This patch defers the clone until we get an insertion or removal
> request.  This allows us to handle OOM situations correctly.
> 
> This also allows to remove ->dirty in a followup change:
> 
> If ->clone exists, ->dirty is always true
> If ->clone is NULL, ->dirty is always false, no elements were added
> or removed (except catchall elements which are external to the specific
> set backend).
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/netfilter/nft_set_pipapo.c | 62 ++++++++++++++++++++++------------
>  1 file changed, 41 insertions(+), 21 deletions(-)
> 
> diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
> index 2cc905e92889..eef6a978561f 100644
> --- a/net/netfilter/nft_set_pipapo.c
> +++ b/net/netfilter/nft_set_pipapo.c
> @@ -615,6 +615,9 @@ nft_pipapo_get(const struct net *net, const struct nft_set *set,
>  	struct nft_pipapo_match *m = priv->clone;
>  	struct nft_pipapo_elem *e;
>  
> +	if (!m)
> +		m = rcu_dereference(priv->match);
> +
>  	e = pipapo_get(net, set, m, (const u8 *)elem->key.val.data,
>  		       nft_genmask_cur(net), get_jiffies_64(),
>  		       GFP_ATOMIC);
> @@ -1259,6 +1262,23 @@ static bool nft_pipapo_transaction_mutex_held(const struct nft_set *set)
>  #endif
>  }
>  
> +static struct nft_pipapo_match *pipapo_clone(struct nft_pipapo_match *old);
> +
> +static struct nft_pipapo_match *pipapo_maybe_clone(const struct nft_set *set)

Nit:

/**
 * pipapo_maybe_clone() - Build clone for pending data changes, if not existing
 * @set:	nftables API set representation
 *
 * Return: newly created or existing clone, if any. NULL on allocation failure
 */

The rest of the series looks good (and like a big improvement) to me,
thanks! For all the patches minus 3/9,

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano