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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


Hi Florian,

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

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

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:

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

> Its vs. nf.git but really should be re-targetted to nf-next, I'll
> try to do this next week:

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.

[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux