Re: [PATCH nf 1/2] netfilter: nft_set_rbtree: move sync GC from insert path to set->ops->commit

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Florian,

Looking at your series, I don't think we are that far each other, see
below.

On Sun, Oct 01, 2023 at 11:08:16PM +0200, Florian Westphal wrote:
> I've pushed a (not very much tested) version of gc overhaul
> to passive lookups based on expiry candidates, this removes
> the need for gc sequence counters.

This patch ("netfilter: nft_set_rbtree: prefer sync gc to async
worker")

https://git.kernel.org/pub/scm/linux/kernel/git/fwestphal/nf.git/commit/?h=nft_set_gc_query_08&id=edfeb02d758d6a96a3c1c9a483b69e43e5528e87

goes in the same direction I would like to go with my incomplete patch
I posted. However:

+static void nft_rbtree_commit(struct nft_set *set)
+{
+	struct nft_rbtree *priv = nft_set_priv(set);
+
+	if (time_after_eq(jiffies, priv->last_gc + nft_set_gc_interval(set)))
+		nft_rbtree_gc(set);
+}

I don't think this time_after_eq() to postpone element removal will
work. According to Stefano, you cannot store in the rbtree tree
duplicated elements. Same problem already exists for this set backend
in case a transaction add and delete elements in the same batch.
Unless we maintain two copies. I understand you don't want to maintain
the two copies but then this time_after_eq() needs to go away.

This patch above to add .commit interface to rbtree basically undoes:

https://git.kernel.org/pub/scm/linux/kernel/git/fwestphal/nf.git/commit/?h=nft_set_gc_query_08&id=ee48e86518d62db058efafb6ec1b9f426c441a9d

so better fix it by adding the .commit interface to the rbtree in
first place?

According to what I read it seems we agree on that, the only subtle
difference between your patch and my incomplete patch is this
time_after_eq().

> Its vs. nf.git but really should be re-targetted to nf-next, I'll
> try to do this next week:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/fwestphal/nf.git/log/?h=nft_set_gc_query_08

Thanks. The gc sequence removal is a different topic we have been
discussing for a while. Would it be possible to incorrect zap an entry
with the transaction semantics? I mean:

#1 transaction to remove element k in set x y
#2 flush set x y (removes dead element k)
#3 add element k to set x y expires 3 minutes
#4 gc transaction freshly added new element

In this case, no dead flag is set on in this new element k on so GC
transaction will skip it.

As for the element timeout update semantics, I will catch up in a
separated email renaming this thread, as this is a new feature and I
prefer to re-focus the conversation on your branch, it has been me
that has been mixing up different topics anyway.