[RFC PATCH] netlink: Increase netlink dump skb message size

[RFC PATCH] netlink: Increase netlink dump skb message size

[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.

Signed-off-by: Greg Rose <gregory.v.rose@intel.com>
---

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

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index bbad657..d1ff937 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -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);
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d7c4bb4..001c947 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -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;
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c8f35b5..5b1106c 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -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;
 }
 

Re: [RFC PATCH] netlink: Increase netlink dump skb message size

[Eric Dumazet]

Le lundi 25 avril 2011 à 15:01 -0700, Greg Rose a écrit :
> 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 <gregory.v.rose@intel.com>
> ---
> 
>  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.

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c8f35b5..7fa6735 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -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);

Re: [RFC PATCH] netlink: Increase netlink dump skb message size

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 26 Apr 2011 08:33:17 +0200

> Le lundi 25 avril 2011 à 15:01 -0700, Greg Rose a écrit :
>> 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 <gregory.v.rose@intel.com>
>> ---
>> 
>>  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.

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Rose, Gregory V]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Monday, April 25, 2011 11:33 PM
> To: Rose, Gregory V
> Cc: netdev@vger.kernel.org; bhutchings@solarflare.com; davem@davemloft.net
> Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message size
> 
> Le lundi 25 avril 2011 à 15:01 -0700, Greg Rose a écrit :
> > 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 <gregory.v.rose@intel.com>
> > ---
> >
> >  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.

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
> 
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index c8f35b5..7fa6735 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -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);
> 

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Rose, Gregory V]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Monday, April 25, 2011 11:56 PM
> To: eric.dumazet@gmail.com
> Cc: Rose, Gregory V; netdev@vger.kernel.org; bhutchings@solarflare.com
> Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message size
> 
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 26 Apr 2011 08:33:17 +0200
> 
> > Le lundi 25 avril 2011 à 15:01 -0700, Greg Rose a écrit :
> >> 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 <gregory.v.rose@intel.com>
> >> ---
> >>
> >>  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.

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.

> 
> 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.

> 
> 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!

> 
> 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

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Eric Dumazet]

Le mardi 26 avril 2011 à 09:12 -0700, Rose, Gregory V a écrit :

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

I would just try following patch :


diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 4c4ac3f..22cac81 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -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)
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c8f35b5..cb8d6ac 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -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;
 

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Rose, Gregory V]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Tuesday, April 26, 2011 9:21 AM
> To: Rose, Gregory V
> Cc: David Miller; netdev@vger.kernel.org; bhutchings@solarflare.com
> Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message size
> 
> Le mardi 26 avril 2011 à 09:12 -0700, Rose, Gregory V a écrit :
> 
> > 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

> 
> 
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 4c4ac3f..22cac81 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -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)
> 
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index c8f35b5..cb8d6ac 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -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;
> 
> 

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Ben Hutchings]

On Tue, 2011-04-26 at 09:24 -0700, Rose, Gregory V wrote:
> > -----Original Message-----
> > From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> > Sent: Tuesday, April 26, 2011 9:21 AM
> > To: Rose, Gregory V
> > Cc: David Miller; netdev@vger.kernel.org; bhutchings@solarflare.com
> > Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message size
> > 
> > Le mardi 26 avril 2011 à 09:12 -0700, Rose, Gregory V a écrit :
> > 
> > > 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.

-- 
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.

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Rose, Gregory V]

> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: Tuesday, April 26, 2011 12:07 PM
> To: Rose, Gregory V; Eric Dumazet
> Cc: David Miller; netdev@vger.kernel.org
> Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message size
> 
> On Tue, 2011-04-26 at 09:24 -0700, Rose, Gregory V wrote:
> > > -----Original Message-----
> > > From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> > > Sent: Tuesday, April 26, 2011 9:21 AM
> > > To: Rose, Gregory V
> > > Cc: David Miller; netdev@vger.kernel.org; bhutchings@solarflare.com
> > > Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message
> size
> > >
> > > Le mardi 26 avril 2011 à 09:12 -0700, Rose, Gregory V a écrit :
> > >
> > > > 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.

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

Re: [RFC PATCH] netlink: Increase netlink dump skb message size

[Steve Hodgson]

On 04/27/2011 04:24 PM, Eric Dumazet wrote:
> Le mardi 26 avril 2011 à 09:12 -0700, Rose, Gregory V a écrit :
>
>> 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

Re: [RFC PATCH] netlink: Increase netlink dump skb message size

