Patchwork [RFC,1/3] NFQUEUE: introduce CPU fanout

login
register
mail settings
Submitter holger@eitzenberger.org
Date March 19, 2013, 2:14 p.m.
Message ID <20130319141605.158831637@eitzenberger.org>
Download mbox | patch
Permalink /patch/229079/
State RFC
Headers show

Comments

holger@eitzenberger.org - March 19, 2013, 2:14 p.m.
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(-)


--
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
David Miller - March 19, 2013, 2:26 p.m.
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;
}
--
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
Jan Engelhardt - March 19, 2013, 2:34 p.m.
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);
+}
--
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
David Miller - March 19, 2013, 2:37 p.m.
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.
--
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
Florian Westphal - March 19, 2013, 7:56 p.m.
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'.
--
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
holger@eitzenberger.org - March 19, 2013, 8:17 p.m.
> > +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.

--
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
holger@eitzenberger.org - March 19, 2013, 9:34 p.m.
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.

--
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
holger@eitzenberger.org - March 19, 2013, 9:38 p.m.
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.
--
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
Jan Engelhardt - March 19, 2013, 9:57 p.m.
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.
--
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
holger@eitzenberger.org - March 19, 2013, 10:30 p.m.
> >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.
--
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

Patch

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)