[patch v1, kernel version 3.2.1] rtnetlink workaround around the skb buff size issue

From: Stefan Gula <redacted>

Adding new rtnetlink ops and command for getting more information about
network devices, which are not able to fit inside predefined SKB structures
(e.g. PAGE_SIZE limit). DEVDUMP command allows to call specific device driver
code for complete handling this netlink message. Useful if devices needed to
list some addition dynamic structures like hlists and doesn't require to have
complete set of codes for it new PF families.

Signed-off-by: Stefan Gula <redacted>

---
 - this patch is per-requisition for my new macvlan patch


diff -uprN -X linux-3.2.1-orig/Documentation/dontdiff linux-3.2.1-orig/include/linux/rtnetlink.h linux-3.2.1-macvlan/include/linux/rtnetlink.h

--- linux-3.2.1-orig/include/linux/rtnetlink.h	2012-01-27 13:38:57.000000000 +0000
+++ linux-3.2.1-macvlan/include/linux/rtnetlink.h	2012-02-02 08:34:14.000000000 +0000

@@ -120,6 +120,8 @@ enum {
 	RTM_SETDCB,
 #define RTM_SETDCB RTM_SETDCB
 
+	RTM_DEVDUMP = 80,
+#define RTM_DEVDUMP	RTM_DEVDUMP
 	__RTM_MAX,
 #define RTM_MAX		(((__RTM_MAX + 3) & ~3) - 1)
 };

diff -uprN -X linux-3.2.1-orig/Documentation/dontdiff linux-3.2.1-orig/include/net/rtnetlink.h linux-3.2.1-macvlan/include/net/rtnetlink.h

--- linux-3.2.1-orig/include/net/rtnetlink.h	2012-01-27 13:38:57.000000000 +0000
+++ linux-3.2.1-macvlan/include/net/rtnetlink.h	2012-02-02 05:58:50.000000000 +0000

@@ -78,6 +78,9 @@ struct rtnl_link_ops {
 	int			(*get_tx_queues)(struct net *net, struct nlattr *tb[],
 						 unsigned int *tx_queues,
 						 unsigned int *real_tx_queues);
+	int			(*dev_dump)(struct sk_buff *skb,
+					    struct net_device *dev, int type,
+					    u32 pid, u32 seq);
 };
 
 extern int	__rtnl_link_register(struct rtnl_link_ops *ops);

diff -uprN -X linux-3.2.1-orig/Documentation/dontdiff linux-3.2.1-orig/net/core/rtnetlink.c linux-3.2.1-macvlan/net/core/rtnetlink.c

--- linux-3.2.1-orig/net/core/rtnetlink.c	2012-01-27 14:22:12.000000000 +0000
+++ linux-3.2.1-macvlan/net/core/rtnetlink.c	2012-02-02 23:59:54.000000000 +0000

@@ -1874,6 +1874,42 @@ static int rtnl_getlink(struct sk_buff *
 	return err;
 }
 
+static int rtnl_devdump(struct sk_buff *skb, struct nlmsghdr* nlh, void *arg)
+{
+	struct net *net = sock_net(skb->sk);
+	struct ifinfomsg *ifm;
+	char ifname[IFNAMSIZ];
+	struct nlattr *tb[IFLA_MAX+1];
+	struct net_device *dev = NULL;
+	int err;
+	const struct rtnl_link_ops *ops;
+
+	err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, ifla_policy);
+	if (err < 0)
+		return err;
+
+	if (tb[IFLA_IFNAME])
+		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
+
+	ifm = nlmsg_data(nlh);
+	if (ifm->ifi_index > 0)
+		dev = __dev_get_by_index(net, ifm->ifi_index);
+	else if (tb[IFLA_IFNAME])
+		dev = __dev_get_by_name(net, ifname);
+	else
+		return -EINVAL;
+
+	if (dev == NULL)
+		return -ENODEV;
+
+	ops = dev->rtnl_link_ops;
+	if (ops && ops->dev_dump)
+		return ops->dev_dump(skb, dev, RTM_DEVDUMP,
+				     NETLINK_CB(skb).pid, nlh->nlmsg_seq);
+
+	return -EOPNOTSUPP;
+}
+
 static u16 rtnl_calcit(struct sk_buff *skb)
 {
 	return min_ifinfo_dump_size;

@@ -2097,5 +2133,7 @@ void __init rtnetlink_init(void)
 
 	rtnl_register(PF_UNSPEC, RTM_GETADDR, NULL, rtnl_dump_all, NULL);
 	rtnl_register(PF_UNSPEC, RTM_GETROUTE, NULL, rtnl_dump_all, NULL);
+
+	rtnl_register(PF_UNSPEC, RTM_DEVDUMP, rtnl_devdump, NULL, NULL);
 }
 

