netlink: Increase netlink dump skb message size

RFC PATCH In review Greg Rose 5 participants 18 messages Apr 25, 2011 – Apr 29, 2011

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Greg Rose

author

2011-04-25 22:01 UTC #0 lore raw

RFC PATCH 1/1 netlink: Increase netlink dump skb message size

The message size allocated for rtnl info dumps was limited to a single page. This is not enough for additional interface info available with devices that support SR-IOV. Check that the amount of data allocated is sufficient for the amount of data requested.

Signed-off-by Greg Rose <[email protected]>

---

 include/linux/rtnetlink.h |    1 +
 net/core/rtnetlink.c      |    6 ++++++
 net/netlink/af_netlink.c  |   37 +++++++++++++++++++++++++++++++------
 3 files changed, 38 insertions(+), 6 deletions(-)

include/linux/rtnetlink.h +1 -0

index bbad657..d1ff937 100644
@@ -622,6 +622,7 @@ extern int rtnetlink_put_metrics(struct sk_buff *skb, u32 *metrics);
 extern int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst,
 			      u32 id, u32 ts, u32 tsage, long expires,
 			      u32 error);
+extern size_t rtnl_get_nlmsg_size(const struct net_device *dev);
 
 extern void __rta_fill(struct sk_buff *skb, int attrtype, int attrlen, const void *data);

net/core/rtnetlink.c +6 -0

index d7c4bb4..001c947 100644
@@ -764,6 +764,12 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev)
 	       + rtnl_link_get_af_size(dev); /* IFLA_AF_SPEC */
 }
 
+size_t rtnl_get_nlmsg_size(const struct net_device *dev)
+{
+	return if_nlmsg_size(dev);
+}
+EXPORT_SYMBOL(rtnl_get_nlmsg_size);
+
 static int rtnl_vf_ports_fill(struct sk_buff *skb, struct net_device *dev)
 {
 	struct nlattr *vf_ports;

net/netlink/af_netlink.c +31 -6

index c8f35b5..5b1106c 100644
@@ -1664,23 +1664,48 @@ static void netlink_destroy_callback(struct netlink_callback *cb)
 static int netlink_dump(struct sock *sk)
 {
 	struct netlink_sock *nlk = nlk_sk(sk);
+	struct net *net = sock_net(sk);
 	struct netlink_callback *cb;
 	struct sk_buff *skb;
 	struct nlmsghdr *nlh;
+	struct net_device *dev;
+	struct hlist_head *head;
+	struct hlist_node *node;
 	int len, err = -ENOBUFS;
-
-	skb = sock_rmalloc(sk, NLMSG_GOODSIZE, 0, GFP_KERNEL);
-	if (!skb)
-		goto errout;
+	int h, s_h;
+	int idx = 0, s_idx;
+	size_t alloc_size = NLMSG_GOODSIZE;
 
 	mutex_lock(nlk->cb_mutex);
 
 	cb = nlk->cb;
 	if (cb == NULL) {
 		err = -EINVAL;
-		goto errout_skb;
+		goto errout;
 	}
 
+	s_h = cb->args[0];
+	s_idx = cb->args[1];
+
+	for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) {
+		idx = 0;
+		head = &net->dev_index_head[h];
+		hlist_for_each_entry(dev, node, head, index_hlist) {
+			if (idx < s_idx) {
+				idx++;
+				continue;
+			}
+			alloc_size = rtnl_get_nlmsg_size(dev);
+			if (alloc_size < NLMSG_GOODSIZE)
+				alloc_size = NLMSG_GOODSIZE;
+			break;
+		}
+	}
+
+	skb = sock_rmalloc(sk, alloc_size, 0, GFP_KERNEL);
+	if (!skb)
+		goto errout;
+
 	len = cb->dump(skb, cb);
 
 	if (len > 0) {

@@ -1717,9 +1742,9 @@ static int netlink_dump(struct sock *sk)
 	return 0;
 
 errout_skb:
-	mutex_unlock(nlk->cb_mutex);
 	kfree_skb(skb);
 errout:
+	mutex_unlock(nlk->cb_mutex);
 	return err;
 }

Eric Dumazet

2011-04-26 06:33 UTC #1 lore raw

Quoted 5 lines · Greg Rose

The message size allocated for rtnl info dumps was limited to a single page. This is not enough for additional interface info available with devices that support SR-IOV. Check that the amount of data allocated is sufficient for the amount of data requested.

Quoted 8 lines

Signed-off-by Greg Rose <[email protected]>

---

 include/linux/rtnetlink.h |    1 +
 net/core/rtnetlink.c      |    6 ++++++
 net/netlink/af_netlink.c  |   37 +++++++++++++++++++++++++++++++------
 3 files changed, 38 insertions(+), 6 deletions(-)

Hmm, thats a hack, because netlink_dump() is generic and you add something very specific.

I prefer something that allows one dump() to reallocate a bigger skb

Maybe changing->dump() prototype to struct sk_buff **pskb instead of struct sk_buff *skb.

net/netlink/af_netlink.c +1 -1

index c8f35b5..7fa6735 100644
@@ -1681,7 +1681,7 @@ static int netlink_dump(struct sock *sk)
 		goto errout_skb;
 	}
 
