Re: update element timeout support [was 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>]

On Tue, Oct 03, 2023 at 08:24:47PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Right, I think that will work.
> > > For rbtree, sync gc is kept in place, elements are not zapped,
> > > they get tagged as DEAD, including the end element.
> > > 
> > > Then from commit, do full scan and remove any and all elements
> > > that are flagged as DEAD or have expired.
> > 
> > Sounds good.
> > 
> > Would you follow this approach to fix the existing issue with the
> > rbtree on-demand GC in nf.git?
> 
> Actually, I don't see why its needed. With your proposal
> to make the "is_expired" check during transaction consistently based on
> a fixed tstamp, expiry during transaction becomes impossible.
> So we can keep immediate rb_erase around.

Makes sense.

> I suggest to take my proposal to erase, signal -EAGAIN to caller,
> then have caller retry.  Apply this to nf.git as a bug fix.
> 
> Then, I can take my patches that are mixed into the gc rework;
> split those up, and we take the "no more async rbtree gc" for nf-next.
> 
> Do you still spot a problem if we retain the on-insert node erase?

Apart from this unbound loop, which sooner or later will not hit
EAGAIN, no.

> To give some numbers (async gc disabled):
> 
> Insert 20k ranges into rbtree (takes ~4minutes).
> Wait until all have expired.
> Insert a single range: takes 250ms (entire tree has to be purged).
> 
> Don't think it will be any faster with dead-bit approach,
> we simply move cost to later in the transaction.
>
> The only nf.git "advantage" is that we typically won't have
> to zap the entire tree during transaction, but thats due to
> async gc and I'd rather remove it.
> 
> What do you think?

I am fine with this approach.

What it comes, will be redo in nf-next.