From: Stefan Gula <redacted>
Date: Fri, 3 Feb 2012 15:24:21 +0100 (CET)

From: Stefan Gula <redacted>

Adding new rtnetlink ops and command for getting more information about
network devices, which are not able to fit inside predefined SKB structures
(e.g. PAGE_SIZE limit). DEVDUMP command allows to call specific device driver
code for complete handling this netlink message. Useful if devices needed to
list some addition dynamic structures like hlists and doesn't require to have
complete set of codes for it new PF families.

Signed-off-by: Stefan Gula <redacted>

This is not how we're going to fix this.  I already stated the desired
way to fix this, which is to make the existing dump request have a way
for the requestor to enable extended parts of the device dump.

This is just like netlink diag socket dumps, where the dump request
specifies what the user wants to see.

In this case we'd add a netlink attribute to the dump request which
is just a u32 bitmask or similar.

The Intel engineer who added the VF dump support said he would work on
this fix so why don't you just wait patiently for him to do the work?

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of David Miller
Sent: Friday, February 03, 2012 4:30 PM
To: steweg@ynet.sk
Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org
Subject: Re: [patch v1, kernel version 3.2.1] rtnetlink workaround around
the skb buff size issue

From: Stefan Gula <redacted>
Date: Fri, 3 Feb 2012 15:24:21 +0100 (CET)

quoted

From: Stefan Gula <redacted>

Adding new rtnetlink ops and command for getting more information about
network devices, which are not able to fit inside predefined SKB

structures

quoted

(e.g. PAGE_SIZE limit). DEVDUMP command allows to call specific device

driver

quoted

code for complete handling this netlink message. Useful if devices

needed to

quoted

list some addition dynamic structures like hlists and doesn't require to

have

quoted

complete set of codes for it new PF families.

Signed-off-by: Stefan Gula <redacted>

This is not how we're going to fix this.  I already stated the desired
way to fix this, which is to make the existing dump request have a way
for the requestor to enable extended parts of the device dump.

This is just like netlink diag socket dumps, where the dump request
specifies what the user wants to see.

In this case we'd add a netlink attribute to the dump request which
is just a u32 bitmask or similar.

The Intel engineer who added the VF dump support said he would work on
this fix so why don't you just wait patiently for him to do the work?

The patch below is what I've got so far.  Right now the bit mask array is global so if you enable display of VF (n) on one interface it will enable display of the same VF on other interfaces.  I intend to move the bit mask array into the net_device structure so we can set the display mask for each interface independently.

The command to set the filter mask is "set only", I see no reason to add it to the info dump.  If other folks see it differently then I can do that too.

Anyway, it will allow the user to control which VFs are getting displayed during the info dump.  They all default to off so initially no VF info gets displayed.

I've also whipped up a patch for the iproute2 ip command.  It'll work like this:

'ip link set <dev> vf (n) filter [on|off]'

So if you have 128 VFs on the device you could enable info dumps for arbitrary VFs, e.g. VFs 3, 9, 16, 21, and 31.  Only the info for those VFs would display.  This method has the advantage of not breaking scripts which parse the current VF info display.  Of course, one could also script up something to sequentially enable the display of a single VF, dump the info for it, and then move on to the next.

Before I go much further on this let me know if this is the right track or not.

Thanks,

- Greg

---

 include/linux/if_link.h |    6 ++++++
 net/core/rtnetlink.c    |   30 +++++++++++++++++++++++++++---
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index c52d4b5..052c240 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h

