Re: [RFC nf-next] netfilter: nf_tables: remove element flush allocation

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Not signed off as I don't see this as more elegant as v1 here:
> > https://lore.kernel.org/netfilter-devel/20250704123024.59099-1-fw@strlen.de/
> 
> Not very elegant, maybe it is just incomplete.

Its complete (both the sleepable-iter linked above and this RFC).
The RFC omits conversion of NETSETELEM however.
I did not spend time on that because I'm not sure I'm following your
suggestion in the first place.

> > Both DEL/NEWSETELEM would be changed to first peek the transaction list
> > tail to see if a compatible transaction exists and re-use that instead
> > of allocating a new one.
> 
> Right. Would all this provide even more memory savings?

Yes.  The memory saving would come from no-need for elems[], except for
update case.

Atm we can store 124 elements in one nft_trans structure.  Each
nft_trans_elem has 2k size, to hold the pointers to the elements
added/removed.

But with list based approach you need one nft_trans struct only (96
byte) in ideal conditions (same op on same set, e.g. flush case).

So its in the 96 byte vs. 163840 bytes range for 10.000 elem del/add.

The downside is the permanent 8-byte per element increase.
But, the truckload of temporary trans objects will be gone.

> > Pablo, please let me know if you prefer this direction compared to v1.
> > If so, I would also work on removing the trailing dynamically sized
> > array from nft_trans_elem structure in a followup patch.
> 
> I don't remember when precisely, but time ago, you mentioned something
> like "this transaction infrastructure creates myriad of temporary
> objects". Your dynamic array infrastructure made it better.
> 
> Maybe it is time to integrate transaction infrastrcture more tightly
> into the existing infrastructure, so there is not need to allocate so
> many ancilliary objects for large sets?
> 
> There is a trade-off in all this.

Yes, there is.  If you agree I will try to extend the RFC patchset:
- add one patch to convert NEWSETELEM case
- add one patch to get rid of elems[].
  Unless you think there is a use case for update from userspace
  that makes a mass update of a set, such as modifying the timeout
  of 1k elements, then it would be better to keep elems[] and use it
  only for update case.  Let me know what you think -- I don't think
  its a scenario worth optimizing for.