Re: [PATCH v5] netfilter: nfnetlink_queue: optimize verdict lookup with hash table

[Florian Westphal]

Scott Mitchell <scott.k.mitch1@xxxxxxxxx> wrote:
> > > +     NFQA_CFG_HASH_SIZE,             /* __u32 hash table size (rounded to power of 2) */
> >
> > This should use the rhashtable implementation, I don't find a good
> > reason why this is not used in first place for this enhancement.
> 
> Thank you for the review! I can make the changes. Before implementing,
> I have a few questions to ensure I understand the preferred approach:
> 
> 1. For the "perns" allocation comment - which approach did you have in mind:
>   a) Shared rhashtable in nfnl_queue_net (initialized in
> nfnl_queue_net_init) with key={queue_num, packet_id}
>   b) Per-instance rhashtable in nfqnl_instance, with lock refactoring

You could also go with c), single rhashtable created at module init
time, like what af_netlink.c is doing.

hash and compare function would then have to include struct net *
in the hash and the compare.

b) makes no sense; if you do the lock refactoring to also allow
   GFP_ACCOUNT you could also keep the existing hashtable approach,
   I think.

> 2. The lock refactoring (GFP_ATOMIC → GFP_KERNEL) is independent of
> the hash structure choice, correct? We could fix that separately?

Not needed if you go with a) or c).

> 3. Can you help me understand the trade-offs you considered for
> rhashtable vs hlist_head? Removing the API makes sense, and I want to
> better understand how to weigh that against runtime overhead (RCU,
> locks, atomic ops) for future design decisions.

I think for this not using rhashtable is fine, but as-is the patch would
allow almost unlimited memory consumption due to ability to create 64k
queues.