[Eric Dumazet]

Le mercredi 27 avril 2011 à 16:46 +0100, Steve Hodgson a écrit :
> On 04/27/2011 04:24 PM, Eric Dumazet wrote:
> > Le mardi 26 avril 2011 à 09:12 -0700, Rose, Gregory V a écrit :
> >
> >> 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.
> 

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...

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Rose, Gregory V]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Eric Dumazet
> Sent: Wednesday, April 27, 2011 9:30 AM
> To: Steve Hodgson
> Cc: Rose, Gregory V; David Miller; netdev@vger.kernel.org;
> bhutchings@solarflare.com
> Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message size
> 
> Le mercredi 27 avril 2011 à 16:46 +0100, Steve Hodgson a écrit :
> > On 04/27/2011 04:24 PM, Eric Dumazet wrote:
> > > Le mardi 26 avril 2011 à 09:12 -0700, Rose, Gregory V a écrit :
> > >
> > >> 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.
> >
> 
> 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

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Eric Dumazet]

Le mercredi 27 avril 2011 à 10:15 -0700, Rose, Gregory V a écrit :
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> > On Behalf Of Eric Dumazet
> > Sent: Wednesday, April 27, 2011 9:30 AM
> > To: Steve Hodgson
> > Cc: Rose, Gregory V; David Miller; netdev@vger.kernel.org;
> > bhutchings@solarflare.com
> > Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message size
> > 
> > Le mercredi 27 avril 2011 à 16:46 +0100, Steve Hodgson a écrit :
> > > On 04/27/2011 04:24 PM, Eric Dumazet wrote:
> > > > Le mardi 26 avril 2011 à 09:12 -0700, Rose, Gregory V a écrit :
> > > >
> > > >> 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.
> > >
> > 
> > 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.
> 

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

...

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Rose, Gregory V]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Wednesday, April 27, 2011 10:30 AM
> To: Rose, Gregory V
> Cc: Steve Hodgson; David Miller; netdev@vger.kernel.org;
> bhutchings@solarflare.com
> Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message size
> 
> Le mercredi 27 avril 2011 à 10:15 -0700, Rose, Gregory V a écrit :
> > > -----Original Message-----
> > > From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org]
> > > On Behalf Of Eric Dumazet
> > > Sent: Wednesday, April 27, 2011 9:30 AM
> > > To: Steve Hodgson
> > > Cc: Rose, Gregory V; David Miller; netdev@vger.kernel.org;
> > > bhutchings@solarflare.com
> > > Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message
> size
> > >
> > > Le mercredi 27 avril 2011 à 16:46 +0100, Steve Hodgson a écrit :
> > > > On 04/27/2011 04:24 PM, Eric Dumazet wrote:
> > > > > Le mardi 26 avril 2011 à 09:12 -0700, Rose, Gregory V a écrit :
> > > > >
> > > > >> 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.
> > > >
> > >
> > > 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.
> >
> 
> 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.

> 
> 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

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Eric Dumazet]

Le mercredi 27 avril 2011 à 10:39 -0700, Rose, Gregory V a écrit :

> 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.
> 
> > 
> > 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.

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Rose, Gregory V]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Wednesday, April 27, 2011 11:06 AM
> To: Rose, Gregory V
> Cc: Steve Hodgson; David Miller; netdev@vger.kernel.org;
> bhutchings@solarflare.com
> Subject: RE: [RFC PATCH] netlink: Increase netlink dump skb message size
> 
> Le mercredi 27 avril 2011 à 10:39 -0700, Rose, Gregory V a écrit :
> 
> > 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.
> >
> > >
> > > 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.

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

Re: [RFC PATCH] netlink: Increase netlink dump skb message size

[David Miller]

From: Greg Rose <gregory.v.rose@intel.com>
Date: Mon, 25 Apr 2011 15:01:57 -0700

> 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 <gregory.v.rose@intel.com>

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.

RE: [RFC PATCH] netlink: Increase netlink dump skb message size

[Rose, Gregory V]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Friday, April 29, 2011 12:30 PM
> To: Rose, Gregory V
> Cc: netdev@vger.kernel.org; bhutchings@solarflare.com
> Subject: Re: [RFC PATCH] netlink: Increase netlink dump skb message size
> 
> From: Greg Rose <gregory.v.rose@intel.com>
> Date: Mon, 25 Apr 2011 15:01:57 -0700
> 
> > 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 <gregory.v.rose@intel.com>
> 
> 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.
[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