@@ -280,6 +280,7 @@ enum {
 	IFLA_VF_VLAN,
 	IFLA_VF_TX_RATE,	/* TX Bandwidth Allocation */
 	IFLA_VF_SPOOFCHK,	/* Spoof Checking on/off switch */
+	IFLA_VF_INFOFILTER,	/* Filter vfinfo on dumps */
 	__IFLA_VF_MAX,
 };
 

@@ -305,6 +306,11 @@ struct ifla_vf_spoofchk {
 	__u32 vf;
 	__u32 setting;
 };
+
+struct ifla_vf_infofilter {
+	__u32 vf;
+	__u32 filter;
+};
 #ifdef __KERNEL__
 
 /* We don't want this structure exposed to user space */

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index af1da12..8c0c8c1 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

@@ -62,6 +62,9 @@ struct rtnl_link {
 static DEFINE_MUTEX(rtnl_mutex);
 static u16 min_ifinfo_dump_size;
 
+/* VF info display filter - Number of VFs max is 256 */
+static unsigned long show_vfinfo_filter[256 / sizeof(unsigned long)];
+
 void rtnl_lock(void)
 {
 	mutex_lock(&rtnl_mutex);

@@ -876,6 +879,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	const struct rtnl_link_stats64 *stats;
 	struct nlattr *attr, *af_spec;
 	struct rtnl_af_ops *af_ops;
+	u32 num_vf_filters_set = 0;
 
 	ASSERT_RTNL();
 	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*ifm), flags);

@@ -941,10 +945,18 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 		goto nla_put_failure;
 	copy_rtnl_link_stats64(nla_data(attr), stats);
 
-	if (dev->dev.parent)
-		NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent));
+	if (dev->dev.parent) {
+		int j;
+		for (j = 0; j < 256; j++) {
+			if (test_bit(j, show_vfinfo_filter))
+				num_vf_filters_set++;
+		}
+		if (num_vf_filters_set)
+			NLA_PUT_U32(skb, IFLA_NUM_VF, num_vf_filters_set);
+	}
 
