[patch 0/6] netfilter: ctnetlink allocation improvement

[patch 0/6] netfilter: ctnetlink allocation improvement

[Holger Eitzenberger]

Hi,

what follows is a small patchset against net-next-2.6 which tries to
improve the way ctnetlink events are allocated.  By allocating the
ctnetlink skbs roughly the size of the message we prevent the skb from
later being reallocated in netlink_trim().

Though I haven't got any hard performance numbers yet I think this
might introduce a noticable performance gain.

The overall idea of these patches is to compute the proto independant
attribute sizes at compile time, the proto-dependant parts are
computed at registration of the actual proto helpers.  This is
achieved by introducing nla_policy_len(), which computes the max.
length of a nla_policy.

I also have to introduce NF_CT_HELPER_NAME_LEN as an upper limit for
the conntrack protcol helper names, which is not much of a problem
because all names in mainline are actually shorter than that.

It would be great to have that being merged.  Please share youre
comments.

Thanks.

  /holger

[patch 1/6] ctnetlink: add callbacks to the per-proto nlattrs

[Holger Eitzenberger]

[-- Attachment #1: ctnetlink-add-infrastructure-to-the-per-proto-nlatt.diff --]
[-- Type: text/plain, Size: 4124 bytes --]

There is added a single callback for the l3 proto helper.  The two
callbacks for the l4 protos are necessary because of the general
structure of a ctnetlink event, which is in short:

 CTA_TUPLE_ORIG
   <l3/l4-proto-attributes>
 CTA_TUPLE_REPLY
   <l3/l4-proto-attributes>
 CTA_ID
 ...
 CTA_PROTOINFO
   <l4-proto-attributes>
 CTA_TUPLE_MASTER
   <l3/l4-proto-attributes>

Therefore the formular is

 size := sizeof(generic-nlas) + 3 * sizeof(tuple_nlas) + sizeof(protoinfo_nlas)

Some of the NLAs are optional, e. g. CTA_TUPLE_MASTER, which is only
set if it's an expected connection.  But the number of optional NLAs is
small enough to prevent netlink_trim() from reallocating if calculated
properly.

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

Index: net-next-2.6/include/net/netfilter/nf_conntrack_l3proto.h
===================================================================
--- net-next-2.6.orig/include/net/netfilter/nf_conntrack_l3proto.h
+++ net-next-2.6/include/net/netfilter/nf_conntrack_l3proto.h
@@ -53,10 +53,17 @@ struct nf_conntrack_l3proto
 	int (*tuple_to_nlattr)(struct sk_buff *skb,
 			       const struct nf_conntrack_tuple *t);
 
+	/*
+	 * Calculate size of tuple nlattr
+	 */
+	int (*nlattr_tuple_size)(void);
+
 	int (*nlattr_to_tuple)(struct nlattr *tb[],
 			       struct nf_conntrack_tuple *t);
 	const struct nla_policy *nla_policy;
 
+	size_t nla_size;
+
 #ifdef CONFIG_SYSCTL
 	struct ctl_table_header	*ctl_table_header;
 	struct ctl_path		*ctl_table_path;
Index: net-next-2.6/include/net/netfilter/nf_conntrack_l4proto.h
===================================================================
--- net-next-2.6.orig/include/net/netfilter/nf_conntrack_l4proto.h
+++ net-next-2.6/include/net/netfilter/nf_conntrack_l4proto.h
@@ -64,16 +64,22 @@ struct nf_conntrack_l4proto
 	/* convert protoinfo to nfnetink attributes */
 	int (*to_nlattr)(struct sk_buff *skb, struct nlattr *nla,
 			 const struct nf_conn *ct);
+	/* Calculate protoinfo nlattr size */
+	int (*nlattr_size)(void);
 
 	/* convert nfnetlink attributes to protoinfo */
 	int (*from_nlattr)(struct nlattr *tb[], struct nf_conn *ct);
 
 	int (*tuple_to_nlattr)(struct sk_buff *skb,
 			       const struct nf_conntrack_tuple *t);
+	/* Calculate tuple nlattr size */
+	int (*nlattr_tuple_size)(void);
 	int (*nlattr_to_tuple)(struct nlattr *tb[],
 			       struct nf_conntrack_tuple *t);
 	const struct nla_policy *nla_policy;
 
+	size_t nla_size;
+
 #ifdef CONFIG_SYSCTL
 	struct ctl_table_header	**ctl_table_header;
 	struct ctl_table	*ctl_table;
Index: net-next-2.6/net/netfilter/nf_conntrack_proto.c
===================================================================
--- net-next-2.6.orig/net/netfilter/nf_conntrack_proto.c
+++ net-next-2.6/net/netfilter/nf_conntrack_proto.c
@@ -188,6 +188,9 @@ int nf_conntrack_l3proto_register(struct
 	if (proto->l3proto >= AF_MAX)
 		return -EBUSY;
 
+	if (proto->tuple_to_nlattr && !proto->nlattr_tuple_size)
+		return -EINVAL;
+
 	mutex_lock(&nf_ct_proto_mutex);
 	if (nf_ct_l3protos[proto->l3proto] != &nf_conntrack_l3proto_generic) {
 		ret = -EBUSY;
@@ -198,6 +201,9 @@ int nf_conntrack_l3proto_register(struct
 	if (ret < 0)
 		goto out_unlock;
 
+	if (proto->nlattr_tuple_size)
+		proto->nla_size = 3 * proto->nlattr_tuple_size();
+
 	rcu_assign_pointer(nf_ct_l3protos[proto->l3proto], proto);
 
 out_unlock:
@@ -284,6 +290,10 @@ int nf_conntrack_l4proto_register(struct
 	if (l4proto->l3proto >= PF_MAX)
 		return -EBUSY;
 
+	if ((l4proto->to_nlattr && !l4proto->nlattr_size)
+		|| (l4proto->tuple_to_nlattr && !l4proto->nlattr_tuple_size))
+		return -EINVAL;
+
 	mutex_lock(&nf_ct_proto_mutex);
 	if (!nf_ct_protos[l4proto->l3proto]) {
 		/* l3proto may be loaded latter. */
@@ -311,6 +321,12 @@ int nf_conntrack_l4proto_register(struct
 	if (ret < 0)
 		goto out_unlock;
 
+	l4proto->nla_size = 0;
+	if (l4proto->nlattr_size)
+		l4proto->nla_size += l4proto->nlattr_size();
+	if (l4proto->nlattr_tuple_size)
+		l4proto->nla_size += 3 * l4proto->nlattr_tuple_size();
+
 	rcu_assign_pointer(nf_ct_protos[l4proto->l3proto][l4proto->l4proto],
 			   l4proto);
 

-- 

Re: [patch 1/6] ctnetlink: add callbacks to the per-proto nlattrs

[Patrick McHardy]

Holger Eitzenberger wrote:
> There is added a single callback for the l3 proto helper.  The two
> callbacks for the l4 protos are necessary because of the general
> structure of a ctnetlink event, which is in short:
> 
>  CTA_TUPLE_ORIG
>    <l3/l4-proto-attributes>
>  CTA_TUPLE_REPLY
>    <l3/l4-proto-attributes>
>  CTA_ID
>  ...
>  CTA_PROTOINFO
>    <l4-proto-attributes>
>  CTA_TUPLE_MASTER
>    <l3/l4-proto-attributes>
> 
> Therefore the formular is
> 
>  size := sizeof(generic-nlas) + 3 * sizeof(tuple_nlas) + sizeof(protoinfo_nlas)
> 
> Some of the NLAs are optional, e. g. CTA_TUPLE_MASTER, which is only
> set if it's an expected connection.  But the number of optional NLAs is
> small enough to prevent netlink_trim() from reallocating if calculated
> properly.

Applied, thanks.

[patch 2/6] netlink: add nla_policy_len()

[Holger Eitzenberger]

[-- Attachment #1: netlink-add-nla_policy_len.diff --]
[-- Type: text/plain, Size: 2023 bytes --]

It calculates the max. length of a Netlink policy, which is usefull
for allocating Netlink buffers roughly the size of the actual
message.

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

Index: net-next-2.6/include/net/netlink.h
===================================================================
--- net-next-2.6.orig/include/net/netlink.h
+++ net-next-2.6/include/net/netlink.h
@@ -230,6 +230,7 @@ extern int		nla_validate(struct nlattr *
 extern int		nla_parse(struct nlattr *tb[], int maxtype,
 				  struct nlattr *head, int len,
 				  const struct nla_policy *policy);
+extern int		nla_policy_len(const struct nla_policy *, int);
 extern struct nlattr *	nla_find(struct nlattr *head, int len, int attrtype);
 extern size_t		nla_strlcpy(char *dst, const struct nlattr *nla,
 				    size_t dstsize);
Index: net-next-2.6/net/netlink/attr.c
===================================================================
--- net-next-2.6.orig/net/netlink/attr.c
+++ net-next-2.6/net/netlink/attr.c
@@ -133,6 +133,32 @@ errout:
 }
 
 /**
+ * nla_policy_len - Determin the max. length of a policy
+ * @policy: policy to use
+ * @n: number of policies
+ *
+ * Determines the max. length of the policy.  It is currently used
+ * to allocated Netlink buffers roughly the size of the actual
+ * message.
+ *
+ * Returns 0 on success or a negative error code.
+ */
+int
+nla_policy_len(const struct nla_policy *p, int n)
+{
+	int i, len = 0;
+
+	for (i = 0; i < n; i++) {
+		if (p->len)
+			len += nla_total_size(p->len);
+		else if (nla_attr_minlen[p->type])
+			len += nla_total_size(nla_attr_minlen[p->type]);
+	}
+
+	return len;
+}
+
+/**
  * nla_parse - Parse a stream of attributes into a tb buffer
  * @tb: destination array with maxtype+1 elements
  * @maxtype: maximum attribute type to be expected
@@ -456,6 +482,7 @@ int nla_append(struct sk_buff *skb, int 
 }
 
 EXPORT_SYMBOL(nla_validate);
+EXPORT_SYMBOL(nla_policy_len);
 EXPORT_SYMBOL(nla_parse);
 EXPORT_SYMBOL(nla_find);
 EXPORT_SYMBOL(nla_strlcpy);

-- 

Re: [patch 2/6] netlink: add nla_policy_len()

[Patrick McHardy]

Holger Eitzenberger wrote:
> It calculates the max. length of a Netlink policy, which is usefull
> for allocating Netlink buffers roughly the size of the actual
> message.

Applied, thanks.

[patch 3/6] netfilter: limit the length of the helper name

[Holger Eitzenberger]

[-- Attachment #1: netfilter-limit-the-length-of-the-helper-name.diff --]
[-- Type: text/plain, Size: 1183 bytes --]

This is necessary in order to have an upper bound for Netlink
message calculation, which is not a problem at all, as there
are no helpers with a longer name.

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

Index: net-next-2.6/include/net/netfilter/nf_conntrack_helper.h
===================================================================
--- net-next-2.6.orig/include/net/netfilter/nf_conntrack_helper.h
+++ net-next-2.6/include/net/netfilter/nf_conntrack_helper.h
@@ -14,6 +14,8 @@
 
 struct module;
 
+#define NF_CT_HELPER_NAME_LEN	16
+
 struct nf_conntrack_helper
 {
 	struct hlist_node hnode;	/* Internal use. */
Index: net-next-2.6/net/netfilter/nf_conntrack_helper.c
===================================================================
--- net-next-2.6.orig/net/netfilter/nf_conntrack_helper.c
+++ net-next-2.6/net/netfilter/nf_conntrack_helper.c
@@ -142,6 +142,7 @@ int nf_conntrack_helper_register(struct 
 
 	BUG_ON(me->expect_policy == NULL);
 	BUG_ON(me->expect_class_max >= NF_CT_MAX_EXPECT_CLASSES);
+	BUG_ON(strlen(me->name) >= NF_CT_HELPER_NAME_LEN - 1);
 
 	mutex_lock(&nf_ct_helper_mutex);
 	hlist_add_head_rcu(&me->hnode, &nf_ct_helper_hash[h]);

-- 

Re: [patch 3/6] netfilter: limit the length of the helper name

[Patrick McHardy]

Holger Eitzenberger wrote:
> This is necessary in order to have an upper bound for Netlink
> message calculation, which is not a problem at all, as there
> are no helpers with a longer name.
> 
> Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>
> 
> Index: net-next-2.6/include/net/netfilter/nf_conntrack_helper.h
> ===================================================================
> --- net-next-2.6.orig/include/net/netfilter/nf_conntrack_helper.h
> +++ net-next-2.6/include/net/netfilter/nf_conntrack_helper.h
> @@ -14,6 +14,8 @@
>  
>  struct module;
>  
> +#define NF_CT_HELPER_NAME_LEN	16
> +
>  struct nf_conntrack_helper
>  {
>  	struct hlist_node hnode;	/* Internal use. */
> Index: net-next-2.6/net/netfilter/nf_conntrack_helper.c
> ===================================================================
> --- net-next-2.6.orig/net/netfilter/nf_conntrack_helper.c
> +++ net-next-2.6/net/netfilter/nf_conntrack_helper.c
> @@ -142,6 +142,7 @@ int nf_conntrack_helper_register(struct 
>  
>  	BUG_ON(me->expect_policy == NULL);
>  	BUG_ON(me->expect_class_max >= NF_CT_MAX_EXPECT_CLASSES);
> +	BUG_ON(strlen(me->name) >= NF_CT_HELPER_NAME_LEN - 1);

This appears to be an off-by-one. A strlen of exactly
NF_CT_HELPER_NAME_LEN - 1 would be fine, right?

No need to resend, just let me know whether I should change it.

Re: [patch 3/6] netfilter: limit the length of the helper name

[Holger Eitzenberger]

On Wed, Mar 25, 2009 at 06:32:26PM +0100, Patrick McHardy wrote:

> This appears to be an off-by-one. A strlen of exactly
> NF_CT_HELPER_NAME_LEN - 1 would be fine, right?
> 
> No need to resend, just let me know whether I should change it.

Yes please, feel free to change it :).

 /holger

Re: [patch 3/6] netfilter: limit the length of the helper name

[Patrick McHardy]

Holger Eitzenberger wrote:
> On Wed, Mar 25, 2009 at 06:32:26PM +0100, Patrick McHardy wrote:
> 
>> This appears to be an off-by-one. A strlen of exactly
>> NF_CT_HELPER_NAME_LEN - 1 would be fine, right?
>>
>> No need to resend, just let me know whether I should change it.
> 
> Yes please, feel free to change it :).

Done and applied, thanks.

[patch 4/6] ctnetlink: allocate right-sized ctnetlink skb

[Holger Eitzenberger]

[-- Attachment #1: ctnetlink-factor-out-alloc_skb-into-ctnetlink_all.diff --]
[-- Type: text/plain, Size: 3279 bytes --]

Try to allocate a Netlink skb roughly the size of the actual
message, with the help from the l3 and l4 protocol helpers.
This is all to prevent a reallocation in netlink_trim() later.

The overhead of allocating the right-sized skb is rather small, with
ctnetlink_alloc_skb() actually being inlined away on my x86_64 box.
The size of the per-proto space is determined at registration time of
the protocol helper.

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

Index: net-next-2.6/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-2.6.orig/net/netfilter/nf_conntrack_netlink.c
+++ net-next-2.6/net/netfilter/nf_conntrack_netlink.c
@@ -404,6 +404,69 @@ nla_put_failure:
 }
 
 #ifdef CONFIG_NF_CONNTRACK_EVENTS
+/*
+ * The general structure of a ctnetlink event is
+ *
+ *  CTA_TUPLE_ORIG
+ *    <l3/l4-proto-attributes>
+ *  CTA_TUPLE_REPLY
+ *    <l3/l4-proto-attributes>
+ *  CTA_ID
+ *  ...
+ *  CTA_PROTOINFO
+ *    <l4-proto-attributes>
+ *  CTA_TUPLE_MASTER
+ *    <l3/l4-proto-attributes>
+ *
+ * Therefore the formular is
+ *
+ *   size = sizeof(generic_nlas) + 3 * sizeof(tuple_nlas)
+ *		+ sizeof(protoinfo_nlas)
+ */
+static struct sk_buff *
+ctnetlink_alloc_skb(const struct nf_conntrack_tuple *tuple, gfp_t gfp)
+{
+	struct nf_conntrack_l3proto *l3proto;
+	struct nf_conntrack_l4proto *l4proto;
+	int len;
+
+#define NLA_TYPE_SIZE(type)		nla_total_size(sizeof(type))
+
+	/* proto independant part */
+	len = NLMSG_SPACE(sizeof(struct nfgenmsg))
+		+ 3 * nla_total_size(0)		/* CTA_TUPLE_ORIG|REPL|MASTER */
+		+ 3 * nla_total_size(0)		/* CTA_TUPLE_IP */
+		+ 3 * nla_total_size(0)		/* CTA_TUPLE_PROTO */
+		+ 3 * NLA_TYPE_SIZE(u_int8_t)	/* CTA_PROTO_NUM */
+		+ NLA_TYPE_SIZE(u_int32_t)	/* CTA_ID */
+		+ NLA_TYPE_SIZE(u_int32_t)	/* CTA_STATUS */
+		+ 2 * nla_total_size(0)		/* CTA_COUNTERS_ORIG|REPL */
+		+ 2 * NLA_TYPE_SIZE(uint64_t)	/* CTA_COUNTERS_PACKETS */
+		+ 2 * NLA_TYPE_SIZE(uint64_t)	/* CTA_COUNTERS_BYTES */
+		+ NLA_TYPE_SIZE(u_int32_t)	/* CTA_TIMEOUT */
+		+ nla_total_size(0)		/* CTA_PROTOINFO */
+		+ nla_total_size(0)		/* CTA_HELP */
+		+ nla_total_size(NF_CT_HELPER_NAME_LEN)	/* CTA_HELP_NAME */
+		+ NLA_TYPE_SIZE(u_int32_t)	/* CTA_SECMARK */
+		+ 2 * nla_total_size(0)		/* CTA_NAT_SEQ_ADJ_ORIG|REPL */
+		+ 2 * NLA_TYPE_SIZE(u_int32_t)	/* CTA_NAT_SEQ_CORRECTION_POS */
+		+ 2 * NLA_TYPE_SIZE(u_int32_t)	/* CTA_NAT_SEQ_CORRECTION_BEFORE */
+		+ 2 * NLA_TYPE_SIZE(u_int32_t)	/* CTA_NAT_SEQ_CORRECTION_AFTER */
+		+ NLA_TYPE_SIZE(u_int32_t);	/* CTA_MARK */
+
+#undef NLA_TYPE_SIZE
+
+	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
+	len += l3proto->nla_size;
+	nf_ct_l3proto_put(l3proto);
+
+	l4proto = nf_ct_l4proto_find_get(tuple->src.l3num, tuple->dst.protonum);
+	len += l4proto->nla_size;
+	nf_ct_l4proto_put(l4proto);
+
+	return alloc_skb(len, gfp);
+}
+
 static int ctnetlink_conntrack_event(struct notifier_block *this,
 				     unsigned long events, void *ptr)
 {
@@ -437,7 +500,7 @@ static int ctnetlink_conntrack_event(str
 	if (!nfnetlink_has_listeners(group))
 		return NOTIFY_DONE;
 
-	skb = alloc_skb(NLMSG_GOODSIZE, GFP_ATOMIC);
+	skb = ctnetlink_alloc_skb(tuple(ct, IP_CT_DIR_ORIGINAL), GFP_ATOMIC);
 	if (!skb)
 		return NOTIFY_DONE;
 

-- 

Re: [patch 4/6] ctnetlink: allocate right-sized ctnetlink skb

[Patrick McHardy]

Holger Eitzenberger wrote:
> Try to allocate a Netlink skb roughly the size of the actual
> message, with the help from the l3 and l4 protocol helpers.
> This is all to prevent a reallocation in netlink_trim() later.
> 
> The overhead of allocating the right-sized skb is rather small, with
> ctnetlink_alloc_skb() actually being inlined away on my x86_64 box.
> The size of the per-proto space is determined at registration time of
> the protocol helper.

This one clashes with Florian's changes to get rid of
nf_ct_l4proto_find_get(). I've pushed out the patches I've
already applied, please rediff the remaining ones against
my current tree. Thanks.

[patch 5/6] netfilter: add generic function to get len of generic policy

[Holger Eitzenberger]

[-- Attachment #1: netfilter-add-generic-fn-for-generic-policy.diff --]
[-- Type: text/plain, Size: 1330 bytes --]

Usefull for all protocols which do not add additional data, such
as GRE or UDPlite.

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

Index: net-next-2.6/include/net/netfilter/nf_conntrack_l4proto.h
===================================================================
--- net-next-2.6.orig/include/net/netfilter/nf_conntrack_l4proto.h
+++ net-next-2.6/include/net/netfilter/nf_conntrack_l4proto.h
@@ -121,6 +121,7 @@ extern int nf_ct_port_tuple_to_nlattr(st
 				      const struct nf_conntrack_tuple *tuple);
 extern int nf_ct_port_nlattr_to_tuple(struct nlattr *tb[],
 				      struct nf_conntrack_tuple *t);
+extern int nf_ct_port_nlattr_tuple_size(void);
 extern const struct nla_policy nf_ct_port_nla_policy[];
 
 #ifdef CONFIG_SYSCTL
Index: net-next-2.6/net/netfilter/nf_conntrack_core.c
===================================================================
--- net-next-2.6.orig/net/netfilter/nf_conntrack_core.c
+++ net-next-2.6/net/netfilter/nf_conntrack_core.c
@@ -902,6 +902,12 @@ int nf_ct_port_nlattr_to_tuple(struct nl
 	return 0;
 }
 EXPORT_SYMBOL_GPL(nf_ct_port_nlattr_to_tuple);
+
+int nf_ct_port_nlattr_tuple_size(void)
+{
+	return nla_policy_len(nf_ct_port_nla_policy, CTA_PROTO_MAX + 1);
+}
+EXPORT_SYMBOL_GPL(nf_ct_port_nlattr_tuple_size);
 #endif
 
 /* Used by ipt_REJECT and ip6t_REJECT. */

-- 

[patch 6/6] netfilter: calculate per-protocol nlattr size

[Holger Eitzenberger]

[-- Attachment #1: netfilter-calculate-per-protocol-nlattr-size.diff --]
[-- Type: text/plain, Size: 10021 bytes --]

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

Index: net-next-2.6/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
===================================================================
--- net-next-2.6.orig/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ net-next-2.6/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -326,6 +326,11 @@ static int ipv4_nlattr_to_tuple(struct n
 
 	return 0;
 }
+
+static int ipv4_nlattr_tuple_size(void)
+{
+	return nla_policy_len(ipv4_nla_policy, CTA_IP_MAX + 1);
+}
 #endif
 
 static struct nf_sockopt_ops so_getorigdst = {
@@ -345,6 +350,7 @@ struct nf_conntrack_l3proto nf_conntrack
 	.get_l4proto	 = ipv4_get_l4proto,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr = ipv4_tuple_to_nlattr,
+	.nlattr_tuple_size = ipv4_nlattr_tuple_size,
 	.nlattr_to_tuple = ipv4_nlattr_to_tuple,
 	.nla_policy	 = ipv4_nla_policy,
 #endif
Index: net-next-2.6/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
===================================================================
--- net-next-2.6.orig/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
+++ net-next-2.6/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
@@ -262,6 +262,11 @@ static int icmp_nlattr_to_tuple(struct n
 
 	return 0;
 }
+
+static int icmp_nlattr_tuple_size(void)
+{
+	return nla_policy_len(icmp_nla_policy, CTA_PROTO_MAX + 1);
+}
 #endif
 
 #ifdef CONFIG_SYSCTL
@@ -309,6 +314,7 @@ struct nf_conntrack_l4proto nf_conntrack
 	.me			= NULL,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= icmp_tuple_to_nlattr,
+	.nlattr_tuple_size	= icmp_nlattr_tuple_size,
 	.nlattr_to_tuple	= icmp_nlattr_to_tuple,
 	.nla_policy		= icmp_nla_policy,
 #endif
Index: net-next-2.6/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
===================================================================
--- net-next-2.6.orig/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
+++ net-next-2.6/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
@@ -341,6 +341,11 @@ static int ipv6_nlattr_to_tuple(struct n
 
 	return 0;
 }
+
+static int ipv6_nlattr_tuple_size(void)
+{
+	return nla_policy_len(ipv6_nla_policy, CTA_IP_MAX + 1);
+}
 #endif
 
 struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv6 __read_mostly = {
@@ -352,6 +357,7 @@ struct nf_conntrack_l3proto nf_conntrack
 	.get_l4proto		= ipv6_get_l4proto,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= ipv6_tuple_to_nlattr,
+	.nlattr_tuple_size	= ipv6_nlattr_tuple_size,
 	.nlattr_to_tuple	= ipv6_nlattr_to_tuple,
 	.nla_policy		= ipv6_nla_policy,
 #endif
Index: net-next-2.6/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
===================================================================
--- net-next-2.6.orig/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
+++ net-next-2.6/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
@@ -243,6 +243,11 @@ static int icmpv6_nlattr_to_tuple(struct
 
 	return 0;
 }
+
+static int icmpv6_nlattr_tuple_size(void)
+{
+	return nla_policy_len(icmpv6_nla_policy, CTA_PROTO_MAX + 1);
+}
 #endif
 
 #ifdef CONFIG_SYSCTL
@@ -274,6 +279,7 @@ struct nf_conntrack_l4proto nf_conntrack
 	.error			= icmpv6_error,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= icmpv6_tuple_to_nlattr,
+	.nlattr_tuple_size	= icmpv6_nlattr_tuple_size,
 	.nlattr_to_tuple	= icmpv6_nlattr_to_tuple,
 	.nla_policy		= icmpv6_nla_policy,
 #endif
Index: net-next-2.6/net/netfilter/nf_conntrack_proto_dccp.c
===================================================================
--- net-next-2.6.orig/net/netfilter/nf_conntrack_proto_dccp.c
+++ net-next-2.6/net/netfilter/nf_conntrack_proto_dccp.c
@@ -657,6 +657,12 @@ static int nlattr_to_dccp(struct nlattr 
 	write_unlock_bh(&dccp_lock);
 	return 0;
 }
+
+static int dccp_nlattr_size(void)
+{
+	return nla_total_size(0)	/* CTA_PROTOINFO_DCCP */
+		+ nla_policy_len(dccp_nla_policy, CTA_PROTOINFO_DCCP_MAX + 1);
+}
 #endif
 
 #ifdef CONFIG_SYSCTL
@@ -746,8 +752,10 @@ static struct nf_conntrack_l4proto dccp_
 	.print_conntrack	= dccp_print_conntrack,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.to_nlattr		= dccp_to_nlattr,
+	.nlattr_size		= dccp_nlattr_size,
 	.from_nlattr		= nlattr_to_dccp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
+	.nlattr_tuple_size	= nf_ct_port_nlattr_tuple_size,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
@@ -773,6 +781,7 @@ static struct nf_conntrack_l4proto dccp_
 	.to_nlattr		= dccp_to_nlattr,
 	.from_nlattr		= nlattr_to_dccp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
+	.nlattr_tuple_size	= nf_ct_port_nlattr_tuple_size,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
Index: net-next-2.6/net/netfilter/nf_conntrack_proto_gre.c
===================================================================
--- net-next-2.6.orig/net/netfilter/nf_conntrack_proto_gre.c
+++ net-next-2.6/net/netfilter/nf_conntrack_proto_gre.c
@@ -293,6 +293,7 @@ static struct nf_conntrack_l4proto nf_co
 	.me 		 = THIS_MODULE,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr = nf_ct_port_tuple_to_nlattr,
+	.nlattr_tuple_size = nf_ct_port_nlattr_tuple_size,
 	.nlattr_to_tuple = nf_ct_port_nlattr_to_tuple,
 	.nla_policy	 = nf_ct_port_nla_policy,
 #endif
Index: net-next-2.6/net/netfilter/nf_conntrack_proto_sctp.c
===================================================================
--- net-next-2.6.orig/net/netfilter/nf_conntrack_proto_sctp.c
+++ net-next-2.6/net/netfilter/nf_conntrack_proto_sctp.c
@@ -537,6 +537,12 @@ static int nlattr_to_sctp(struct nlattr 
 
 	return 0;
 }
+
+static int sctp_nlattr_size(void)
+{
+	return nla_total_size(0)	/* CTA_PROTOINFO_SCTP */
+		+ nla_policy_len(sctp_nla_policy, CTA_PROTOINFO_SCTP_MAX + 1);
+}
 #endif
 
 #ifdef CONFIG_SYSCTL
@@ -668,8 +674,10 @@ static struct nf_conntrack_l4proto nf_co
 	.me 			= THIS_MODULE,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.to_nlattr		= sctp_to_nlattr,
+	.nlattr_size		= sctp_nlattr_size,
 	.from_nlattr		= nlattr_to_sctp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
+	.nlattr_tuple_size	= nf_ct_port_nlattr_tuple_size,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
@@ -696,8 +704,10 @@ static struct nf_conntrack_l4proto nf_co
 	.me 			= THIS_MODULE,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.to_nlattr		= sctp_to_nlattr,
+	.nlattr_size		= sctp_nlattr_size,
 	.from_nlattr		= nlattr_to_sctp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
+	.nlattr_tuple_size	= nf_ct_port_nlattr_tuple_size,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
Index: net-next-2.6/net/netfilter/nf_conntrack_proto_tcp.c
===================================================================
--- net-next-2.6.orig/net/netfilter/nf_conntrack_proto_tcp.c
+++ net-next-2.6/net/netfilter/nf_conntrack_proto_tcp.c
@@ -1181,6 +1181,17 @@ static int nlattr_to_tcp(struct nlattr *
 
 	return 0;
 }
+
+static int tcp_nlattr_size(void)
+{
+	return nla_total_size(0)	   /* CTA_PROTOINFO_TCP */
+		+ nla_policy_len(tcp_nla_policy, CTA_PROTOINFO_TCP_MAX + 1);
+}
+
+static int tcp_nlattr_tuple_size(void)
+{
+	return nla_policy_len(nf_ct_port_nla_policy, CTA_PROTO_MAX + 1);
+}
 #endif
 
 #ifdef CONFIG_SYSCTL
@@ -1396,9 +1407,11 @@ struct nf_conntrack_l4proto nf_conntrack
 	.error			= tcp_error,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.to_nlattr		= tcp_to_nlattr,
+	.nlattr_size		= tcp_nlattr_size,
 	.from_nlattr		= nlattr_to_tcp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_tuple_size	= tcp_nlattr_tuple_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL
@@ -1426,9 +1439,11 @@ struct nf_conntrack_l4proto nf_conntrack
 	.error			= tcp_error,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.to_nlattr		= tcp_to_nlattr,
+	.nlattr_size		= tcp_nlattr_size,
 	.from_nlattr		= nlattr_to_tcp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_tuple_size	= tcp_nlattr_tuple_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL
Index: net-next-2.6/net/netfilter/nf_conntrack_proto_udp.c
===================================================================
--- net-next-2.6.orig/net/netfilter/nf_conntrack_proto_udp.c
+++ net-next-2.6/net/netfilter/nf_conntrack_proto_udp.c
@@ -193,6 +193,7 @@ struct nf_conntrack_l4proto nf_conntrack
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_tuple_size	= nf_ct_port_nlattr_tuple_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL
@@ -220,6 +221,7 @@ struct nf_conntrack_l4proto nf_conntrack
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
+	.nlattr_tuple_size	= nf_ct_port_nlattr_tuple_size,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif
 #ifdef CONFIG_SYSCTL
Index: net-next-2.6/net/netfilter/nf_conntrack_proto_udplite.c
===================================================================
--- net-next-2.6.orig/net/netfilter/nf_conntrack_proto_udplite.c
+++ net-next-2.6/net/netfilter/nf_conntrack_proto_udplite.c
@@ -180,6 +180,7 @@ static struct nf_conntrack_l4proto nf_co
 	.error			= udplite_error,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
+	.nlattr_tuple_size	= nf_ct_port_nlattr_tuple_size,
 	.nlattr_to_tuple	= nf_ct_port_nlattr_to_tuple,
 	.nla_policy		= nf_ct_port_nla_policy,
 #endif

-- 

Re: [patch 0/6] netfilter: ctnetlink allocation improvement

[Patrick McHardy]

Holger Eitzenberger wrote:
> what follows is a small patchset against net-next-2.6 which tries to
> improve the way ctnetlink events are allocated.  By allocating the
> ctnetlink skbs roughly the size of the message we prevent the skb from
> later being reallocated in netlink_trim().
> 
> Though I haven't got any hard performance numbers yet I think this
> might introduce a noticable performance gain.
> 
> The overall idea of these patches is to compute the proto independant
> attribute sizes at compile time, the proto-dependant parts are
> computed at registration of the actual proto helpers.  This is
> achieved by introducing nla_policy_len(), which computes the max.
> length of a nla_policy.
> 
> I also have to introduce NF_CT_HELPER_NAME_LEN as an upper limit for
> the conntrack protcol helper names, which is not much of a problem
> because all names in mainline are actually shorter than that.

The helper name change is fine. We currently have a limit of 30
in the helper match, but that is just as arbitrary and 16 does
seem like enough. In fact I need the same change for nftables :)

> It would be great to have that being merged.  Please share youre
> comments.

These patches look almost perfect, there's just one minor thing
that should be fixed from my perspective (from patch 4):

> +	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
> +	len += l3proto->nla_size;
> +	nf_ct_l3proto_put(l3proto);
> +
> +	l4proto = nf_ct_l4proto_find_get(tuple->src.l3num, tuple->dst.protonum);
> +	len += l4proto->nla_size;
> +	nf_ct_l4proto_put(l4proto);
> +
> +	return alloc_skb(len, gfp);

Its preferrable not to use module reference counting during packet
processing, the protocols can be accessed safely using RCU. I thought
I had fixed all those areas, but I now notice that ctnetlink is full
of similar spots and takes and drops module references quite
excessively. So just leave it as it is I guess, this should be fully
fixed anyways.

I'll wait a few hours for others to comment before applying your
patches.

Re: [patch 0/6] netfilter: ctnetlink allocation improvement

[Holger Eitzenberger]

On Tue, Mar 17, 2009 at 05:35:47AM +0100, Patrick McHardy wrote:

> Its preferrable not to use module reference counting during packet
> processing, the protocols can be accessed safely using RCU. I thought
> I had fixed all those areas, but I now notice that ctnetlink is full
> of similar spots and takes and drops module references quite
> excessively. So just leave it as it is I guess, this should be fully
> fixed anyways.

Thanks Patrick.

Re: [patch 0/6] netfilter: ctnetlink allocation improvement

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
>> +	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
>> +	len += l3proto->nla_size;
>> +	nf_ct_l3proto_put(l3proto);
>> +
>> +	l4proto = nf_ct_l4proto_find_get(tuple->src.l3num, tuple->dst.protonum);
>> +	len += l4proto->nla_size;
>> +	nf_ct_l4proto_put(l4proto);
>> +
>> +	return alloc_skb(len, gfp);
>
> Its preferrable not to use module reference counting during packet
> processing, the protocols can be accessed safely using RCU. I thought
> I had fixed all those areas, but I now notice that ctnetlink is full
> of similar spots and takes and drops module references quite
> excessively. So just leave it as it is I guess, this should be fully
> fixed anyways.

I'll look into converting all those spots to RCU.

Thanks, Florian

Re: [patch 0/6] netfilter: ctnetlink allocation improvement

[Pablo Neira Ayuso]

Hi Patrick,

Patrick McHardy wrote:
> These patches look almost perfect, there's just one minor thing
> that should be fixed from my perspective (from patch 4):
> 
>> +    l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
>> +    len += l3proto->nla_size;
>> +    nf_ct_l3proto_put(l3proto);
>> +
>> +    l4proto = nf_ct_l4proto_find_get(tuple->src.l3num,
>> tuple->dst.protonum);
>> +    len += l4proto->nla_size;
>> +    nf_ct_l4proto_put(l4proto);
>> +
>> +    return alloc_skb(len, gfp);
> 
> Its preferrable not to use module reference counting during packet
> processing, the protocols can be accessed safely using RCU. I thought
> I had fixed all those areas, but I now notice that ctnetlink is full
> of similar spots and takes and drops module references quite
> excessively. So just leave it as it is I guess, this should be fully
> fixed anyways.

Indeed, there are still a couple of module reference spots. We only
remove them from the event delivery path.

> I'll wait a few hours for others to comment before applying your
> patches.

Hm, it seems that our patches follows two different principles. AFAICS,
Holger calculates an approximate message size to avoid trimming, while I
calculate the exact message size.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [patch 0/6] netfilter: ctnetlink allocation improvement

[Pablo Neira Ayuso]

Hi Holger,

Holger Eitzenberger wrote:
> what follows is a small patchset against net-next-2.6 which tries to
> improve the way ctnetlink events are allocated.  By allocating the
> ctnetlink skbs roughly the size of the message we prevent the skb from
> later being reallocated in netlink_trim().

I have a similar patch here that I sent time ago, I'll send it again, I
think that we can merge your patches and mine.

> Though I haven't got any hard performance numbers yet I think this
> might introduce a noticable performance gain.

I'm not sure about this. Instead, I noticed with oprofile that we spend
more cycles to calculate the message size. I think it's more like a
trade-off, reducing the message size to what we need reduces the chances
to hit ENOBUFS.

> I also have to introduce NF_CT_HELPER_NAME_LEN as an upper limit for
> the conntrack protcol helper names, which is not much of a problem
> because all names in mainline are actually shorter than that.

I'm fine with the helper size change. Actually the current size of 30
bytes doesn't make too much sense.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [patch 0/6] netfilter: ctnetlink allocation improvement

[Holger Eitzenberger]

On Tue, Mar 17, 2009 at 10:49:00AM +0100, Pablo Neira Ayuso wrote:

> > Though I haven't got any hard performance numbers yet I think this
> > might introduce a noticable performance gain.
> 
> I'm not sure about this. Instead, I noticed with oprofile that we spend
> more cycles to calculate the message size. I think it's more like a
> trade-off, reducing the message size to what we need reduces the chances
> to hit ENOBUFS.

Hi Pablo,

your patch is a bit different, as you do the message length
calculation on each packet.  Instead I do it once at compile time for
the proto independant part and then later for each protocol helper at
the time it is registered.  At unload time the helper is replaced by
the generic one (which implies =0 always).

Cheers,

 /holger