On Fri, Sep 22, 2023 at 12:27:42PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Main agenda here is to not just fix the spurious failure but to
> > > get rid of the async gc worker.
> > 
> > I would like to move this sync GC collection from insert() path, it is
> > sloppy and zapping entries that we hold references to as in this case.
> > I would like to move to use the .commit phase just like pipapo.
> 
> I can experiment with this next week.
>
> I already have a patch that converts async to sync gc similar to
> pipapo but it currently keeps the limited on-demand cycle too.

I am attaching a patch I tried, it moves GC sync from insert path it
to .commit step as an alternative to:

commit 2ee52ae94baabf7ee09cf2a8d854b990dac5d0e4
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Mon Sep 4 02:14:36 2023 +0200

    netfilter: nft_set_rbtree: skip sync GC for new elements in this transaction

Moreover, Stefano reported that having redundant nodes with same key
is a problem because rotations might hide entire subtrees.

> > The only solution I can see right now is to maintain two copies of the
> > rbtree, just like pipapo, then use the .commit phase, I started
> > sketching this updates.
> 
> I would like to avoid this, see below.
> 
> > Meanwhile setting rbe_ge and rbe_le to NULL if the element that is
> > referenced is removed makes sense to me.
> 
> Great, I will submit this patch formally with a slightly updated
> commit message.

Thanks.

> > The current GC sync inlined in insert() is also making it hard to
> > support for timeout refresh (element update command) without
> > reintroducing the _BUSY bit, which is something I would like to skip.
> 
> Ugh, yes, no busy bit please.

Agreed.

> > Then, there is another possibility that is to provide a hint to
> > userspace to use pipapo instead rbtree, via _GENMSG, but there is a
> > need to update pipapo to allow for singleton sets (with no
> > concatenation), which requires a oneliner in the kernel.
> >
> > The rbtree set backend is the corner that holds more technical debt
> > IMO.
> 
> I'm all in favor of getting rid of rbtree where possible.
> So we can keep it in-tree with 'acceptable' shortcomings (= no crashes)
> but userspace would no longer use it.

OK, still it would prevent from adding timeout refresh support, that
is a new command we have to deal with (element update) and we would
need to make sure GC sync cycle embbeded into _insert() does not zap
an element that has been updated. Things get a lot easier with GC sync
from .commit() step.