[PATCH RFC 0/3] NFQUEUE: introduce CPU fanout

[PATCH RFC 0/3] NFQUEUE: introduce CPU fanout

[holger]

Hi,

with the following patchset I'd like to improve the NFQUEUE
performance if the --queue-balance argument is used for steering
packets to the NFQUEUEs.  By changing the way which NFQUEUE is
assgined I'm able to improve the performance if the processes reading
on the NFQUEUEs are pinned correctly.

Current NFQUEUE target uses a hash, computed over source and
destination address (and other parameters), for steering the packet
to the actual NFQUEUE.  This however forgets about the fact that the
packet eventually is handled by a particular CPU on user request.

If e. g. 

  1) IRQ affinity is used to handle packets on a particular CPU already
     (both single-queue or multi-queue case)

and/or

  2) RPS is used to steer packets to a specific softirq

the target easily chooses an NFQUEUE which is not handled by a process
pinned to the same CPU.

The idea is therefore to use the CPU index for determining the
NFQUEUE handling the packet.

E. g. when having a system with 4 CPUs, 4 MQ queues and 4 NFQUEUEs it
looks like this:

 +-----+  +-----+  +-----+  +-----+
 |NFQ#0|  |NFQ#1|  |NFQ#2|  |NFQ#3|
 +-----+  +-----+  +-----+  +-----+
    ^        ^        ^        ^
    |        |NFQUEUE |        |
    +        +        +        +
 +-----+  +-----+  +-----+  +-----+
 |rx-0 |  |rx-1 |  |rx-2 |  |rx-3 |
 +-----+  +-----+  +-----+  +-----+

The NFQUEUEs not necessarily have to start with number 0, setups with
less NFQUEUEs than packet-handling CPUs are not a problem as well.

The first patch extends the NFQUEUE target to accept a new
NFQ_FLAG_CPU_FANOUT flag.  If this is specified the target uses the
CPU index for determining the NFQUEUE being used.  I have to introduce
rev3 for this, sorry.

The 2nd patch coalesces rev1 and rev3 hashing by introducing
nfqueue_hash(), then used in both revisions.

The 3rd patch extends iptables userspace to accept the
--queue-cpu-fanout argument.

This is an RFC patchset, I very much appreciate your feedback.

Thank you.

 /Holger

[PATCH RFC 1/3] NFQUEUE: introduce CPU fanout

[holger]

[-- Attachment #1: net-next/NFQUEUE-cpu-fanout.diff --]
[-- Type: text/plain, Size: 2800 bytes --]

Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

---
 include/uapi/linux/netfilter/xt_NFQUEUE.h |    8 ++++++
 net/netfilter/xt_NFQUEUE.c                |   41 ++++++++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/netfilter/xt_NFQUEUE.h b/include/uapi/linux/netfilter/xt_NFQUEUE.h
index 9eafdbb..1f24680 100644
--- a/include/uapi/linux/netfilter/xt_NFQUEUE.h
+++ b/include/uapi/linux/netfilter/xt_NFQUEUE.h
@@ -26,4 +26,12 @@ struct xt_NFQ_info_v2 {
 	__u16 bypass;
 };
 
+struct xt_NFQ_info_v3 {
+	__u16 queuenum;
+	__u16 queues_total;
+	__u16 bypass;
+	__u16 flags;
+#define NFQ_FLAG_CPU_FANOUT		0x01 /* use current CPU (no hashing) */
+};
+
 #endif /* _XT_NFQ_TARGET_H */
diff --git a/net/netfilter/xt_NFQUEUE.c b/net/netfilter/xt_NFQUEUE.c
index 817f9e9..cf9c6a1 100644
--- a/net/netfilter/xt_NFQUEUE.c
+++ b/net/netfilter/xt_NFQUEUE.c
@@ -108,7 +108,7 @@ nfqueue_tg_v2(struct sk_buff *skb, const struct xt_action_param *par)
 
 static int nfqueue_tg_check(const struct xt_tgchk_param *par)
 {
-	const struct xt_NFQ_info_v2 *info = par->targinfo;
+	const struct xt_NFQ_info_v3 *info = par->targinfo;
 	u32 maxid;
 
 	if (unlikely(!rnd_inited)) {
@@ -125,11 +125,39 @@ static int nfqueue_tg_check(const struct xt_tgchk_param *par)
 		       info->queues_total, maxid);
 		return -ERANGE;
 	}
-	if (par->target->revision == 2 && info->bypass > 1)
+	if (par->target->revision >= 2 && info->bypass > 1)
+		return -EINVAL;
+	if (par->target->revision == 3 && info->flags & ~NFQ_FLAG_CPU_FANOUT)
 		return -EINVAL;
+
 	return 0;
 }
 