-	len = cb->dump(skb, cb);
+	len = cb->dump(&skb, cb);
 
 	if (len > 0) {
 		mutex_unlock(nlk->cb_mutex);

David Miller

maintainer

2011-04-26 06:56 UTC #2 lore raw

From: Eric Dumazet <[email protected]>
Date: Tue, 26 Apr 2011 08:33:17 +0200

Quoted 9 lines · Greg Rose

Quoted 8 lines
The message size allocated for rtnl info dumps was limited to a single page. This is not enough for additional interface info available with devices that support SR-IOV. Check that the amount of data allocated is sufficient for the amount of data requested.
Signed-off-by Greg Rose <[email protected]>
---

Quoted 8 lines

Quoted 5 lines
 include/linux/rtnetlink.h |    1 +
 net/core/rtnetlink.c      |    6 ++++++
 net/netlink/af_netlink.c  |   37 +++++++++++++++++++++++++++++++------
 3 files changed, 38 insertions(+), 6 deletions(-)
Hmm, thats a hack, because netlink_dump() is generic and you add something very specific.

You also can't do this without breaking applications.

We've trained every single netlink library out there about this message size formula, so they know that if you allocate at least 8192 bytes for a recvmsg() call they can receive fully any single netlink message.

And they must be able to make assumptions like this, otherwise they cannot know how to reliably be able to receive a netlink message in it's entirety in a generic fashion.

So one must not attack this problem from this angle.

It is absolutely necessary to find some way to report the VF information, out of band, instead of trying to make the message larger.

Needing more than 8K to get a dump of a single device over netlink is absolutely rediculious, this VF stuff was poorly designed. If has to be fixed and the current stuff marked deprecated.

Rose, Gregory V

author

2011-04-26 16:12 UTC #3 lore raw

Quoted 30 lines

-----Original Message-----
From: David Miller [mailto:[email protected]]
Sent: Monday, April 25, 2011 11:56 PM
To: [email protected]
Cc: Rose, Gregory V; [email protected]; [email protected]
Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message size
From: Eric Dumazet <[email protected]>
Date: Tue, 26 Apr 2011 08:33:17 +0200
Quoted 2 lines · Greg Rose
Quoted 1 line
The message size allocated for rtnl info dumps was limited to a single
page.
Quoted 2 lines
Quoted 2 lines
This is not enough for additional interface info available with devices that support SR-IOV. Check that the amount of data allocated is
sufficient
Quoted 5 lines
Quoted 5 lines
for the amount of data requested.
Signed-off-by Greg Rose <[email protected]>
---
Quoted 8 lines
Quoted 5 lines
 include/linux/rtnetlink.h |    1 +
 net/core/rtnetlink.c      |    6 ++++++
 net/netlink/af_netlink.c  |   37 +++++++++++++++++++++++++++++++------
 3 files changed, 38 insertions(+), 6 deletions(-)
Hmm, thats a hack, because netlink_dump() is generic and you add something very specific.

Quoted 7 lines

You also can't do this without breaking applications.
We've trained every single netlink library out there about this message size
formula, so they know that if you allocate at least 8192 bytes for a recvmsg()
call they can receive fully any single netlink message.

I checked the message sizes being generated and they were far less than 8K for 63 VFs and from my calculation would still be less than 8K for 127 VFs as per Ben Hutching's requirements. I thought I had 16k to work with as that is what is allocated by the iproute2 ip application also as per Ben's prior email. I checked and that is the case. But whatever the case I think we're still fine for 8K. I could put a check in to make sure alloc_size doesn't go over 8k.

Quoted 6 lines

And they must be able to make assumptions like this, otherwise they cannot know how to reliably be able to receive a netlink message in it's entirety in a generic fashion.
So one must not attack this problem from this angle.

This was just a bug fix. If you recall the email thread from last week that brought up this whole thing I was advocating for an entirely new angle myself. If the bug fix causes more problems than it solves, and that's not rare by any means, then we can toss it and go back to the start.

Quoted 4 lines

It is absolutely necessary to find some way to report the VF information, out of band, instead of trying to make the message larger.

Absolutely agreed!

Quoted 4 lines

Needing more than 8K to get a dump of a single device over netlink is absolutely rediculious, this VF stuff was poorly designed. If has to be fixed and the current stuff marked deprecated.

Again, absolutely no argument from me.

And I have an idea as to how to do this but Ben asked me to come up with a bug fix first and then work on the extensions that would do just what you're saying here later. I'm fine with dropping the bug fix hack and getting back to my original ideas on the subject. The problem there is that due to my work load it would be a month or more before I could finish it up and meanwhile the bug still exists.

I'm fine with however you folks want to approach this, just give me some direction.

- Greg

Eric Dumazet

2011-04-26 16:21 UTC #4 lore raw

Quoted 1 line · Gregory V

I'm fine with however you folks want to approach this, just give me some direction.

I would just try following patch :

include/linux/netlink.h +1 -0

index 4c4ac3f..22cac81 100644
@@ -210,6 +210,7 @@ int netlink_sendskb(struct sock *sk, struct sk_buff *skb);
 #else
 #define NLMSG_GOODSIZE	SKB_WITH_OVERHEAD(8192UL)
 #endif
+#define NLMSG_DUMPSIZE SKB_WITH_OVERHEAD(8192UL)
 
 #define NLMSG_DEFAULT_SIZE (NLMSG_GOODSIZE - NLMSG_HDRLEN)

net/netlink/af_netlink.c +1 -1

index c8f35b5..cb8d6ac 100644
@@ -1669,7 +1669,7 @@ static int netlink_dump(struct sock *sk)
 	struct nlmsghdr *nlh;
 	int len, err = -ENOBUFS;
 
-	skb = sock_rmalloc(sk, NLMSG_GOODSIZE, 0, GFP_KERNEL);
+	skb = sock_rmalloc(sk, NLMSG_DUMPSIZE, 0, GFP_KERNEL);
 	if (!skb)
 		goto errout;

Rose, Gregory V

author

2011-04-26 16:24 UTC #5 lore raw

Quoted 7 lines

-----Original Message-----
From: Eric Dumazet [mailto:[email protected]]
Sent: Tuesday, April 26, 2011 9:21 AM
To: Rose, Gregory V
Cc: David Miller; [email protected]; [email protected]
Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message size

Quoted 6 lines

Quoted 1 line · Gregory V
I'm fine with however you folks want to approach this, just give me some
direction.
I would just try following patch :

I'll test it out, it's certainly a lot simpler. Perhaps I was getting a bit too fancy.

Ben would want to make sure it works for 127 VFs, my device does 63.

- Greg

Quoted 29 lines

include/linux/netlink.h +1 -0

index 4c4ac3f..22cac81 100644
@@ -210,6 +210,7 @@ int netlink_sendskb(struct sock *sk, struct sk_buff
 *skb);
 #else
 #define NLMSG_GOODSIZE	SKB_WITH_OVERHEAD(8192UL)
 #endif
+#define NLMSG_DUMPSIZE SKB_WITH_OVERHEAD(8192UL)
 
 #define NLMSG_DEFAULT_SIZE (NLMSG_GOODSIZE - NLMSG_HDRLEN)

net/netlink/af_netlink.c +1 -1

index c8f35b5..cb8d6ac 100644
@@ -1669,7 +1669,7 @@ static int netlink_dump(struct sock *sk)
 	struct nlmsghdr *nlh;
 	int len, err = -ENOBUFS;
 
-	skb = sock_rmalloc(sk, NLMSG_GOODSIZE, 0, GFP_KERNEL);
+	skb = sock_rmalloc(sk, NLMSG_DUMPSIZE, 0, GFP_KERNEL);
 	if (!skb)
 		goto errout;

Ben Hutchings

2011-04-26 19:07 UTC #6 lore raw

Quoted 9 lines · Gregory V

Quoted 9 lines
-----Original Message-----
From: Eric Dumazet [mailto:[email protected]]
Sent: Tuesday, April 26, 2011 9:21 AM
To: Rose, Gregory V
Cc: David Miller; [email protected]; [email protected]
Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message size
Le mardi 26 avril 2011 à 09:12 -0700, Rose, Gregory V a écrit :

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
I'm fine with however you folks want to approach this, just give me some
direction.
I would just try following patch :
I'll test it out, it's certainly a lot simpler. Perhaps I was getting a bit too fancy.
Ben would want to make sure it works for 127 VFs, my device does 63.

I haven't been working on SR-IOV myself so I'll pass this on to a colleague. Thanks, Eric.

Ben.

Signature · 3 lines

Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Rose, Gregory V

author

2011-04-26 21:59 UTC #7 lore raw

Quoted 24 lines

-----Original Message-----
From: Ben Hutchings [mailto:[email protected]]
Sent: Tuesday, April 26, 2011 12:07 PM
To: Rose, Gregory V; Eric Dumazet
Cc: David Miller; [email protected]
Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message size
Quoted 6 lines · Gregory V
Quoted 6 lines
-----Original Message-----
From: Eric Dumazet [mailto:[email protected]]
Sent: Tuesday, April 26, 2011 9:21 AM
To: Rose, Gregory V
Cc: David Miller; [email protected]; [email protected]
Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message
size
Quoted 4 lines
Quoted 4 lines · Gregory V
Quoted 2 lines
I'm fine with however you folks want to approach this, just give me
some
Quoted 4 lines
Quoted 3 lines
direction.
I would just try following patch :

Quoted 7 lines

Quoted 1 line
I'll test it out, it's certainly a lot simpler. Perhaps I was getting a
bit too fancy.
Quoted 2 lines
Ben would want to make sure it works for 127 VFs, my device does 63.
I haven't been working on SR-IOV myself so I'll pass this on to a colleague. Thanks, Eric.

Eric's patch works fine for the case of 63 VFs. For most dumps it's allocating quite a bit more memory than necessary but that's not a big issue since it's transient and not held for long.

- Greg

Steve Hodgson

2011-04-27 15:46 UTC #8 lore raw

In reply to Eric Dumazet

Quoted 6 lines · Eric Dumazet

Quoted 1 line · Gregory V
I'm fine with however you folks want to approach this, just give me some direction.
I would just try following patch :

This allows the sfc driver to use 102 VFs, up from the current limit of 45 VFs.

It's unfortunate that this patch isn't sufficient to allow all 127 VFs to be used, but whilst we wait for a new netlink api this is an improvement worth having.

I'd like to see this patch committed.

Thanks Eric,

- Steve

Eric Dumazet

2011-04-27 16:30 UTC #9 lore raw

Quoted 8 lines · Steve Hodgson

Quoted 6 lines · Eric Dumazet
Quoted 1 line · Gregory V
I'm fine with however you folks want to approach this, just give me some direction.
I would just try following patch :

Quoted 7 lines

This allows the sfc driver to use 102 VFs, up from the current limit of 45 VFs.
It's unfortunate that this patch isn't sufficient to allow all 127 VFs to be used, but whilst we wait for a new netlink api this is an improvement worth having.

netlink recvmsg() supports MSG_PEEK so user would get the needed size of its buffer before calling the real recvmsg()

big blobs could be attached as skb fragments (up to 64Kbytes), but do we really want this...

Rose, Gregory V

author

2011-04-27 17:15 UTC #10 lore raw

Quoted 27 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Eric Dumazet
Sent: Wednesday, April 27, 2011 9:30 AM
To: Steve Hodgson
Cc: Rose, Gregory V; David Miller; [email protected]; [email protected]
Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message size
Quoted 4 lines · Steve Hodgson
Quoted 3 lines · Eric Dumazet
Quoted 1 line · Gregory V
I'm fine with however you folks want to approach this, just give me
some direction.
Quoted 11 lines
Quoted 3 lines
I would just try following patch :
This allows the sfc driver to use 102 VFs, up from the current limit of 45 VFs.
It's unfortunate that this patch isn't sufficient to allow all 127 VFs to be used, but whilst we wait for a new netlink api this is an improvement worth having.

Quoted 5 lines

netlink recvmsg() supports MSG_PEEK so user would get the needed size of its buffer before calling the real recvmsg()
big blobs could be attached as skb fragments (up to 64Kbytes), but do we really want this...

[Greg Rose]

I'm looking into an approach in which we make the get info dump for VFs orthogonal to the set VF info, i.e. like this:

To set VF info we would follow the current convention:

# ip link set eth(x) vf (n) mac xx:xx:xx:xx:xx:xx
# ip link set eth(x) vf (n) vlan (nnnn)

To see VF info:

# ip link show eth(x) vf (n)

would show that VF's mac and vlan and could then be expanded in the future to display more information required for additional features that users are asking for.

The IFLA_VF_INFO dump would be moved out of the info dump for the physical function interface and would no longer be nested which would get rid of the need for huge amounts of buffer for info dumps on VFs. The ip link show command for the PF would need to report the number of VFs currently allocated to the PF so that could fed into a script that loops to show each VFs info.

I think this approach would fix the problems we're looking at right now.

- Greg

Eric Dumazet

2011-04-27 17:29 UTC #11 lore raw

Quoted 43 lines · Gregory V

Quoted 27 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Eric Dumazet
Sent: Wednesday, April 27, 2011 9:30 AM
To: Steve Hodgson
Cc: Rose, Gregory V; David Miller; [email protected]; [email protected]
Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message size
Quoted 4 lines · Steve Hodgson
Quoted 3 lines · Eric Dumazet
Quoted 1 line · Gregory V
I'm fine with however you folks want to approach this, just give me
some direction.
Quoted 11 lines
Quoted 3 lines
I would just try following patch :
This allows the sfc driver to use 102 VFs, up from the current limit of 45 VFs.
It's unfortunate that this patch isn't sufficient to allow all 127 VFs to be used, but whilst we wait for a new netlink api this is an improvement worth having.
Quoted 5 lines
netlink recvmsg() supports MSG_PEEK so user would get the needed size of its buffer before calling the real recvmsg()
big blobs could be attached as skb fragments (up to 64Kbytes), but do we really want this...
[Greg Rose]
I'm looking into an approach in which we make the get info dump for VFs orthogonal to the set VF info, i.e. like this:
To set VF info we would follow the current convention:
# ip link set eth(x) vf (n) mac xx:xx:xx:xx:xx:xx
# ip link set eth(x) vf (n) vlan (nnnn)
To see VF info:

Quoted 8 lines

# ip link show eth(x) vf (n)
would show that VF's mac and vlan and could then be expanded in the future to display more information required for additional features that users are asking for.
The IFLA_VF_INFO dump would be moved out of the info dump for the physical function interface and would no longer be nested which would get rid of the need for huge amounts of buffer for info dumps on VFs. The ip link show command for the PF would need to report the number of VFs currently allocated to the PF so that could fed into a script that loops to show each VFs info.
I think this approach would fix the problems we're looking at right now.

Hmm, if you look at "ip link ..." you'll see it dumps everything from kernel and does the filter inside user command.

BTW "ip" uses a 16384 bytes buffer, not a 8192 bytes one.

$ strace -e trace=recvmsg ip link
recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
msg_iov(1)=[{"\340\3\0\0\20\0\2\0dR\270M\224s\0\0\0\0\4\3\1\0\0\0I\0\1\0
\0\0\0\0"..., 16384}], msg_controllen=0, msg_flags=0}, 0) = 3000
recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
msg_iov(1)=[{"\364\3\0\0\20\0\2\0dR\270M\224s\0\0\0\0\1\0\4\0\0\0C\24\1
\0\0\0\0\0"..., 16384}], msg_controllen=0, msg_flags=0}, 0) = 3024
recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
msg_iov(1)=[{"\34\4\0\0\20\0\2\0dR\270M\224s\0\0\0\0\1\0\7\0\0\0C\20\1\0
\0\0\0\0"..., 16384}], msg_controllen=0, msg_flags=0}, 0) = 2104
recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000},
msg_iov(1)=[{"\24\0\0\0\3\0\2\0dR\270M\224s\0\0\0\0\0\0\7\0\0\0C\20\1\0
\0\0\0\0"..., 16384}], msg_controllen=0, msg_flags=0}, 0) = 20

...

Rose, Gregory V

author

2011-04-27 17:40 UTC #12 lore raw

Quoted 73 lines

-----Original Message-----
From: Eric Dumazet [mailto:[email protected]]
Sent: Wednesday, April 27, 2011 10:30 AM
To: Rose, Gregory V
Cc: Steve Hodgson; David Miller; [email protected]; [email protected]
Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message size
Quoted 2 lines · Gregory V
Quoted 2 lines
-----Original Message-----
From: [email protected] [mailto:netdev-
[email protected]]
Quoted 6 lines
Quoted 6 lines
On Behalf Of Eric Dumazet
Sent: Wednesday, April 27, 2011 9:30 AM
To: Steve Hodgson
Cc: Rose, Gregory V; David Miller; [email protected]; [email protected]
Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message
size
Quoted 6 lines
Quoted 6 lines · Steve Hodgson
Quoted 4 lines · Eric Dumazet
Quoted 3 lines · Gregory V
Quoted 2 lines
I'm fine with however you folks want to approach this, just give
me
Quoted 6 lines
Quoted 6 lines
some direction.
Quoted 5 lines
Quoted 3 lines
I would just try following patch :
This allows the sfc driver to use 102 VFs, up from the current limit
of
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
45 VFs.
It's unfortunate that this patch isn't sufficient to allow all 127
VFs
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
to be used, but whilst we wait for a new netlink api this is an improvement worth having.
netlink recvmsg() supports MSG_PEEK so user would get the needed size
of
Quoted 3 lines
Quoted 3 lines
its buffer before calling the real recvmsg()
big blobs could be attached as skb fragments (up to 64Kbytes), but do
we
Quoted 4 lines
Quoted 1 line
really want this...
[Greg Rose]
I'm looking into an approach in which we make the get info dump for VFs
orthogonal to the set VF info, i.e. like this:
Quoted 11 lines
To set VF info we would follow the current convention:
# ip link set eth(x) vf (n) mac xx:xx:xx:xx:xx:xx
# ip link set eth(x) vf (n) vlan (nnnn)
To see VF info:
# ip link show eth(x) vf (n)
would show that VF's mac and vlan and could then be expanded in the
future to display more information required for additional features that users are asking for.
Quoted 2 lines
The IFLA_VF_INFO dump would be moved out of the info dump for the
physical function interface and would no longer be nested which would get rid of the need for huge amounts of buffer for info dumps on VFs. The ip link show command for the PF would need to report the number of VFs currently allocated to the PF so that could fed into a script that loops to show each VFs info.
Quoted 1 line

Quoted 5 lines

Quoted 2 lines
I think this approach would fix the problems we're looking at right now.
Hmm, if you look at "ip link ..." you'll see it dumps everything from kernel and does the filter inside user command.

Right, but when I look in rtnetlink I see the routine to calculate the amount of buffer needed for VF info dump is the number of device parent (PF) VFs * the sizeof various IFLA_VF_INFO items. The more the VFs the bigger this gets, especially if you want to add more stuff to IFLA_VF_INFO. So when the kernel dumps this all out it can get bigger than the NLMSG_GOODSIZE (or DUMPSIZE) pretty quickly.

Quoted 2 lines

BTW "ip" uses a 16384 bytes buffer, not a 8192 bytes one.

I know, that's why I suffered some confusion about which size to use. The ip command uses 16K but the NLMSG_GOODSIZE can be as small as 3712 bytes (depending on page size). Despite the user buffer being 16k if the size calculated by if_nlmsg_size() in rtnetlink.c is bigger than NLMSG_GOODSIZE then you don't see the info for more than 40 or so VFs. More VFs than that and nothing gets displayed.

- Greg

Eric Dumazet

2011-04-27 18:05 UTC #13 lore raw

Quoted 8 lines · Gregory V

Right, but when I look in rtnetlink I see the routine to calculate the amount of buffer needed for VF info dump is the number of device parent (PF) VFs * the sizeof various IFLA_VF_INFO items. The more the VFs the bigger this gets, especially if you want to add more stuff to IFLA_VF_INFO. So when the kernel dumps this all out it can get bigger than the NLMSG_GOODSIZE (or DUMPSIZE) pretty quickly.
Quoted 1 line

Quoted 9 lines

Quoted 1 line
BTW "ip" uses a 16384 bytes buffer, not a 8192 bytes one.
I know, that's why I suffered some confusion about which size to use. The ip command uses 16K but the NLMSG_GOODSIZE can be as small as 3712 bytes (depending on page size). Despite the user buffer being 16k if the size calculated by if_nlmsg_size() in rtnetlink.c is bigger than NLMSG_GOODSIZE then you don't see the info for more than 40 or so VFs. More VFs than that and nothing gets displayed.

One solution is to change rtnl_dump_ifinfo() to call rtnl_fill_ifinfo() once time per device (RTM_NEWLINK like now but no more VFINFO inside), then call another function to provide vf/vlan informations (RTM_NEWVF), using cb->args[2] as an index into VF space, so that we can stop if current skb is filled, and next recvmsg() starts at previous saved index.

Or implement a new "ip vf ..." dumper and only dumps RTM_NEWVF messages.

Rose, Gregory V

author

2011-04-27 18:08 UTC #14 lore raw

Quoted 28 lines

-----Original Message-----
From: Eric Dumazet [mailto:[email protected]]
Sent: Wednesday, April 27, 2011 11:06 AM
To: Rose, Gregory V
Cc: Steve Hodgson; David Miller; [email protected]; [email protected]
Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message size
Quoted 8 lines · Gregory V
Right, but when I look in rtnetlink I see the routine to calculate the amount of buffer needed for VF info dump is the number of device parent (PF) VFs * the sizeof various IFLA_VF_INFO items. The more the VFs the bigger this gets, especially if you want to add more stuff to IFLA_VF_INFO. So when the kernel dumps this all out it can get bigger than the NLMSG_GOODSIZE (or DUMPSIZE) pretty quickly.
Quoted 1 line
Quoted 9 lines
Quoted 1 line
BTW "ip" uses a 16384 bytes buffer, not a 8192 bytes one.
I know, that's why I suffered some confusion about which size to use. The ip command uses 16K but the NLMSG_GOODSIZE can be as small as 3712 bytes (depending on page size). Despite the user buffer being 16k if the size calculated by if_nlmsg_size() in rtnetlink.c is bigger than NLMSG_GOODSIZE then you don't see the info for more than 40 or so VFs. More VFs than that and nothing gets displayed.

Quoted 8 lines

One solution is to change rtnl_dump_ifinfo() to call rtnl_fill_ifinfo() once time per device (RTM_NEWLINK like now but no more VFINFO inside), then call another function to provide vf/vlan informations (RTM_NEWVF), using cb->args[2] as an index into VF space, so that we can stop if current skb is filled, and next recvmsg() starts at previous saved index.
Or implement a new "ip vf ..." dumper and only dumps RTM_NEWVF messages.

I'm a netlink newbie so maybe that is a more obvious solution that I missed. Let me have a look at the code and see.

Thanks,

- Greg

Rose, Gregory V

author

2011-04-26 16:02 UTC #15 lore raw

In reply to Eric Dumazet

Quoted 24 lines

-----Original Message-----
From: Eric Dumazet [mailto:[email protected]]
Sent: Monday, April 25, 2011 11:33 PM
To: Rose, Gregory V
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message size
Quoted 1 line · Greg Rose
The message size allocated for rtnl info dumps was limited to a single
page.
Quoted 2 lines
This is not enough for additional interface info available with devices that support SR-IOV. Check that the amount of data allocated is
sufficient
Quoted 10 lines
for the amount of data requested.
Signed-off-by Greg Rose <[email protected]>
---
 include/linux/rtnetlink.h |    1 +
 net/core/rtnetlink.c      |    6 ++++++
 net/netlink/af_netlink.c  |   37 +++++++++++++++++++++++++++++++------
 3 files changed, 38 insertions(+), 6 deletions(-)

Quoted 7 lines

Hmm, thats a hack, because netlink_dump() is generic and you add something very specific.
I prefer something that allows one dump() to reallocate a bigger skb
Maybe changing->dump() prototype to struct sk_buff **pskb instead of struct sk_buff *skb.

I'll have a look at that, although Dave's not too happy with the whole lot of this mess and I very much agree with him.

- Greg

Quoted 15 lines

net/netlink/af_netlink.c +1 -1

index c8f35b5..7fa6735 100644
@@ -1681,7 +1681,7 @@ static int netlink_dump(struct sock *sk)
 		goto errout_skb;
 	}
 
-	len = cb->dump(skb, cb);
+	len = cb->dump(&skb, cb);
 
 	if (len > 0) {
 		mutex_unlock(nlk->cb_mutex);

David Miller

maintainer

2011-04-29 19:30 UTC #16 lore raw

In reply to Greg Rose

From: Greg Rose <[email protected]>
Date: Mon, 25 Apr 2011 15:01:57 -0700

Quoted 6 lines

The message size allocated for rtnl info dumps was limited to a single page. This is not enough for additional interface info available with devices that support SR-IOV. Check that the amount of data allocated is sufficient for the amount of data requested.
Signed-off-by Greg Rose <[email protected]>

Actually, thinking about this problem some more I think your approach is close to what we should actually do.

The only problem is that you've specialized netlink_dump() too much, hide the issue in the rtnetlink device dumping code and provide the necessary information via the control block.

1) Add some information to struct netlink_callback.

"u16 min_dump_alloc;"

I think you can change "int family;" there to "u16 family;" so that there is no new space required to add this functionality.

2) In netlink_dump().

	int alloc_size;

	mutex_lock(nlk->cb_mutex);
	cb = nlk->cb;
	if (cb == NULL) {
		...
	}
	alloc_size = max_t(int, cb->min_dump_alloc, NLMSG_GOODSIZE);
	skb = sock_rmalloc(sk, alloc_size, 0, GFP_KERNEL);
	if (!skb) {
	...

3) In net/core/rtnetlink.c add a new method pointer to struct rtnl_link, "u16 (*calc_min_dump_alloc)(struct sk_buff *);"
Add an implementation for the RTM_GETLINK slot.

This function does the logic to compute the maximum space needed for the devices currently configured, as you hacked directly into the netlink_dump() logic in your match.

4) Make netlink_dump_start() take a new argument, this is where you pass the new min_dump_alloc value that will get stored in the newly allocated callback object.

	calcit = rtnl_get_calcit(family, type);

   	min_dump_alloc = 0;
	if (calcit)
		min_dump_alloc = calcit(skb);
	err = netlink_dump_start(rtnl, skb, nlh, dumpit, min_dump_alloc, NULL);

Anyways, something like that.

Rose, Gregory V

author

2011-04-29 19:37 UTC #17 lore raw

Quoted 61 lines

-----Original Message-----
From: David Miller [mailto:[email protected]]
Sent: Friday, April 29, 2011 12:30 PM
To: Rose, Gregory V
Cc: [email protected]; [email protected]
Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message size
From: Greg Rose <[email protected]>
Date: Mon, 25 Apr 2011 15:01:57 -0700
Quoted 1 line
The message size allocated for rtnl info dumps was limited to a single
page.
Quoted 2 lines
This is not enough for additional interface info available with devices that support SR-IOV. Check that the amount of data allocated is
sufficient
Quoted 3 lines
for the amount of data requested.
Signed-off-by Greg Rose <[email protected]>
Actually, thinking about this problem some more I think your approach is close to what we should actually do.
The only problem is that you've specialized netlink_dump() too much, hide the issue in the rtnetlink device dumping code and provide the necessary information via the control block.
1) Add some information to struct netlink_callback.
"u16 min_dump_alloc;"
I think you can change "int family;" there to "u16 family;" so that there is no new space required to add this functionality.
2) In netlink_dump().
	int alloc_size;

	mutex_lock(nlk->cb_mutex);
	cb = nlk->cb;
	if (cb == NULL) {
		...
	}
	alloc_size = max_t(int, cb->min_dump_alloc, NLMSG_GOODSIZE);
	skb = sock_rmalloc(sk, alloc_size, 0, GFP_KERNEL);
	if (!skb) {
	...
3) In net/core/rtnetlink.c add a new method pointer to struct rtnl_link, "u16 (*calc_min_dump_alloc)(struct sk_buff *);"
Add an implementation for the RTM_GETLINK slot.
This function does the logic to compute the maximum space needed for the devices currently configured, as you hacked directly into the netlink_dump() logic in your match.
4) Make netlink_dump_start() take a new argument, this is where you pass the new min_dump_alloc value that will get stored in the newly allocated callback object.
	calcit = rtnl_get_calcit(family, type);

Quoted 7 lines

   	min_dump_alloc = 0;
	if (calcit)
		min_dump_alloc = calcit(skb);
	err = netlink_dump_start(rtnl, skb, nlh, dumpit, min_dump_alloc,

NULL);

Anyways, something like that.

[Greg Rose]

I'll look into this and try to get a revised patch done by early next week.

The feedback and suggestions are much appreciated.

- Greg