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

[Florian Westphal <fw@strlen.de>]

scott.k.mitch1@gmail.com <scott.k.mitch1@gmail.com> wrote:

LGTM, just a few minor nits.

> diff --git a/include/net/netfilter/nf_queue.h b/include/net/netfilter/nf_queue.h
> index 4aeffddb7586..e6803831d6af 100644
> --- a/include/net/netfilter/nf_queue.h
> +++ b/include/net/netfilter/nf_queue.h
> @@ -6,11 +6,13 @@
>  #include <linux/ipv6.h>
>  #include <linux/jhash.h>
>  #include <linux/netfilter.h>
> +#include <linux/rhashtable-types.h>
>  #include <linux/skbuff.h>
>  
>  /* Each queued (to userspace) skbuff has one of these. */
>  struct nf_queue_entry {
>  	struct list_head	list;
> +	struct rhash_head	hash_node;
>  	struct sk_buff		*skb;
>  	unsigned int		id;
>  	unsigned int		hook_index;	/* index in hook_entries->hook[] */
> @@ -20,6 +22,7 @@ struct nf_queue_entry {
>  #endif
>  	struct nf_hook_state	state;
>  	u16			size; /* sizeof(entry) + saved route keys */
> +	u16			queue_num;
>  
>  	/* extra space to store route keys */
>  };
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 8b7b39d8a109..e7372955c5a8 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
>  #define NFQNL_MAX_COPY_RANGE (0xffff - NLA_HDRLEN)
>  
> +/* Composite key for packet lookup: (net, queue_num, packet_id) */
> +struct nfqnl_packet_key {
> +	struct net *net;

possible_net_t net;

so we don't have this member for CONFIG_NET_NS=n

> +	u32 packet_id;
> +	u16 queue_num;
> +};
> +
> +/* Global rhashtable - one for entire system, all netns */
> +static struct rhashtable nfqnl_packet_map __read_mostly;
> +
>  struct nfqnl_instance {
>  	struct hlist_node hlist;		/* global list of queues */
>  	struct rcu_head rcu;
> @@ -100,6 +114,45 @@ static inline u_int8_t instance_hashfn(u_int16_t queue_num)
>  	return ((queue_num >> 8) ^ queue_num) % INSTANCE_BUCKETS;
>  }
>  
> +/* Extract composite key from nf_queue_entry for hashing */
> +static u32 nfqnl_packet_obj_hashfn(const void *data, u32 len, u32 seed)
> +{
> +	const struct nf_queue_entry *entry = data;
> +	struct nfqnl_packet_key key;
> +
> +	/* Zero entire struct including padding to ensure deterministic hashing */
> +	memset(&key, 0, sizeof(key));
> +	key.net = entry->state.net;

write_pnet()

> +	/* jhash2 requires size to be a multiple of sizeof(u32) */
> +	BUILD_BUG_ON(sizeof(key) % sizeof(u32) != 0);

I wonder if this works on 32bit.

If not, alternative is to either add __align to the struct or
to resort to jhash_3words().

Up to you.

> +/* Compare stack-allocated key against entry */
> +static int nfqnl_packet_obj_cmpfn(struct rhashtable_compare_arg *arg,
> +				  const void *obj)
> +{
> +	const struct nfqnl_packet_key *key = arg->key;
> +	const struct nf_queue_entry *entry = obj;
> +
> +	return entry->state.net != key->net ||

net_eq(entry->state.net, read_pnet( ...

> +static inline int
>  __enqueue_entry(struct nfqnl_instance *queue, struct nf_queue_entry *entry)
>  {

Is the inline keyword needed here?

> +	memset(&key, 0, sizeof(key));
> +	key.net = net;
> +	key.packet_id = id;
> +	key.queue_num = queue->queue_num;

Maybe its worth to add a small helper that fills the structure.

> +	/* Insert into hash BEFORE unicast. If failure don't send to userspace. */
> +	err = __enqueue_entry(queue, entry);
> +	if (unlikely(err)) {
> +		if (queue->flags & NFQA_CFG_F_FAIL_OPEN) {
> +			failopen = 1;
> +			err = 0;
> +		} else {
> +			queue->queue_dropped++;
> +			net_warn_ratelimited("nf_queue: hash insert failed: %d\n", err);
> +		}
> +		goto err_out_free_nskb;

This repeated conditional is not so nice, is there a way to avoid it?
E.g. new common helper or via goto.

> +	status = rhashtable_init(&nfqnl_packet_map, &nfqnl_rhashtable_params);
> +	if (status < 0) {
> +		pr_err("failed to init packet hash table\n");

Please remove this pr_err, kernel gets quite noisy when
it starts to run out of memory.  The other pr_err()s init .init
are just historic artefacts.