+static unsigned int
+nfqueue_tg_v3(struct sk_buff *skb, const struct xt_action_param *par)
+{
+	const struct xt_NFQ_info_v3 *info = par->targinfo;
+	u32 queue = info->queuenum;
+
+	if (info->queues_total > 1) {
+		if (info->flags & NFQ_FLAG_CPU_FANOUT) {
+			int cpu = smp_processor_id();
+
+			queue = info->queuenum + cpu % info->queues_total;
+		} else {
+			if (par->family == NFPROTO_IPV4)
+				queue = (((u64) hash_v4(skb) * info->queues_total) >>
+						 32) + queue;
+#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
+			else if (par->family == NFPROTO_IPV6)
+				queue = (((u64) hash_v6(skb) * info->queues_total) >>
+						 32) + queue;
+#endif
+		}
+	}
+	return NF_QUEUE_NR(queue);
+}
+
 static struct xt_target nfqueue_tg_reg[] __read_mostly = {
 	{
 		.name		= "NFQUEUE",
@@ -156,6 +184,15 @@ static struct xt_target nfqueue_tg_reg[] __read_mostly = {
 		.targetsize	= sizeof(struct xt_NFQ_info_v2),
 		.me		= THIS_MODULE,
 	},
+	{
+		.name		= "NFQUEUE",
+		.revision	= 3,
+		.family		= NFPROTO_UNSPEC,
+		.checkentry	= nfqueue_tg_check,
+		.target		= nfqueue_tg_v3,
+		.targetsize	= sizeof(struct xt_NFQ_info_v3),
+		.me		= THIS_MODULE,
+	},
 };
 
 static int __init nfqueue_tg_init(void)

[PATCH RFC 2/3] NFQUEUE: coalesce IPv4 and IPv6 hashing

[holger]

[-- Attachment #1: net-next/NFQUEUE-coalesce.diff --]
[-- Type: text/plain, Size: 2291 bytes --]

Because rev1 and rev3 of the target share the same hashing
generalize it by introduing nfqueue_hash().

Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

---
 net/netfilter/xt_NFQUEUE.c |   49 +++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/net/netfilter/xt_NFQUEUE.c b/net/netfilter/xt_NFQUEUE.c
index cf9c6a1..90684a1 100644
--- a/net/netfilter/xt_NFQUEUE.c
+++ b/net/netfilter/xt_NFQUEUE.c
@@ -76,22 +76,37 @@ static u32 hash_v6(const struct sk_buff *skb)
 }
 #endif
 
-static unsigned int
-nfqueue_tg_v1(struct sk_buff *skb, const struct xt_action_param *par)
+static u32
+nfqueue_hash(const struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct xt_NFQ_info_v1 *info = par->targinfo;
-	u32 queue = info->queuenum;
+	u32 queue;
 
-	if (info->queues_total > 1) {
-		if (par->family == NFPROTO_IPV4)
-			queue = (((u64) hash_v4(skb) * info->queues_total) >>
-				 32) + queue;
+	if (par->family == NFPROTO_IPV4)
+		queue = (((u64) hash_v4(skb) * info->queues_total) >>
+			 32) + queue;
 #if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
-		else if (par->family == NFPROTO_IPV6)
-			queue = (((u64) hash_v6(skb) * info->queues_total) >>
-				 32) + queue;
+	else if (par->family == NFPROTO_IPV6)
+		queue = (((u64) hash_v6(skb) * info->queues_total) >>
+			 32) + queue;
 #endif
-	}
+	else
+		queue = info->queuenum;
+
+	return queue;
+}
+
+static unsigned int
+nfqueue_tg_v1(struct sk_buff *skb, const struct xt_action_param *par)
+{
+	const struct xt_NFQ_info_v1 *info = par->targinfo;
+	u32 queue;
+
+	if (info->queues_total > 1)
+		queue = nfqueue_hash(skb, par);
+	else
+		queue = info->queuenum;
+
 	return NF_QUEUE_NR(queue);
 }
 
@@ -144,16 +159,8 @@ nfqueue_tg_v3(struct sk_buff *skb, const struct xt_action_param *par)
 			int cpu = smp_processor_id();
 
 			queue = info->queuenum + cpu % info->queues_total;
-		} else {
-			if (par->family == NFPROTO_IPV4)
-				queue = (((u64) hash_v4(skb) * info->queues_total) >>
-						 32) + queue;
-#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
-			else if (par->family == NFPROTO_IPV6)
-				queue = (((u64) hash_v6(skb) * info->queues_total) >>
-						 32) + queue;
-#endif
-		}
+		} else
+			queue = nfqueue_hash(skb, par);
 	}
 	return NF_QUEUE_NR(queue);
 }

[PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter

[holger]

[-- Attachment #1: iptables/iptables-NFQUEUE-cpu-fanout.diff --]
[-- Type: text/plain, Size: 4030 bytes --]

Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

---
 extensions/libxt_NFQUEUE.c           |   59 +++++++++++++++++++++++++++++++++-
 include/linux/netfilter/xt_NFQUEUE.h |    8 +++++
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/extensions/libxt_NFQUEUE.c b/extensions/libxt_NFQUEUE.c
index 8c2f699..8106425 100644
--- a/extensions/libxt_NFQUEUE.c
+++ b/extensions/libxt_NFQUEUE.c
@@ -13,8 +13,10 @@ enum {
 	O_QUEUE_NUM = 0,
 	O_QUEUE_BALANCE,
 	O_QUEUE_BYPASS,
+	O_QUEUE_CPU_FANOUT,
 	F_QUEUE_NUM     = 1 << O_QUEUE_NUM,
 	F_QUEUE_BALANCE = 1 << O_QUEUE_BALANCE,
+	F_QUEUE_CPU_FANOUT = 1 << O_QUEUE_CPU_FANOUT,
 };
 
 static void NFQUEUE_help(void)
@@ -37,7 +39,15 @@ static void NFQUEUE_help_v2(void)
 {
 	NFQUEUE_help_v1();
 	printf(
-"  --queue-bypass		Bypass Queueing if no queue instance exists.\n");
+"  --queue-bypass		Bypass Queueing if no queue instance exists.\n"
+"  --queue-cpu-fanout	Use current CPU (no hashing)\n");
+}
+
+static void NFQUEUE_help_v3(void)
+{
+	NFQUEUE_help_v2();
+	printf(
+"  --queue-cpu-fanout	Use current CPU (no hashing)\n");
 }
 
 #define s struct xt_NFQ_info
@@ -48,6 +58,8 @@ static const struct xt_option_entry NFQUEUE_opts[] = {
 	{.name = "queue-balance", .id = O_QUEUE_BALANCE,
 	 .type = XTTYPE_UINT16RC, .excl = F_QUEUE_NUM},
 	{.name = "queue-bypass", .id = O_QUEUE_BYPASS, .type = XTTYPE_NONE},
+	{.name = "queue-cpu-fanout", .id = O_QUEUE_CPU_FANOUT,
+	 .type = XTTYPE_NONE, .also = O_QUEUE_BALANCE},
 	XTOPT_TABLEEND,
 };
 #undef s
@@ -92,6 +104,18 @@ static void NFQUEUE_parse_v2(struct xt_option_call *cb)
 	}
 }
 
