* [PATCH RFC 0/3] NFQUEUE: introduce CPU fanout
@ 2013-03-19 14:14 holger
2013-03-19 14:14 ` [PATCH RFC 1/3] " holger
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: holger @ 2013-03-19 14:14 UTC (permalink / raw)
To: netfilter-devel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout 2013-03-19 14:14 [PATCH RFC 0/3] NFQUEUE: introduce CPU fanout holger @ 2013-03-19 14:14 ` holger 2013-03-19 14:26 ` David Miller 2013-03-19 19:56 ` Florian Westphal 2013-03-19 14:14 ` [PATCH RFC 2/3] NFQUEUE: coalesce IPv4 and IPv6 hashing holger 2013-03-19 14:14 ` [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter holger 2 siblings, 2 replies; 19+ messages in thread From: holger @ 2013-03-19 14:14 UTC (permalink / raw) To: netfilter-devel [-- 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) ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout 2013-03-19 14:14 ` [PATCH RFC 1/3] " holger @ 2013-03-19 14:26 ` David Miller 2013-03-19 14:34 ` Jan Engelhardt 2013-03-19 19:56 ` Florian Westphal 1 sibling, 1 reply; 19+ messages in thread From: David Miller @ 2013-03-19 14:26 UTC (permalink / raw) To: holger; +Cc: netfilter-devel 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; } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout 2013-03-19 14:26 ` David Miller @ 2013-03-19 14:34 ` Jan Engelhardt 2013-03-19 14:37 ` David Miller 2013-03-19 21:34 ` Holger Eitzenberger 0 siblings, 2 replies; 19+ messages in thread From: Jan Engelhardt @ 2013-03-19 14:34 UTC (permalink / raw) To: holger; +Cc: David Miller, Netfilter Developer Mailing List 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); +} ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout 2013-03-19 14:34 ` Jan Engelhardt @ 2013-03-19 14:37 ` David Miller 2013-03-19 21:38 ` Holger Eitzenberger 2013-03-19 21:34 ` Holger Eitzenberger 1 sibling, 1 reply; 19+ messages in thread From: David Miller @ 2013-03-19 14:37 UTC (permalink / raw) To: jengelh; +Cc: holger, netfilter-devel 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout 2013-03-19 14:37 ` David Miller @ 2013-03-19 21:38 ` Holger Eitzenberger 0 siblings, 0 replies; 19+ messages in thread From: Holger Eitzenberger @ 2013-03-19 21:38 UTC (permalink / raw) To: David Miller; +Cc: netfilter-devel 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout 2013-03-19 14:34 ` Jan Engelhardt 2013-03-19 14:37 ` David Miller @ 2013-03-19 21:34 ` Holger Eitzenberger 2013-03-19 21:57 ` Jan Engelhardt 1 sibling, 1 reply; 19+ messages in thread From: Holger Eitzenberger @ 2013-03-19 21:34 UTC (permalink / raw) To: Jan Engelhardt; +Cc: netfilter-devel 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout 2013-03-19 21:34 ` Holger Eitzenberger @ 2013-03-19 21:57 ` Jan Engelhardt 2013-03-19 22:30 ` Holger Eitzenberger 0 siblings, 1 reply; 19+ messages in thread From: Jan Engelhardt @ 2013-03-19 21:57 UTC (permalink / raw) To: Holger Eitzenberger; +Cc: netfilter-devel 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout 2013-03-19 21:57 ` Jan Engelhardt @ 2013-03-19 22:30 ` Holger Eitzenberger 0 siblings, 0 replies; 19+ messages in thread From: Holger Eitzenberger @ 2013-03-19 22:30 UTC (permalink / raw) To: Jan Engelhardt; +Cc: netfilter-devel > >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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout 2013-03-19 14:14 ` [PATCH RFC 1/3] " holger 2013-03-19 14:26 ` David Miller @ 2013-03-19 19:56 ` Florian Westphal 2013-03-19 20:17 ` Holger Eitzenberger 1 sibling, 1 reply; 19+ messages in thread From: Florian Westphal @ 2013-03-19 19:56 UTC (permalink / raw) To: holger; +Cc: netfilter-devel 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'. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 1/3] NFQUEUE: introduce CPU fanout 2013-03-19 19:56 ` Florian Westphal @ 2013-03-19 20:17 ` Holger Eitzenberger 0 siblings, 0 replies; 19+ messages in thread From: Holger Eitzenberger @ 2013-03-19 20:17 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel > > +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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC 2/3] NFQUEUE: coalesce IPv4 and IPv6 hashing 2013-03-19 14:14 [PATCH RFC 0/3] NFQUEUE: introduce CPU fanout holger 2013-03-19 14:14 ` [PATCH RFC 1/3] " holger @ 2013-03-19 14:14 ` holger 2013-03-19 14:27 ` David Miller 2013-03-19 14:14 ` [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter holger 2 siblings, 1 reply; 19+ messages in thread From: holger @ 2013-03-19 14:14 UTC (permalink / raw) To: netfilter-devel [-- 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); } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 2/3] NFQUEUE: coalesce IPv4 and IPv6 hashing 2013-03-19 14:14 ` [PATCH RFC 2/3] NFQUEUE: coalesce IPv4 and IPv6 hashing holger @ 2013-03-19 14:27 ` David Miller 2013-03-19 14:39 ` Holger Eitzenberger 0 siblings, 1 reply; 19+ messages in thread From: David Miller @ 2013-03-19 14:27 UTC (permalink / raw) To: holger; +Cc: netfilter-devel 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 2/3] NFQUEUE: coalesce IPv4 and IPv6 hashing 2013-03-19 14:27 ` David Miller @ 2013-03-19 14:39 ` Holger Eitzenberger 0 siblings, 0 replies; 19+ messages in thread From: Holger Eitzenberger @ 2013-03-19 14:39 UTC (permalink / raw) To: David Miller; +Cc: netfilter-devel 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! :) ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter 2013-03-19 14:14 [PATCH RFC 0/3] NFQUEUE: introduce CPU fanout holger 2013-03-19 14:14 ` [PATCH RFC 1/3] " holger 2013-03-19 14:14 ` [PATCH RFC 2/3] NFQUEUE: coalesce IPv4 and IPv6 hashing holger @ 2013-03-19 14:14 ` holger 2013-03-19 14:34 ` Eric Leblond 2 siblings, 1 reply; 19+ messages in thread From: holger @ 2013-03-19 14:14 UTC (permalink / raw) To: netfilter-devel [-- 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 */ ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter 2013-03-19 14:14 ` [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter holger @ 2013-03-19 14:34 ` Eric Leblond 2013-03-19 16:07 ` Holger Eitzenberger 2013-03-23 19:52 ` Holger Eitzenberger 0 siblings, 2 replies; 19+ messages in thread From: Eric Leblond @ 2013-03-19 14:34 UTC (permalink / raw) To: holger; +Cc: netfilter-devel 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/ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter 2013-03-19 14:34 ` Eric Leblond @ 2013-03-19 16:07 ` Holger Eitzenberger 2013-03-23 19:52 ` Holger Eitzenberger 1 sibling, 0 replies; 19+ messages in thread From: Holger Eitzenberger @ 2013-03-19 16:07 UTC (permalink / raw) To: Eric Leblond; +Cc: netfilter-devel 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! ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter 2013-03-19 14:34 ` Eric Leblond 2013-03-19 16:07 ` Holger Eitzenberger @ 2013-03-23 19:52 ` Holger Eitzenberger 2013-03-23 21:53 ` Eric Leblond 1 sibling, 1 reply; 19+ messages in thread From: Holger Eitzenberger @ 2013-03-23 19:52 UTC (permalink / raw) To: Eric Leblond; +Cc: netfilter-devel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter 2013-03-23 19:52 ` Holger Eitzenberger @ 2013-03-23 21:53 ` Eric Leblond 0 siblings, 0 replies; 19+ messages in thread From: Eric Leblond @ 2013-03-23 21:53 UTC (permalink / raw) To: Holger Eitzenberger; +Cc: netfilter-devel 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/ ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2013-03-23 21:53 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-03-19 14:14 [PATCH RFC 0/3] NFQUEUE: introduce CPU fanout holger 2013-03-19 14:14 ` [PATCH RFC 1/3] " holger 2013-03-19 14:26 ` David Miller 2013-03-19 14:34 ` Jan Engelhardt 2013-03-19 14:37 ` David Miller 2013-03-19 21:38 ` Holger Eitzenberger 2013-03-19 21:34 ` Holger Eitzenberger 2013-03-19 21:57 ` Jan Engelhardt 2013-03-19 22:30 ` Holger Eitzenberger 2013-03-19 19:56 ` Florian Westphal 2013-03-19 20:17 ` Holger Eitzenberger 2013-03-19 14:14 ` [PATCH RFC 2/3] NFQUEUE: coalesce IPv4 and IPv6 hashing holger 2013-03-19 14:27 ` David Miller 2013-03-19 14:39 ` Holger Eitzenberger 2013-03-19 14:14 ` [PATCH RFC 3/3] NFQUEUE: add --queue-cpu-fanout parameter holger 2013-03-19 14:34 ` Eric Leblond 2013-03-19 16:07 ` Holger Eitzenberger 2013-03-23 19:52 ` Holger Eitzenberger 2013-03-23 21:53 ` Eric Leblond
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.