-	if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent) {
+	if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent &&
+			num_vf_filters_set) {
 		int i;
 
 		struct nlattr *vfinfo, *vf;

@@ -960,6 +972,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			struct ifla_vf_tx_rate vf_tx_rate;
 			struct ifla_vf_spoofchk vf_spoofchk;
 
+			if (!test_bit(i, show_vfinfo_filter))
+				continue;
+
 			/*
 			 * Not all SR-IOV capable drivers support the
 			 * spoofcheck query.  Preset to -1 so the user

@@ -1234,6 +1249,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
 							       ivs->setting);
 			break;
 		}
+		case IFLA_VF_INFOFILTER: {
+			 struct ifla_vf_infofilter *ivf;
+			 ivf = nla_data(vf);
+			 if (ivf->filter)
+				 set_bit(ivf->vf, show_vfinfo_filter);
+			 else
+				 clear_bit(ivf->vf, show_vfinfo_filter);
+			 break;
+		}
 		default:
 			err = -EINVAL;
 			break;

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

2012/2/6 Rose, Gregory V [off-list ref]:

The patch below is what I've got so far.  Right now the bit mask array is global so if you enable display of VF (n) on one interface it will enable display of the same VF on other interfaces.  I intend to move the bit mask array into the net_device structure so we can set the display mask for each interface independently.

The command to set the filter mask is "set only", I see no reason to add it to the info dump.  If other folks see it differently then I can do that too.

Anyway, it will allow the user to control which VFs are getting displayed during the info dump.  They all default to off so initially no VF info gets displayed.

I've also whipped up a patch for the iproute2 ip command.  It'll work like this:

'ip link set <dev> vf (n) filter [on|off]'

So if you have 128 VFs on the device you could enable info dumps for arbitrary VFs, e.g. VFs 3, 9, 16, 21, and 31.  Only the info for those VFs would display.  This method has the advantage of not breaking scripts which parse the current VF info display.  Of course, one could also script up something to sequentially enable the display of a single VF, dump the info for it, and then move on to the next.

As this patch will allow one to filter some information and possible
lower the need on skb buffer size, the general idea is ok. On the
other hand it will not eliminate the problem. e.g.:
- assume that one didn't know the limits behind it and put all options enabled
- it also doesn't fix the need to fill relevant info by interface
bigger than buffer size, e.g. my macvlan interface mac address list.
If I try to request for it, it will eventually fail with a lot of
records even with filtering...

So I would rather see a proper general method for requesting some
information in cycles inside the single interface like sending request
to kernel per VF for particular device or per MAC address from macvlan
associated lists. This approach is I believe slightly more scalable as
it can be potentially reused on other types of network devices as
well.

My original idea was to have these methods:
1st kernel method will return some info about the absolute number of
cycles needed per given interface - this can be done in standard
GETLINK operation with some associated IFLA_* value.
2nd user-space method will based on that number sends netlink request
per records or reasonable page of records (e.g. 10) and parse the
output in the user-space.
 - this is needed to overcome another issue when kernel generates so
many netlink messages with NLM_F_MULTI that netlink socket will not be
able to hold and further write/read code will fail.
3rd kernel method, which will allocated, fill, and send required info
per record -> this one can be done by ops/command netlink association
(in my proposal it is DEVDUMP)

To sum up, I believe that both approaches (using cycles and filtering)
should be allowed to coexists in kernel, but they should be considered
separately as they are doing different jobs.

I can provide whole example of code (not just rtnetlink part), if the
list is interested to see live example.

From: Štefan Gula <redacted>
Date: Mon, 6 Feb 2012 09:53:28 +0100

If I try to request for it, it will eventually fail with a lot of
records even with filtering...

Then the user can loop increasing the buffer size until the netlink
request succeeds.

It is not a problem.

Le lundi 06 février 2012 à 10:15 -0500, David Miller a écrit :

From: Štefan Gula <redacted>
Date: Mon, 6 Feb 2012 09:53:28 +0100

quoted

If I try to request for it, it will eventually fail with a lot of
records even with filtering...

Then the user can loop increasing the buffer size until the netlink
request succeeds.

It is not a problem.

Actually we always truncate message in netlink_recvmsg()

We could use a MSG_NOPARTIAL flag in netlink_recvmsg() so that user can
avoid the MSG_PEEK operation to fetch next message length.

(Ie not consume/copy skb if user buffer is too small to hold full
message, and only return the needed length)

-----Original Message-----
From: steweg@gmail.com [mailto:steweg@gmail.com] On Behalf Of Štefan Gula
Sent: Monday, February 06, 2012 12:53 AM
To: Rose, Gregory V
Cc: David Miller; linux-kernel@vger.kernel.org; netdev@vger.kernel.org
Subject: Re: [patch v1, kernel version 3.2.1] rtnetlink workaround around
the skb buff size issue

2012/2/6 Rose, Gregory V [off-list ref]:

quoted

The patch below is what I've got so far.  Right now the bit mask array

is global so if you enable display of VF (n) on one interface it will
enable display of the same VF on other interfaces.  I intend to move the
bit mask array into the net_device structure so we can set the display
mask for each interface independently.

quoted

The command to set the filter mask is "set only", I see no reason to add

it to the info dump.  If other folks see it differently then I can do that
too.

quoted

Anyway, it will allow the user to control which VFs are getting

displayed during the info dump.  They all default to off so initially no
VF info gets displayed.

quoted

I've also whipped up a patch for the iproute2 ip command.  It'll work

like this:

quoted

'ip link set <dev> vf (n) filter [on|off]'

So if you have 128 VFs on the device you could enable info dumps for

arbitrary VFs, e.g. VFs 3, 9, 16, 21, and 31.  Only the info for those VFs
would display.  This method has the advantage of not breaking scripts
which parse the current VF info display.  Of course, one could also script
up something to sequentially enable the display of a single VF, dump the
info for it, and then move on to the next.
As this patch will allow one to filter some information and possible
lower the need on skb buffer size, the general idea is ok. On the
other hand it will not eliminate the problem. e.g.:
- assume that one didn't know the limits behind it and put all options
enabled
- it also doesn't fix the need to fill relevant info by interface
bigger than buffer size, e.g. my macvlan interface mac address list.
If I try to request for it, it will eventually fail with a lot of
records even with filtering...

So I would rather see a proper general method for requesting some
information in cycles inside the single interface like sending request
to kernel per VF for particular device or per MAC address from macvlan
associated lists. This approach is I believe slightly more scalable as
it can be potentially reused on other types of network devices as
well.

My original idea was to have these methods:
1st kernel method will return some info about the absolute number of
cycles needed per given interface - this can be done in standard
GETLINK operation with some associated IFLA_* value.
2nd user-space method will based on that number sends netlink request
per records or reasonable page of records (e.g. 10) and parse the
output in the user-space.
 - this is needed to overcome another issue when kernel generates so
many netlink messages with NLM_F_MULTI that netlink socket will not be
able to hold and further write/read code will fail.
3rd kernel method, which will allocated, fill, and send required info
per record -> this one can be done by ops/command netlink association
(in my proposal it is DEVDUMP)

To sum up, I believe that both approaches (using cycles and filtering)
should be allowed to coexists in kernel, but they should be considered
separately as they are doing different jobs.

Stefan,

That is exactly my approach.  We currently have a *bug* in the kernel that this patch is addressing.  The kernel is attempting to provide too much information for the netlink interface to handle and it's breaking things.  So what I want to do is fix the immediate problem while still providing a way for folks to get the information they need.  I've accomplished this by doing exactly what Dave asked me to do, provide a filter that defaults to off and then provide a way for the user to request discrete chunks of information in the dump that won't exceed the netlink buffer limits.

The patch is fairly unobtrusive and simple to understand.

I appreciate that it doesn't do all that you'd like to see done and I see no reason why you couldn't go on and develop the extended features that you would like to see, correct?  There's nothing in my patch that would prevent that so far as I can tell, although I'm not that familiar with your requirements or proposals yet.

- Greg

I can provide whole example of code (not just rtnetlink part), if the
list is interested to see live example.

2012/2/6 Rose, Gregory V [off-list ref]:

That is exactly my approach.  We currently have a *bug* in the kernel that this patch is addressing.  The kernel is attempting to provide too much information for the netlink interface to handle and it's breaking things.  So what I want to do is fix the immediate problem while still providing a way for folks to get the information they need.  I've accomplished this by doing exactly what Dave asked me to do, provide a filter that defaults to off and then provide a way for the user to request discrete chunks of information in the dump that won't exceed the netlink buffer limits.

The patch is fairly unobtrusive and simple to understand.

I appreciate that it doesn't do all that you'd like to see done and I see no reason why you couldn't go on and develop the extended features that you would like to see, correct?  There's nothing in my patch that would prevent that so far as I can tell, although I'm not that familiar with your requirements or proposals yet.

Greg, your patch is completely ok for filtering. I like that thing. I
am just stating that it doesn't eliminate every possible option that
can happen so I believe we should also have method for using cycles -
that's what my patch is doing. So I believe both approaches should be
applied.

-----Original Message-----
From: steweg@gmail.com [mailto:steweg@gmail.com] On Behalf Of Štefan Gula
Sent: Monday, February 06, 2012 10:32 AM
To: Rose, Gregory V
Cc: David Miller; linux-kernel@vger.kernel.org; netdev@vger.kernel.org
Subject: Re: [patch v1, kernel version 3.2.1] rtnetlink workaround around
the skb buff size issue

2012/2/6 Rose, Gregory V [off-list ref]:

quoted

That is exactly my approach.  We currently have a *bug* in the kernel

that this patch is addressing.  The kernel is attempting to provide too
much information for the netlink interface to handle and it's breaking
things.  So what I want to do is fix the immediate problem while still
providing a way for folks to get the information they need.  I've
accomplished this by doing exactly what Dave asked me to do, provide a
filter that defaults to off and then provide a way for the user to request
discrete chunks of information in the dump that won't exceed the netlink
buffer limits.

quoted

The patch is fairly unobtrusive and simple to understand.

I appreciate that it doesn't do all that you'd like to see done and I

see no reason why you couldn't go on and develop the extended features
that you would like to see, correct?  There's nothing in my patch that
would prevent that so far as I can tell, although I'm not that familiar
with your requirements or proposals yet.
Greg, your patch is completely ok for filtering. I like that thing. I
am just stating that it doesn't eliminate every possible option that
can happen so I believe we should also have method for using cycles -
that's what my patch is doing. So I believe both approaches should be
applied.

OK, good deal.  I'll go ahead and finish up my kernel patch and the associated iproute2 patch.

Thanks,

- Greg

2012/2/6 Eric Dumazet [off-list ref]:

Le lundi 06 février 2012 à 10:15 -0500, David Miller a écrit :

quoted

From: Štefan Gula <redacted>
Date: Mon, 6 Feb 2012 09:53:28 +0100

quoted

If I try to request for it, it will eventually fail with a lot of
records even with filtering...

Then the user can loop increasing the buffer size until the netlink
request succeeds.

It is not a problem.

Actually we always truncate message in netlink_recvmsg()

We could use a MSG_NOPARTIAL flag in netlink_recvmsg() so that user can
avoid the MSG_PEEK operation to fetch next message length.

(Ie not consume/copy skb if user buffer is too small to hold full
message, and only return the needed length)

Not sure if this will work. I tried to implement this by the way of
sending one request from user-space to kernel and using NLM_F_MULTI
messages per record to receive the data back from kernel. The problem
was that if I went somewhere beyond 700 messages/records. I get EAGAIN
error code from kernel while trying to write to netlink socket. On the
other hand iproute code was getting error on recvmsg() that buffer is
full. The messages was only 40B long so they should always be able to
fit the 16k buffer used. So I end up with not being able to write nor
read from the socket -> not really sure why. If I introduce paging to
this, so kernel will put only limited number of records (in my case it
was 10) per one request and wait for another request message to
continue... this approach has done job for me. So maybe a good thing
here would be to post the whole code, including rtnetlink part,
macvlan part, iproute part and let you guys check, if you want. Do you
agree?

On Mon, 2012-02-06 at 04:41 +0000, Rose, Gregory V wrote:
[...]

quoted

This is not how we're going to fix this.  I already stated the desired
way to fix this, which is to make the existing dump request have a way
for the requestor to enable extended parts of the device dump.

This is just like netlink diag socket dumps, where the dump request
specifies what the user wants to see.

In this case we'd add a netlink attribute to the dump request which
is just a u32 bitmask or similar.

The Intel engineer who added the VF dump support said he would work on
this fix so why don't you just wait patiently for him to do the work?

The patch below is what I've got so far.  Right now the bit mask array
is global so if you enable display of VF (n) on one interface it will
enable display of the same VF on other interfaces.  I intend to move
the bit mask array into the net_device structure so we can set the
display mask for each interface independently.

I don't think this can work.  Using an application that requests VF
information and uses large buffers (e.g. the updated 'ip') will break
another application that doesn't (e.g. current Network Manager), won't
it?

The filter should be per-request and not persistent (and I think it
could just be a boolean or a limit value rather than a bitmask).

The command to set the filter mask is "set only", I see no reason to
add it to the info dump.  If other folks see it differently then I can
do that too.

Anyway, it will allow the user to control which VFs are getting
displayed during the info dump.  They all default to off so initially
no VF info gets displayed.

I've also whipped up a patch for the iproute2 ip command.  It'll work
like this:

'ip link set <dev> vf (n) filter [on|off]'

Well there's no need for a persistent filter.  And I think that the
default behaviour of 'ip' should be to show all the VFs, as it does now.

[...]

[...]

This array is 8 times too large; use BITS_TO_LONGS.

Ben.

-- 
Ben Hutchings, Staff 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.

-----Original Message-----
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Sent: Monday, February 06, 2012 3:51 PM
To: Rose, Gregory V
Cc: David Miller; steweg@ynet.sk; linux-kernel@vger.kernel.org;
netdev@vger.kernel.org
Subject: RE: [patch v1, kernel version 3.2.1] rtnetlink workaround around
the skb buff size issue

On Mon, 2012-02-06 at 04:41 +0000, Rose, Gregory V wrote:
[...]

quoted

quoted

This is not how we're going to fix this.  I already stated the desired
way to fix this, which is to make the existing dump request have a way
for the requestor to enable extended parts of the device dump.

This is just like netlink diag socket dumps, where the dump request
specifies what the user wants to see.

In this case we'd add a netlink attribute to the dump request which
is just a u32 bitmask or similar.

The Intel engineer who added the VF dump support said he would work on
this fix so why don't you just wait patiently for him to do the work?

The patch below is what I've got so far.  Right now the bit mask array
is global so if you enable display of VF (n) on one interface it will
enable display of the same VF on other interfaces.  I intend to move
the bit mask array into the net_device structure so we can set the
display mask for each interface independently.

I don't think this can work.  Using an application that requests VF
information and uses large buffers (e.g. the updated 'ip') will break
another application that doesn't (e.g. current Network Manager), won't
it?

It's my understanding that the problem isn't with the application buffer size but with the kernel buffer size, which is limited to a page.

The filter should be per-request and not persistent (and I think it
could just be a boolean or a limit value rather than a bitmask).

Why?

I think having the ability to specify one or more discrete VFs for the info dump is useful.

quoted

The command to set the filter mask is "set only", I see no reason to
add it to the info dump.  If other folks see it differently then I can
do that too.

Anyway, it will allow the user to control which VFs are getting
displayed during the info dump.  They all default to off so initially
no VF info gets displayed.

I've also whipped up a patch for the iproute2 ip command.  It'll work
like this:

'ip link set <dev> vf (n) filter [on|off]'

Well there's no need for a persistent filter.  And I think that the
default behaviour of 'ip' should be to show all the VFs, as it does now.

Dave asked me to make the default behavior to not show the VFs.  Take it up with him.

[...]

quoted

--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

@@ -62,6 +62,9 @@ struct rtnl_link {
 static DEFINE_MUTEX(rtnl_mutex);
 static u16 min_ifinfo_dump_size;

+/* VF info display filter - Number of VFs max is 256 */
+static unsigned long show_vfinfo_filter[256 / sizeof(unsigned long)];

[...]

This array is 8 times too large; use BITS_TO_LONGS.

Oops... yeah, that'll be fixed in the actual patch.

- Greg

On Tue, 2012-02-07 at 00:13 +0000, Rose, Gregory V wrote:

quoted

-----Original Message-----
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Sent: Monday, February 06, 2012 3:51 PM
To: Rose, Gregory V
Cc: David Miller; steweg@ynet.sk; linux-kernel@vger.kernel.org;
netdev@vger.kernel.org
Subject: RE: [patch v1, kernel version 3.2.1] rtnetlink workaround around
the skb buff size issue

On Mon, 2012-02-06 at 04:41 +0000, Rose, Gregory V wrote:
[...]

quoted

quoted

This is not how we're going to fix this.  I already stated the desired
way to fix this, which is to make the existing dump request have a way
for the requestor to enable extended parts of the device dump.

This is just like netlink diag socket dumps, where the dump request
specifies what the user wants to see.

In this case we'd add a netlink attribute to the dump request which
is just a u32 bitmask or similar.

The Intel engineer who added the VF dump support said he would work on
this fix so why don't you just wait patiently for him to do the work?

The patch below is what I've got so far.  Right now the bit mask array
is global so if you enable display of VF (n) on one interface it will
enable display of the same VF on other interfaces.  I intend to move
the bit mask array into the net_device structure so we can set the
display mask for each interface independently.

I don't think this can work.  Using an application that requests VF
information and uses large buffers (e.g. the updated 'ip') will break
another application that doesn't (e.g. current Network Manager), won't
it?

It's my understanding that the problem isn't with the application
buffer size but with the kernel buffer size, which is limited to a
page.

[...]

Then it's no wonder you're going about this the wrong way.

Ben.

-- 
Ben Hutchings, Staff 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.

From: Ben Hutchings <redacted>
Date: Tue, 7 Feb 2012 18:26:32 +0000

On Tue, 2012-02-07 at 00:13 +0000, Rose, Gregory V wrote:

quoted

quoted

-----Original Message-----
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Sent: Monday, February 06, 2012 3:51 PM
To: Rose, Gregory V
Cc: David Miller; steweg@ynet.sk; linux-kernel@vger.kernel.org;
netdev@vger.kernel.org
Subject: RE: [patch v1, kernel version 3.2.1] rtnetlink workaround around
the skb buff size issue

On Mon, 2012-02-06 at 04:41 +0000, Rose, Gregory V wrote:
[...]

quoted

quoted

This is not how we're going to fix this.  I already stated the desired
way to fix this, which is to make the existing dump request have a way
for the requestor to enable extended parts of the device dump.

This is just like netlink diag socket dumps, where the dump request
specifies what the user wants to see.

In this case we'd add a netlink attribute to the dump request which
is just a u32 bitmask or similar.

The Intel engineer who added the VF dump support said he would work on
this fix so why don't you just wait patiently for him to do the work?

The patch below is what I've got so far.  Right now the bit mask array
is global so if you enable display of VF (n) on one interface it will
enable display of the same VF on other interfaces.  I intend to move
the bit mask array into the net_device structure so we can set the
display mask for each interface independently.

I don't think this can work.  Using an application that requests VF
information and uses large buffers (e.g. the updated 'ip') will break
another application that doesn't (e.g. current Network Manager), won't
it?

It's my understanding that the problem isn't with the application
buffer size but with the kernel buffer size, which is limited to a
page.

[...]

Then it's no wonder you're going about this the wrong way.

It is the userland buffer size that is the problem, userland will allocate
max(8192, PAGE_SIZE) for it's recvmsg() call, that's why we have to only
dump VF's and other potentially large objects when the user enables it in
the request.

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of David Miller
Sent: Tuesday, February 07, 2012 10:32 AM
To: bhutchings@solarflare.com
Cc: Rose, Gregory V; steweg@ynet.sk; linux-kernel@vger.kernel.org;
netdev@vger.kernel.org
Subject: Re: [patch v1, kernel version 3.2.1] rtnetlink workaround around
the skb buff size issue

From: Ben Hutchings <redacted>
Date: Tue, 7 Feb 2012 18:26:32 +0000

quoted

On Tue, 2012-02-07 at 00:13 +0000, Rose, Gregory V wrote:

quoted

quoted

-----Original Message-----
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Sent: Monday, February 06, 2012 3:51 PM
To: Rose, Gregory V
Cc: David Miller; steweg@ynet.sk; linux-kernel@vger.kernel.org;
netdev@vger.kernel.org
Subject: RE: [patch v1, kernel version 3.2.1] rtnetlink workaround

around

quoted

quoted

quoted

the skb buff size issue

On Mon, 2012-02-06 at 04:41 +0000, Rose, Gregory V wrote:
[...]

quoted

quoted

This is not how we're going to fix this.  I already stated the

desired

quoted

quoted

quoted

quoted

quoted

way to fix this, which is to make the existing dump request have

a way

quoted

quoted

quoted

quoted

quoted

for the requestor to enable extended parts of the device dump.

This is just like netlink diag socket dumps, where the dump

request

quoted

quoted

quoted

quoted

quoted

specifies what the user wants to see.

In this case we'd add a netlink attribute to the dump request

which

quoted

quoted

quoted

quoted

quoted

is just a u32 bitmask or similar.

The Intel engineer who added the VF dump support said he would

work on

quoted

quoted

quoted

quoted

quoted

this fix so why don't you just wait patiently for him to do the

work?

quoted

quoted

quoted

quoted

The patch below is what I've got so far.  Right now the bit mask

array

quoted

quoted

quoted

quoted

is global so if you enable display of VF (n) on one interface it

will

quoted

quoted

quoted

quoted

enable display of the same VF on other interfaces.  I intend to

move

quoted

quoted

quoted

quoted

the bit mask array into the net_device structure so we can set the
display mask for each interface independently.

I don't think this can work.  Using an application that requests VF
information and uses large buffers (e.g. the updated 'ip') will break
another application that doesn't (e.g. current Network Manager),

won't

quoted

quoted

quoted

it?

It's my understanding that the problem isn't with the application
buffer size but with the kernel buffer size, which is limited to a
page.

[...]

Then it's no wonder you're going about this the wrong way.

It is the userland buffer size that is the problem, userland will allocate
max(8192, PAGE_SIZE) for it's recvmsg() call, that's why we have to only
dump VF's and other potentially large objects when the user enables it in
the request.

Ah well, when that was stated before I was under the impression it was in the kernel.  My bad...

Anyway, if Ben would like to do this work it's fine by me.  He seems to have some very definite opinions about this and I don't feel like going through another several weeks of fighting about it him.  I've got a patch ready to submit today that goes along the lines of what I've already posted but with a few clean ups.  I'll post it, if it gets shot down then fine, I'll get on with my other work.

- Greg

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html