+static void NFQUEUE_parse_v3(struct xt_option_call *cb)
+{
+	struct xt_NFQ_info_v3 *info = cb->data;
+
+	NFQUEUE_parse_v2(cb);
+	switch (cb->entry->id) {
+	case O_QUEUE_CPU_FANOUT:
+		info->flags |= NFQ_FLAG_CPU_FANOUT;
+		break;
+	}
+}
+
 static void NFQUEUE_print(const void *ip,
                           const struct xt_entry_target *target, int numeric)
 {
@@ -124,6 +148,16 @@ static void NFQUEUE_print_v2(const void *ip,
 		printf(" bypass");
 }
 
+static void NFQUEUE_print_v3(const void *ip,
+                             const struct xt_entry_target *target, int numeric)
+{
+	const struct xt_NFQ_info_v3 *info = (void *)target->data;
+
+	NFQUEUE_print_v2(ip, target, numeric);
+	if (info->flags & NFQ_FLAG_CPU_FANOUT)
+		printf(" cpu-fanout");
+}
+
 static void NFQUEUE_save(const void *ip, const struct xt_entry_target *target)
 {
 	const struct xt_NFQ_info *tinfo =
@@ -155,6 +189,16 @@ static void NFQUEUE_save_v2(const void *ip, const struct xt_entry_target *target
 		printf(" --queue-bypass");
 }
 
+static void NFQUEUE_save_v3(const void *ip,
+			    const struct xt_entry_target *target)
+{
+	const struct xt_NFQ_info_v3 *info = (void *)target->data;
+
+	NFQUEUE_save_v2(ip, target);
+	if (info->flags & NFQ_FLAG_CPU_FANOUT)
+		printf(" --queue-cpu-fanout");
+}
+
 static void NFQUEUE_init_v1(struct xt_entry_target *t)
 {
 	struct xt_NFQ_info_v1 *tinfo = (void *)t->data;
@@ -199,6 +243,19 @@ static struct xtables_target nfqueue_targets[] = {
 	.save		= NFQUEUE_save_v2,
 	.x6_parse	= NFQUEUE_parse_v2,
 	.x6_options	= NFQUEUE_opts,
+},{
+	.family		= NFPROTO_UNSPEC,
+	.revision	= 3,
+	.name		= "NFQUEUE",
+	.version	= XTABLES_VERSION,
+	.size		= XT_ALIGN(sizeof(struct xt_NFQ_info_v3)),
+	.userspacesize	= XT_ALIGN(sizeof(struct xt_NFQ_info_v3)),
+	.help		= NFQUEUE_help_v3,
+	.init		= NFQUEUE_init_v1,
+	.print		= NFQUEUE_print_v3,
+	.save		= NFQUEUE_save_v3,
+	.x6_parse	= NFQUEUE_parse_v3,
+	.x6_options	= NFQUEUE_opts,
 }
 };
 
diff --git a/include/linux/netfilter/xt_NFQUEUE.h b/include/linux/netfilter/xt_NFQUEUE.h
index 9eafdbb..1f24680 100644
--- a/include/linux/netfilter/xt_NFQUEUE.h
+++ b/include/linux/netfilter/xt_NFQUEUE.h
@@ -26,4 +26,12 @@ struct xt_NFQ_info_v2 {
 	__u16 bypass;
 };
 
+struct xt_NFQ_info_v3 {
+	__u16 queuenum;
+	__u16 queues_total;
+	__u16 bypass;
+	__u16 flags;
+#define NFQ_FLAG_CPU_FANOUT		0x01 /* use current CPU (no hashing) */
+};
+
 #endif /* _XT_NFQ_TARGET_H */

Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout

[David Miller]

From: holger@eitzenberger.org
Date: Tue, 19 Mar 2013 15:14:43 +0100

> +			if (par->family == NFPROTO_IPV4)
> +				queue = (((u64) hash_v4(skb) * info->queues_total) >>
> +						 32) + queue;
> +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
> +			else if (par->family == NFPROTO_IPV6)
> +				queue = (((u64) hash_v6(skb) * info->queues_total) >>
> +						 32) + queue;
> +#endif

Maybe add a helper function so you don't have to indent so deeply.  Something
like:

static u32 compute_queue(struct sk_buff *skb, u32 queue, u16 family, u16 qtotl)
{
	u32 hash;

	if (family == NFPROTO_IPV4)
		hash = hash_v4(skb);
#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
	else if (family == NFPROTO_IPV6)
		hash = hash_v6(skb);
#endif

	return (((u64) hash * qtotl) >> 32) + queue;
}

Re: [PATCH RFC 2/3] NFQUEUE: coalesce IPv4 and IPv6 hashing

[David Miller]

From: holger@eitzenberger.org
Date: Tue, 19 Mar 2013 15:14:44 +0100

> +	if (par->family == NFPROTO_IPV4)
> +		queue = (((u64) hash_v4(skb) * info->queues_total) >>
> +			 32) + queue;
>  #if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
> -		else if (par->family == NFPROTO_IPV6)
> -			queue = (((u64) hash_v6(skb) * info->queues_total) >>
> -				 32) + queue;
> +	else if (par->family == NFPROTO_IPV6)
> +		queue = (((u64) hash_v6(skb) * info->queues_total) >>
> +			 32) + queue;
>  #endif

Similarly, add a helper function.

Since there are so many similar call sites, perhaps put it into a
common header file.

Re: [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter

[Eric Leblond]

Hello,

Cool job! This CPU-based setup has proven to be really efficient on
af_packet capture. I hope this will bring a performance boost to NFQ.

If possible, it could be interesting to be able to setup the balance
parameter by using an option in the same way fail-open option:  
       uint32_t flags = NFQA_CFG_F_FAIL_OPEN;
       uint32_t mask = NFQA_CFG_F_FAIL_OPEN;
       int r = nfq_set_queue_flags(qh, mask, flags);
This way, it is possible to tune the system without changing the
ruleset.

What do you think ?

BR,

On Tue, 2013-03-19 at 15:14 +0100, holger@eitzenberger.org wrote:
> plain text document attachment (iptables)
> Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>
> 
> ---
>  extensions/libxt_NFQUEUE.c           |   59 +++++++++++++++++++++++++++++++++-
>  include/linux/netfilter/xt_NFQUEUE.h |    8 +++++
>  2 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/extensions/libxt_NFQUEUE.c b/extensions/libxt_NFQUEUE.c
> index 8c2f699..8106425 100644
> --- a/extensions/libxt_NFQUEUE.c
> +++ b/extensions/libxt_NFQUEUE.c
> @@ -13,8 +13,10 @@ enum {
>  	O_QUEUE_NUM = 0,
>  	O_QUEUE_BALANCE,
>  	O_QUEUE_BYPASS,
> +	O_QUEUE_CPU_FANOUT,
>  	F_QUEUE_NUM     = 1 << O_QUEUE_NUM,
>  	F_QUEUE_BALANCE = 1 << O_QUEUE_BALANCE,
> +	F_QUEUE_CPU_FANOUT = 1 << O_QUEUE_CPU_FANOUT,
>  };
>  
>  static void NFQUEUE_help(void)
> @@ -37,7 +39,15 @@ static void NFQUEUE_help_v2(void)
>  {
>  	NFQUEUE_help_v1();
>  	printf(
> -"  --queue-bypass		Bypass Queueing if no queue instance exists.\n");
> +"  --queue-bypass		Bypass Queueing if no queue instance exists.\n"
> +"  --queue-cpu-fanout	Use current CPU (no hashing)\n");
> +}
> +
> +static void NFQUEUE_help_v3(void)
> +{
> +	NFQUEUE_help_v2();
> +	printf(
> +"  --queue-cpu-fanout	Use current CPU (no hashing)\n");
>  }
>  
>  #define s struct xt_NFQ_info
> @@ -48,6 +58,8 @@ static const struct xt_option_entry NFQUEUE_opts[] = {
>  	{.name = "queue-balance", .id = O_QUEUE_BALANCE,
>  	 .type = XTTYPE_UINT16RC, .excl = F_QUEUE_NUM},
>  	{.name = "queue-bypass", .id = O_QUEUE_BYPASS, .type = XTTYPE_NONE},
> +	{.name = "queue-cpu-fanout", .id = O_QUEUE_CPU_FANOUT,
> +	 .type = XTTYPE_NONE, .also = O_QUEUE_BALANCE},
>  	XTOPT_TABLEEND,
>  };
>  #undef s
> @@ -92,6 +104,18 @@ static void NFQUEUE_parse_v2(struct xt_option_call *cb)
>  	}
>  }
>  
> +static void NFQUEUE_parse_v3(struct xt_option_call *cb)
> +{
> +	struct xt_NFQ_info_v3 *info = cb->data;
> +
> +	NFQUEUE_parse_v2(cb);
> +	switch (cb->entry->id) {
> +	case O_QUEUE_CPU_FANOUT:
> +		info->flags |= NFQ_FLAG_CPU_FANOUT;
> +		break;
> +	}
> +}
> +
>  static void NFQUEUE_print(const void *ip,
>                            const struct xt_entry_target *target, int numeric)
>  {
> @@ -124,6 +148,16 @@ static void NFQUEUE_print_v2(const void *ip,
>  		printf(" bypass");
>  }
>  
> +static void NFQUEUE_print_v3(const void *ip,
> +                             const struct xt_entry_target *target, int numeric)
> +{
> +	const struct xt_NFQ_info_v3 *info = (void *)target->data;
> +
> +	NFQUEUE_print_v2(ip, target, numeric);
> +	if (info->flags & NFQ_FLAG_CPU_FANOUT)
> +		printf(" cpu-fanout");
> +}
> +
>  static void NFQUEUE_save(const void *ip, const struct xt_entry_target *target)
>  {
>  	const struct xt_NFQ_info *tinfo =
> @@ -155,6 +189,16 @@ static void NFQUEUE_save_v2(const void *ip, const struct xt_entry_target *target
>  		printf(" --queue-bypass");
>  }
>  
> +static void NFQUEUE_save_v3(const void *ip,
> +			    const struct xt_entry_target *target)
> +{
> +	const struct xt_NFQ_info_v3 *info = (void *)target->data;
> +
> +	NFQUEUE_save_v2(ip, target);
> +	if (info->flags & NFQ_FLAG_CPU_FANOUT)
> +		printf(" --queue-cpu-fanout");
> +}
> +
>  static void NFQUEUE_init_v1(struct xt_entry_target *t)
>  {
>  	struct xt_NFQ_info_v1 *tinfo = (void *)t->data;
> @@ -199,6 +243,19 @@ static struct xtables_target nfqueue_targets[] = {
>  	.save		= NFQUEUE_save_v2,
>  	.x6_parse	= NFQUEUE_parse_v2,
>  	.x6_options	= NFQUEUE_opts,
> +},{
> +	.family		= NFPROTO_UNSPEC,
> +	.revision	= 3,
> +	.name		= "NFQUEUE",
> +	.version	= XTABLES_VERSION,
> +	.size		= XT_ALIGN(sizeof(struct xt_NFQ_info_v3)),
> +	.userspacesize	= XT_ALIGN(sizeof(struct xt_NFQ_info_v3)),
> +	.help		= NFQUEUE_help_v3,
> +	.init		= NFQUEUE_init_v1,
> +	.print		= NFQUEUE_print_v3,
> +	.save		= NFQUEUE_save_v3,
> +	.x6_parse	= NFQUEUE_parse_v3,
> +	.x6_options	= NFQUEUE_opts,
>  }
>  };
>  
> diff --git a/include/linux/netfilter/xt_NFQUEUE.h b/include/linux/netfilter/xt_NFQUEUE.h
> index 9eafdbb..1f24680 100644
> --- a/include/linux/netfilter/xt_NFQUEUE.h
> +++ b/include/linux/netfilter/xt_NFQUEUE.h
> @@ -26,4 +26,12 @@ struct xt_NFQ_info_v2 {
>  	__u16 bypass;
>  };
>  
> +struct xt_NFQ_info_v3 {
> +	__u16 queuenum;
> +	__u16 queues_total;
> +	__u16 bypass;
> +	__u16 flags;
> +#define NFQ_FLAG_CPU_FANOUT		0x01 /* use current CPU (no hashing) */
> +};
> +
>  #endif /* _XT_NFQ_TARGET_H */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Eric Leblond <eric@regit.org>
Blog: https://home.regit.org/

Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout

[Jan Engelhardt]

On Tuesday 2013-03-19 15:26, David Miller wrote:
>
>> +			if (par->family == NFPROTO_IPV4)
>> +				queue = (((u64) hash_v4(skb) * info->queues_total) >>
>> +						 32) + queue;
>> +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
>> +			else if (par->family == NFPROTO_IPV6)
>> +				queue = (((u64) hash_v6(skb) * info->queues_total) >>
>> +						 32) + queue;
>> +#endif
>
>Maybe add a helper function so you don't have to indent so deeply.  Something
>like:

And combined with smart arranging of clauses, won't need an extra 
helper function.
http://www.braceless.org/Various/The%20Else%20Statement%20Considered%20Harmful

Something like

+static unsigned int
+nfqueue_tg_v3(struct sk_buff *skb, const struct xt_action_param *par)
+{
+	const struct xt_NFQ_info_v3 *info = par->targinfo;
+	u32 queue = info->queuenum;
+
+	if (info->queues_total == 1)
+		return NF_QUEUE_NR(queue);
+	if (info->flags & NFQ_FLAG_CPU_FANOUT) {
+		int cpu = smp_processor_id();
+
+		queue = info->queuenum + cpu % info->queues_total;
+	} else if (par->family == NFPROTO_IPV4) {
+		queue = (((u64) hash_v4(skb) * info->queues_total) >>
+				 32) + queue;
+#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
+	} else if (par->family == NFPROTO_IPV6) {
+		queue = (((u64) hash_v6(skb) * info->queues_total) >>
+				 32) + queue;
+	}
+#endif
+	return NF_QUEUE_NR(queue);
+}

Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout

[David Miller]

From: Jan Engelhardt <jengelh@inai.de>
Date: Tue, 19 Mar 2013 15:34:56 +0100 (CET)

> 
> On Tuesday 2013-03-19 15:26, David Miller wrote:
>>
>>> +			if (par->family == NFPROTO_IPV4)
>>> +				queue = (((u64) hash_v4(skb) * info->queues_total) >>
>>> +						 32) + queue;
>>> +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
>>> +			else if (par->family == NFPROTO_IPV6)
>>> +				queue = (((u64) hash_v6(skb) * info->queues_total) >>
>>> +						 32) + queue;
>>> +#endif
>>
>>Maybe add a helper function so you don't have to indent so deeply.  Something
>>like:
> 
> And combined with smart arranging of clauses, won't need an extra 
> helper function.
> http://www.braceless.org/Various/The%20Else%20Statement%20Considered%20Harmful
> 
> Something like

Yes, but:

> +		queue = (((u64) hash_v4(skb) * info->queues_total) >>
> +				 32) + queue;
                                 ^^^^^^

it's that terribly indented giblet that I want to disappear.

Re: [PATCH RFC 2/3] NFQUEUE: coalesce IPv4 and IPv6 hashing

[Holger Eitzenberger]

On Tue, Mar 19, 2013 at 10:27:49AM -0400, David S. Miller wrote:
> From: holger@eitzenberger.org
> Date: Tue, 19 Mar 2013 15:14:44 +0100
> 
> > +	if (par->family == NFPROTO_IPV4)
> > +		queue = (((u64) hash_v4(skb) * info->queues_total) >>
> > +			 32) + queue;
> >  #if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
> > -		else if (par->family == NFPROTO_IPV6)
> > -			queue = (((u64) hash_v6(skb) * info->queues_total) >>
> > -				 32) + queue;
> > +	else if (par->family == NFPROTO_IPV6)
> > +		queue = (((u64) hash_v6(skb) * info->queues_total) >>
> > +			 32) + queue;
> >  #endif
> 
> Similarly, add a helper function.
> 
> Since there are so many similar call sites, perhaps put it into a
> common header file.

Look at patch #2! :)

Re: [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter

[Holger Eitzenberger]

Hi Eric,

> Cool job! This CPU-based setup has proven to be really efficient on
> af_packet capture. I hope this will bring a performance boost to NFQ.
> 
> If possible, it could be interesting to be able to setup the balance
> parameter by using an option in the same way fail-open option:  
>        uint32_t flags = NFQA_CFG_F_FAIL_OPEN;
>        uint32_t mask = NFQA_CFG_F_FAIL_OPEN;
>        int r = nfq_set_queue_flags(qh, mask, flags);
> This way, it is possible to tune the system without changing the
> ruleset.
> 
> What do you think ?

seems like a good change to me.  I will add that as well!

Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout

[Florian Westphal]

holger@eitzenberger.org <holger@eitzenberger.org> wrote:
> +struct xt_NFQ_info_v3 {
> +	__u16 queuenum;
> +	__u16 queues_total;
> +	__u16 bypass;
> +	__u16 flags;
> +#define NFQ_FLAG_CPU_FANOUT		0x01 /* use current CPU (no hashing) */
> +};

minor nit:
'bypass' could be folded into 'flags' via a '#define NFQ_FLAG_BYPASS'.

Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout

[Holger Eitzenberger]

> > +struct xt_NFQ_info_v3 {
> > +	__u16 queuenum;
> > +	__u16 queues_total;
> > +	__u16 bypass;
> > +	__u16 flags;
> > +#define NFQ_FLAG_CPU_FANOUT		0x01 /* use current CPU (no hashing) */
> > +};
> 
> minor nit:
> 'bypass' could be folded into 'flags' via a '#define NFQ_FLAG_BYPASS'.

Yes, but I still need the new revision.  Thanks.

Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout

[Holger Eitzenberger]

Hi Jan,

> http://www.braceless.org/Various/The%20Else%20Statement%20Considered%20Harmful
> 
> Something like
> 
> +static unsigned int
> +nfqueue_tg_v3(struct sk_buff *skb, const struct xt_action_param *par)
> +{
> +	const struct xt_NFQ_info_v3 *info = par->targinfo;
> +	u32 queue = info->queuenum;
> +
> +	if (info->queues_total == 1)
> +		return NF_QUEUE_NR(queue);
> +	if (info->flags & NFQ_FLAG_CPU_FANOUT) {
> +		int cpu = smp_processor_id();
> +
> +		queue = info->queuenum + cpu % info->queues_total;
> +	} else if (par->family == NFPROTO_IPV4) {
> +		queue = (((u64) hash_v4(skb) * info->queues_total) >>
> +				 32) + queue;
> +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
> +	} else if (par->family == NFPROTO_IPV6) {
> +		queue = (((u64) hash_v6(skb) * info->queues_total) >>
> +				 32) + queue;
> +	}
> +#endif
> +	return NF_QUEUE_NR(queue);
> +}

having read through the webpage - and using 'goto' for error handling
in own code quite often - I don't see why your code above should be
more readable.

Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout

[Holger Eitzenberger]

On Tue, Mar 19, 2013 at 10:37:08AM -0400, David S. Miller wrote:
> From: Jan Engelhardt <jengelh@inai.de>
> Date: Tue, 19 Mar 2013 15:34:56 +0100 (CET)
> 
> > 
> > On Tuesday 2013-03-19 15:26, David Miller wrote:
> >>
> >>> +			if (par->family == NFPROTO_IPV4)
> >>> +				queue = (((u64) hash_v4(skb) * info->queues_total) >>
> >>> +						 32) + queue;
> >>> +#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
> >>> +			else if (par->family == NFPROTO_IPV6)
> >>> +				queue = (((u64) hash_v6(skb) * info->queues_total) >>
> >>> +						 32) + queue;
> >>> +#endif
> >>
> >>Maybe add a helper function so you don't have to indent so deeply.  Something
> >>like:
> > 
> > And combined with smart arranging of clauses, won't need an extra 
> > helper function.
> > http://www.braceless.org/Various/The%20Else%20Statement%20Considered%20Harmful
> > 
> > Something like
> 
> Yes, but:
> 
> > +		queue = (((u64) hash_v4(skb) * info->queues_total) >>
> > +				 32) + queue;
>                                  ^^^^^^
> 
> it's that terribly indented giblet that I want to disappear.

Fully agreed, but at this time I kept the indentation.  Take
a look at the changes done in nfqueue_tg_v1() from patch #2.

Will see how I can address your point.

Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout

[Jan Engelhardt]

On Tuesday 2013-03-19 22:34, Holger Eitzenberger wrote:

>> http://www.braceless.org/Various/The%20Else%20Statement%20Considered%20Harmful
>> 
>> Something like
>> 
>> +static unsigned int
>> +nfqueue_tg_v3(struct sk_buff *skb, const struct xt_action_param *par)
>> +{
>> +	const struct xt_NFQ_info_v3 *info = par->targinfo;
>> +	u32 queue = info->queuenum;
>> +
>> +	if (info->queues_total == 1)
>> +		return NF_QUEUE_NR(queue);
>> ...
>> +}
>
>having read through the webpage - and using 'goto' for error handling
>in own code quite often - I don't see why your code above should be
>more readable.

It was only coincidental that the webpage also mentioned goto.
The general idea (and google keywords) was called something like
"Else Considered Harmful", and the lesson consisted of early
exclusion of simple cases from the flow of the remainder of the
function. Perhaps to illustrate it better:

	list_for_each_entry(thing, &yourlist, anchor) {
		if (thing->property == MAGIC) {
			do_stuff;
		}
	}

=>

	list_for_each_entry(thing, &yourlist, anchor) {
		if (thing->property != MAGIC)
			continue;
		do_stuff;
	}

Code may become more readable as one gains one level of indent and
have some more freedom in arranging the giblets inside the do_stuff parts.

Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout

[Holger Eitzenberger]

> >having read through the webpage - and using 'goto' for error handling
> >in own code quite often - I don't see why your code above should be
> >more readable.
> 
> It was only coincidental that the webpage also mentioned goto.
> The general idea (and google keywords) was called something like
> "Else Considered Harmful", and the lesson consisted of early
> exclusion of simple cases from the flow of the remainder of the
> function. Perhaps to illustrate it better:
> 
> 	list_for_each_entry(thing, &yourlist, anchor) {
> 		if (thing->property == MAGIC) {
> 			do_stuff;
> 		}
> 	}
> 
> =>
> 
> 	list_for_each_entry(thing, &yourlist, anchor) {
> 		if (thing->property != MAGIC)
> 			continue;
> 		do_stuff;
> 	}
> 
> Code may become more readable as one gains one level of indent and
> have some more freedom in arranging the giblets inside the do_stuff parts.

Ack.

Re: [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter

[Holger Eitzenberger]

Hi Eric,

> If possible, it could be interesting to be able to setup the balance
> parameter by using an option in the same way fail-open option:  
>        uint32_t flags = NFQA_CFG_F_FAIL_OPEN;
>        uint32_t mask = NFQA_CFG_F_FAIL_OPEN;
>        int r = nfq_set_queue_flags(qh, mask, flags);
> This way, it is possible to tune the system without changing the
> ruleset.

Not sure how the FAIL_OPEN relates to the CPU fanout.  Do you want to
setup the CPU fanout (on, off) per queue?

 /Holger

Re: [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter

[Eric Leblond]

Hi,

On Sat, 2013-03-23 at 20:52 +0100, Holger Eitzenberger wrote:
> Hi Eric,
> 
> > If possible, it could be interesting to be able to setup the balance
> > parameter by using an option in the same way fail-open option:  
> >        uint32_t flags = NFQA_CFG_F_FAIL_OPEN;
> >        uint32_t mask = NFQA_CFG_F_FAIL_OPEN;
> >        int r = nfq_set_queue_flags(qh, mask, flags);
> > This way, it is possible to tune the system without changing the
> > ruleset.
> 
> Not sure how the FAIL_OPEN relates to the CPU fanout.  Do you want to
> setup the CPU fanout (on, off) per queue?

You are right on a major point. FAIL_OPEN is definitely per-queue and
CPU fanout is related to a fanout group. So we will have to have the
whole mechanism bind to a queue, warn the kernel that it is in a fanout
group...

That could be interesting as we will be able to tune the setting from
the application but this seems to be a lot of work too. And I don't see
any functional benefit omit this tuning from the application.

So, if you don't find any other one, forgot my request for now :)

BR,
-- 
Eric Leblond <eric@regit.org>
Blog: https://home.regit.org/