[ethtool PATCH] ethtool: Support n-tuple filter programming

[ethtool PATCH] ethtool: Support n-tuple filter programming

[Jeff Kirsher]

From: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>

Program underlying ethernet devices with n-tuple flow classification
filters.

This also adds a new flag to ethtool_flags, allowing n-tuple
programming to be toggled using the set_flags call.

Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 ethtool-copy.h |   35 +++++++++++++
 ethtool.c      |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 186 insertions(+), 5 deletions(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index d366c3a..2681cd8 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -241,6 +241,7 @@ enum ethtool_stringset {
 	ETH_SS_TEST		= 0,
 	ETH_SS_STATS,
 	ETH_SS_PRIV_FLAGS,
+	ETH_SS_NTUPLE_FILTERS,
 };
 
 /* for passing string sets for data tagging */
@@ -289,6 +290,7 @@ struct ethtool_perm_addr {
  */
 enum ethtool_flags {
 	ETH_FLAG_LRO		= (1 << 15),	/* LRO is enabled */
+	ETH_FLAG_NTUPLE		= (1 << 27),	/* N-tuple filters enabled */
 };
 
 /* The following structures are for supporting RX network flow
@@ -374,6 +376,36 @@ struct ethtool_flash {
 	char	data[ETHTOOL_FLASH_MAX_FILENAME];
 };
 
+struct ethtool_rx_ntuple_flow_spec {
+	int		flow_type;
+	union {
+		struct ethtool_tcpip4_spec		tcp_ip4_spec;
+		struct ethtool_tcpip4_spec		udp_ip4_spec;
+		struct ethtool_tcpip4_spec		sctp_ip4_spec;
+		struct ethtool_ah_espip4_spec		ah_ip4_spec;
+		struct ethtool_ah_espip4_spec		esp_ip4_spec;
+		struct ethtool_rawip4_spec		raw_ip4_spec;
+		struct ethtool_ether_spec		ether_spec;
+		struct ethtool_usrip4_spec		usr_ip4_spec;
+		__u8					hdata[64];
+	} h_u, m_u; /* entry, mask */
+
+	__u16	        vlan_tag;
+	__u16	        vlan_tag_mask;
+	__u64		data;      /* user-defined flow spec data */
+	__u64		data_mask; /* user-defined flow spec mask */
+
+	/* signed to distinguish between queue and actions (DROP) */
+	int		action;
+#define ETHTOOL_RXNTUPLE_ACTION_DROP -1
+};
+
+#define ETHTOOL_MAX_NTUPLE_LIST_ENTRY 1024
+#define ETHTOOL_MAX_NTUPLE_STRING_PER_ENTRY 14
+struct ethtool_rx_ntuple {
+	__u32					cmd;
+	struct ethtool_rx_ntuple_flow_spec	fs;
+};
 
 /* CMDs currently supported */
 #define ETHTOOL_GSET		0x00000001 /* Get settings. */
@@ -431,6 +463,9 @@ struct ethtool_flash {
 #define	ETHTOOL_FLASHDEV	0x00000033 /* Flash firmware to device */
 #define	ETHTOOL_RESET		0x00000034 /* Reset hardware */
 
+#define ETHTOOL_SRXNTUPLE	0x00000035 /* Add an n-tuple filter to device */
+#define ETHTOOL_GRXNTUPLE	0x00000036 /* Get n-tuple filters from device */
+
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
 #define SPARC_ETH_SSET		ETHTOOL_SSET
diff --git a/ethtool.c b/ethtool.c
index 10ff1f1..fc9e419 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -78,6 +78,8 @@ static char *unparse_rxfhashopts(u64 opts);
 static int dump_rxfhash(int fhash, u64 val);
 static int do_srxclass(int fd, struct ifreq *ifr);
 static int do_grxclass(int fd, struct ifreq *ifr);
+static int do_srxntuple(int fd, struct ifreq *ifr);
+static int do_grxntuple(int fd, struct ifreq *ifr);
 static int do_flash(int fd, struct ifreq *ifr);
 static int send_ioctl(int fd, struct ifreq *ifr);
 
@@ -103,6 +105,8 @@ static enum {
 	MODE_GSTATS,
 	MODE_GNFC,
 	MODE_SNFC,
+	MODE_SNTUPLE,
+	MODE_GNTUPLE,
 	MODE_FLASHDEV,
 } mode = MODE_GSET;
 
@@ -168,6 +172,7 @@ static struct option {
 		"		[ gso on|off ]\n"
 		"		[ gro on|off ]\n"
 		"		[ lro on|off ]\n"
+		"		[ ntuple on|off ]\n"
     },
     { "-i", "--driver", MODE_GDRV, "Show driver information" },
     { "-d", "--register-dump", MODE_GREGS, "Do a register dump",
@@ -199,6 +204,16 @@ static struct option {
 		"classification options",
 		"		[ rx-flow-hash tcp4|udp4|ah4|sctp4|"
 		"tcp6|udp6|ah6|sctp6 m|v|t|s|d|f|n|r... ]\n" },
+    { "-U", "--config-ntuple", MODE_SNTUPLE, "Configure Rx ntuple filters "
+		"and actions",
+		"               [ flow-type tcp4|udp4|sctp4 src-ip <addr> "
+		"src-ip-mask <mask> dst-ip <addr> dst-ip-mask <mask> "
+		"src-port <port> src-port-mask <mask> dst-port <port> "
+		"dst-port-mask <mask> vlan <VLAN tag> vlan-mask <mask> "
+		"user-def <data> user-def-mask <mask> "
+		"action <queue or drop>\n" },
+    { "-u", "--show-ntuple", MODE_GNTUPLE,
+		"Get Rx ntuple filters and actions\n" },
     { "-h", "--help", MODE_HELP, "Show this help" },
     {}
 };
@@ -241,6 +256,7 @@ static int off_ufo_wanted = -1;
 static int off_gso_wanted = -1;
 static int off_lro_wanted = -1;
 static int off_gro_wanted = -1;
+static int off_ntuple_wanted = -1;
 
 static struct ethtool_pauseparam epause;
 static int gpause_changed = 0;
@@ -312,6 +328,8 @@ static int rx_fhash_get = 0;
 static int rx_fhash_set = 0;
 static u32 rx_fhash_val = 0;
 static int rx_fhash_changed = 0;
+static int sntuple_changed = 0;
+static struct ethtool_rx_ntuple_flow_spec ntuple_fs;
 static char *flash_file = NULL;
 static int flash = -1;
 static int flash_region = -1;
@@ -363,6 +381,7 @@ static struct cmdline_info cmdline_offload[] = {
 	{ "gso", CMDL_BOOL, &off_gso_wanted, NULL },
 	{ "lro", CMDL_BOOL, &off_lro_wanted, NULL },
 	{ "gro", CMDL_BOOL, &off_gro_wanted, NULL },
+	{ "ntuple", CMDL_BOOL, &off_ntuple_wanted, NULL },
 };
 
 static struct cmdline_info cmdline_pause[] = {
@@ -403,6 +422,22 @@ static struct cmdline_info cmdline_coalesce[] = {
 	{ "tx-frames-high", CMDL_INT, &coal_tx_frames_high_wanted, &ecoal.tx_max_coalesced_frames_high },
 };
 
+static struct cmdline_info cmdline_ntuple[] = {
+	{ "src-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4src, NULL },
+	{ "src-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4src, NULL },
+	{ "dst-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4dst, NULL },
+	{ "dst-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4dst, NULL },
+	{ "src-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.psrc, NULL },
+	{ "src-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.psrc, NULL },
+	{ "dst-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.pdst, NULL },
+	{ "dst-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.pdst, NULL },
+	{ "vlan", CMDL_INT, &ntuple_fs.vlan_tag, NULL },
+	{ "vlan-mask", CMDL_UINT, &ntuple_fs.vlan_tag_mask, NULL },
+	{ "user-def", CMDL_INT, &ntuple_fs.data, NULL },
+	{ "user-def-mask", CMDL_UINT, &ntuple_fs.data_mask, NULL },
+	{ "action", CMDL_INT, &ntuple_fs.action, NULL },
+};
+
 static int get_int(char *str, int base)
 {
 	long v;
@@ -544,6 +579,8 @@ static void parse_cmdline(int argc, char **argp)
 			    (mode == MODE_GSTATS) ||
 			    (mode == MODE_GNFC) ||
 			    (mode == MODE_SNFC) ||
+			    (mode == MODE_SNTUPLE) ||
+			    (mode == MODE_GNTUPLE) ||
 			    (mode == MODE_PHYS_ID) ||
 			    (mode == MODE_FLASHDEV)) {
 				devname = argp[i];
@@ -626,6 +663,27 @@ static void parse_cmdline(int argc, char **argp)
 				i = argc;
 				break;
 			}
+			if (mode == MODE_SNTUPLE) {
+				if (!strcmp(argp[i], "flow-type")) {
+					i += 1;
+					if (i >= argc) {
+						show_usage(1);
+						break;
+					}
+					ntuple_fs.flow_type =
+					            rxflow_str_to_type(argp[i]);
+					i += 1;
+					parse_generic_cmdline(argc, argp, i,
+						&sntuple_changed,
+						cmdline_ntuple,
+						ARRAY_SIZE(cmdline_ntuple));
+					i = argc;
+					break;
+				} else {
+					show_usage(1);
+				}
+				break;
+			}
 			if (mode == MODE_GNFC) {
 				if (!strcmp(argp[i], "rx-flow-hash")) {
 					i += 1;
@@ -1468,7 +1526,7 @@ static int dump_coalesce(void)
 }
 
 static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso,
-			int gro, int lro)
+			int gro, int lro, int ntuple)
 {
 	fprintf(stdout,
 		"rx-checksumming: %s\n"
@@ -1478,7 +1536,8 @@ static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso,
 		"udp-fragmentation-offload: %s\n"
 		"generic-segmentation-offload: %s\n"
 		"generic-receive-offload: %s\n"
-		"large-receive-offload: %s\n",
+		"large-receive-offload: %s\n"
+		"ntuple-filters: %s\n",
 		rx ? "on" : "off",
 		tx ? "on" : "off",
 		sg ? "on" : "off",
@@ -1486,7 +1545,8 @@ static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso,
 		ufo ? "on" : "off",
 		gso ? "on" : "off",
 		gro ? "on" : "off",
-		lro ? "on" : "off");
+		lro ? "on" : "off",
+		ntuple ? "on" : "off");
 
 	return 0;
 }
@@ -1590,6 +1650,10 @@ static int doit(void)
 		return do_grxclass(fd, &ifr);
 	} else if (mode == MODE_SNFC) {
 		return do_srxclass(fd, &ifr);
+	} else if (mode == MODE_SNTUPLE) {
+		return do_srxntuple(fd, &ifr);
+	} else if (mode == MODE_GNTUPLE) {
+		return do_grxntuple(fd, &ifr);
 	} else if (mode == MODE_FLASHDEV) {
 		return do_flash(fd, &ifr);
 	}
@@ -1799,7 +1863,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 {
 	struct ethtool_value eval;
 	int err, allfail = 1, rx = 0, tx = 0, sg = 0;
-	int tso = 0, ufo = 0, gso = 0, gro = 0, lro = 0;
+	int tso = 0, ufo = 0, gso = 0, gro = 0, lro = 0, ntuple = 0;
 
 	fprintf(stdout, "Offload parameters for %s:\n", devname);
 
@@ -1870,6 +1934,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 		perror("Cannot get device flags");
 	} else {
 		lro = (eval.data & ETH_FLAG_LRO) != 0;
+		ntuple = (eval.data & ETH_FLAG_NTUPLE) != 0;
 		allfail = 0;
 	}
 
@@ -1888,7 +1953,7 @@ static int do_goffload(int fd, struct ifreq *ifr)
 		return 83;
 	}
 
-	return dump_offload(rx, tx, sg, tso, ufo, gso, gro, lro);
+	return dump_offload(rx, tx, sg, tso, ufo, gso, gro, lro, ntuple);
 }
 
 static int do_soffload(int fd, struct ifreq *ifr)
@@ -1999,6 +2064,29 @@ static int do_soffload(int fd, struct ifreq *ifr)
 			return 93;
 		}
 	}
+	if (off_ntuple_wanted >= 0) {
+		changed = 1;
+		eval.cmd = ETHTOOL_GFLAGS;
+		eval.data = 0;
+		ifr->ifr_data = (caddr_t)&eval;
+		err = ioctl(fd, SIOCETHTOOL, ifr);
+		if (err) {
+			perror("Cannot get device flag settings");
+			return 91;
+		}
+
+		eval.cmd = ETHTOOL_SFLAGS;
+		if (off_ntuple_wanted == 1)
+			eval.data |= ETH_FLAG_NTUPLE;
+		else
+			eval.data &= ~ETH_FLAG_NTUPLE;
+
+		err = ioctl(fd, SIOCETHTOOL, ifr);
+		if (err) {
+			perror("Cannot set n-tuple filter settings");
+			return 93;
+		}
+	}
 
 	if (!changed) {
 		fprintf(stdout, "no offload settings changed\n");
@@ -2545,6 +2633,64 @@ static int do_flash(int fd, struct ifreq *ifr)
 	return err;
 }
 
+static int do_srxntuple(int fd, struct ifreq *ifr)
+{
+	int err;
+
+	if (sntuple_changed) {
+		struct ethtool_rx_ntuple ntuplecmd;
+
+		ntuplecmd.cmd = ETHTOOL_SRXNTUPLE;
+		memcpy(&ntuplecmd.fs, &ntuple_fs,
+		       sizeof(struct ethtool_rx_ntuple_flow_spec));
+
+		ifr->ifr_data = (caddr_t)&ntuplecmd;
+		err = ioctl(fd, SIOCETHTOOL, ifr);
+		if (err < 0)
+			perror("Cannot add new RX n-tuple filter");
+	} else {
+		show_usage(1);
+	}
+
+	return 0;
+}
+
+static int do_grxntuple(int fd, struct ifreq *ifr)
+{
+	struct ethtool_gstrings *strings;
+	int sz_str, n_strings, err, i;
+
+	n_strings = ETHTOOL_MAX_NTUPLE_LIST_ENTRY *
+	            ETHTOOL_MAX_NTUPLE_STRING_PER_ENTRY;
+	sz_str = n_strings * ETH_GSTRING_LEN;
+
+	strings = calloc(1, sz_str + sizeof(struct ethtool_gstrings));
+	if (!strings) {
+		fprintf(stderr, "no memory available\n");
+		return 95;
+	}
+
+	strings->cmd = ETHTOOL_GRXNTUPLE;
+	strings->string_set = ETH_SS_NTUPLE_FILTERS;
+	strings->len = n_strings;
+	ifr->ifr_data = (caddr_t) strings;
+	err = send_ioctl(fd, ifr);
+	if (err < 0) {
+		perror("Cannot get Rx n-tuple information");
+		free(strings);
+		return 100;
+	}
+
+	n_strings = strings->len;
+	fprintf(stdout, "Rx n-tuple filters:\n");
+	for (i = 0; i < n_strings; i++)
+		fprintf(stdout, "%s", &strings->data[i * ETH_GSTRING_LEN]);
+
+	free(strings);
+
+	return 0;
+}
+
 static int send_ioctl(int fd, struct ifreq *ifr)
 {
 	return ioctl(fd, SIOCETHTOOL, ifr);

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Jeff Garzik]

On 02/04/2010 02:51 AM, Jeff Kirsher wrote:
> From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel.com>
>
> Program underlying ethernet devices with n-tuple flow classification
> filters.
>
> This also adds a new flag to ethtool_flags, allowing n-tuple
> programming to be toggled using the set_flags call.
>
> Signed-off-by: Peter P Waskiewicz Jr<peter.p.waskiewicz.jr@intel.com>
> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
> ---
>
>   ethtool-copy.h |   35 +++++++++++++
>   ethtool.c      |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 186 insertions(+), 5 deletions(-)

applied, but two problems remain:

1) you failed to document this in the man page.  I will expect a patch 
to ethtool.8.

2) you introduced a deviation from the upstream kernel ethtool.h:


--- ethtool-copy.h      2010-02-24 22:39:21.000000000 -0500
+++ ../net-next-2.6/include/linux/ethtool.h     2010-02-24 
22:14:43.000000000 -0500
@@ -389,8 +389,6 @@
  #define ETHTOOL_RXNTUPLE_ACTION_DROP -1
  };

-#define ETHTOOL_MAX_NTUPLE_LIST_ENTRY 1024
-#define ETHTOOL_MAX_NTUPLE_STRING_PER_ENTRY 14
  struct ethtool_rx_ntuple {
         __u32                                   cmd;
         struct ethtool_rx_ntuple_flow_spec      fs;

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Waskiewicz Jr, Peter P]

On Wed, 24 Feb 2010, Jeff Garzik wrote:

> On 02/04/2010 02:51 AM, Jeff Kirsher wrote:
> > From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel.com>
> >
> > Program underlying ethernet devices with n-tuple flow classification
> > filters.
> >
> > This also adds a new flag to ethtool_flags, allowing n-tuple
> > programming to be toggled using the set_flags call.
> >
> > Signed-off-by: Peter P Waskiewicz Jr<peter.p.waskiewicz.jr@intel.com>
> > Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
> > ---
> >
> >   ethtool-copy.h |   35 +++++++++++++
> >   ethtool.c      |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >   2 files changed, 186 insertions(+), 5 deletions(-)
> 
> applied, but two problems remain:
> 
> 1) you failed to document this in the man page.  I will expect a patch 
> to ethtool.8.

And you shall have it shortly.  My bad.

> 
> 2) you introduced a deviation from the upstream kernel ethtool.h:

I see what happened.  We moved those two entries in ethtool.h into the 
#ifdef __KERNEL__ section.  I can create a patch for the kernel ethtool.h 
to move them back to match userspace if that is what folks want.

> 
> 
> --- ethtool-copy.h      2010-02-24 22:39:21.000000000 -0500
> +++ ../net-next-2.6/include/linux/ethtool.h     2010-02-24 
> 22:14:43.000000000 -0500
> @@ -389,8 +389,6 @@
>   #define ETHTOOL_RXNTUPLE_ACTION_DROP -1
>   };
> 
> -#define ETHTOOL_MAX_NTUPLE_LIST_ENTRY 1024
> -#define ETHTOOL_MAX_NTUPLE_STRING_PER_ENTRY 14
>   struct ethtool_rx_ntuple {
>          __u32                                   cmd;
>          struct ethtool_rx_ntuple_flow_spec      fs;
> 
> 
> 

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Jeff Garzik]

On 02/25/2010 02:04 AM, Waskiewicz Jr, Peter P wrote:
> On Wed, 24 Feb 2010, Jeff Garzik wrote:
>
>> On 02/04/2010 02:51 AM, Jeff Kirsher wrote:
>>> From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel.com>
>>>
>>> Program underlying ethernet devices with n-tuple flow classification
>>> filters.
>>>
>>> This also adds a new flag to ethtool_flags, allowing n-tuple
>>> programming to be toggled using the set_flags call.
>>>
>>> Signed-off-by: Peter P Waskiewicz Jr<peter.p.waskiewicz.jr@intel.com>
>>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>> ---
>>>
>>>    ethtool-copy.h |   35 +++++++++++++
>>>    ethtool.c      |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>    2 files changed, 186 insertions(+), 5 deletions(-)
>>
>> applied, but two problems remain:
>>
>> 1) you failed to document this in the man page.  I will expect a patch
>> to ethtool.8.
>
> And you shall have it shortly.  My bad.
>
>>
>> 2) you introduced a deviation from the upstream kernel ethtool.h:
>
> I see what happened.  We moved those two entries in ethtool.h into the
> #ifdef __KERNEL__ section.  I can create a patch for the kernel ethtool.h
> to move them back to match userspace if that is what folks want.

One way or another, userspace ethtool-copy.h must match the kernel's, 
and userspace ethtool does not build without those definitions...  They 
are de facto part of the interface at this point, though you are welcome 
to change that if you wish.

	Jeff

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Waskiewicz Jr, Peter P]

On Wed, 24 Feb 2010, Jeff Garzik wrote:

> On 02/04/2010 02:51 AM, Jeff Kirsher wrote:
> > From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel.com>
> >
> > Program underlying ethernet devices with n-tuple flow classification
> > filters.
> >
> > This also adds a new flag to ethtool_flags, allowing n-tuple
> > programming to be toggled using the set_flags call.
> >
> > Signed-off-by: Peter P Waskiewicz Jr<peter.p.waskiewicz.jr@intel.com>
> > Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
> > ---
> >
> >   ethtool-copy.h |   35 +++++++++++++
> >   ethtool.c      |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >   2 files changed, 186 insertions(+), 5 deletions(-)
> 
> applied, but two problems remain:
> 
> 1) you failed to document this in the man page.  I will expect a patch 
> to ethtool.8.
> 
> 2) you introduced a deviation from the upstream kernel ethtool.h:

The way I've come up with to fix this is less intrusive than I want, but I 
think it's right way to do it.  The intent wasn't to have ethtool 
(userspace) enforce how many filters could be returned.  What it should do 
is get the number of strings through drvinfo to make the proper memory 
allocation.

What I need to change is the drvinfo struct in the kernel to include the 
ethtool strings field, returned from get_sset_count().  Then I can pull 
that into userspace and make the correct memory allocation, then call 
get_rx_ntuple().

This will require a small kernel change.  Will something like this be 
pulled in at this point, given that 2.6.33 just released?

-PJ

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Jeff Garzik]

On 02/25/2010 08:49 PM, Waskiewicz Jr, Peter P wrote:
> On Wed, 24 Feb 2010, Jeff Garzik wrote:
>
>> On 02/04/2010 02:51 AM, Jeff Kirsher wrote:
>>> From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel.com>
>>>
>>> Program underlying ethernet devices with n-tuple flow classification
>>> filters.
>>>
>>> This also adds a new flag to ethtool_flags, allowing n-tuple
>>> programming to be toggled using the set_flags call.
>>>
>>> Signed-off-by: Peter P Waskiewicz Jr<peter.p.waskiewicz.jr@intel.com>
>>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>> ---
>>>
>>>    ethtool-copy.h |   35 +++++++++++++
>>>    ethtool.c      |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>    2 files changed, 186 insertions(+), 5 deletions(-)
>>
>> applied, but two problems remain:
>>
>> 1) you failed to document this in the man page.  I will expect a patch
>> to ethtool.8.
>>
>> 2) you introduced a deviation from the upstream kernel ethtool.h:
>
> The way I've come up with to fix this is less intrusive than I want, but I
> think it's right way to do it.  The intent wasn't to have ethtool
> (userspace) enforce how many filters could be returned.  What it should do
> is get the number of strings through drvinfo to make the proper memory
> allocation.
>
> What I need to change is the drvinfo struct in the kernel to include the
> ethtool strings field, returned from get_sset_count().  Then I can pull
> that into userspace and make the correct memory allocation, then call
> get_rx_ntuple().

I would say we need a ETHTOOL_GSTRINGS_COUNT rather than expanding 
drvinfo anymore...  but yeah, expanding drvinfo would work too.


> This will require a small kernel change.  Will something like this be
> pulled in at this point, given that 2.6.33 just released?

The n-tuple stuff is not in 2.6.33's ethtool interface, so it hasn't 
actually made it into a released kernel at this point.

It seems reasonable for 2.6.34 to either add ETHTOOL_GSTRINGS_COUNT or 
update drvinfo.  Even if net-next has already been pulled into 
post-2.6.33 merge window, that would IMO qualify as a reasonable 
interface bugfix to get upstream.  The ethtool n-tuple ABI is not locked 
into stone until 2.6.34 is released by Linus.

	Jeff

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Thu, 25 Feb 2010 21:26:38 -0500

> On 02/25/2010 08:49 PM, Waskiewicz Jr, Peter P wrote:
>> This will require a small kernel change.  Will something like this be
>> pulled in at this point, given that 2.6.33 just released?
> 
> The n-tuple stuff is not in 2.6.33's ethtool interface, so it hasn't
> actually made it into a released kernel at this point.
> 
> It seems reasonable for 2.6.34 to either add ETHTOOL_GSTRINGS_COUNT or
> update drvinfo.  Even if net-next has already been pulled into
> post-2.6.33 merge window, that would IMO qualify as a reasonable
> interface bugfix to get upstream.  The ethtool n-tuple ABI is not
> locked into stone until 2.6.34 is released by Linus.

Right.

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Peter P Waskiewicz Jr]

On Thu, 2010-02-25 at 21:53 -0800, David Miller wrote:
> From: Jeff Garzik <jeff@garzik.org>
> Date: Thu, 25 Feb 2010 21:26:38 -0500
> 
> > On 02/25/2010 08:49 PM, Waskiewicz Jr, Peter P wrote:
> >> This will require a small kernel change.  Will something like this be
> >> pulled in at this point, given that 2.6.33 just released?
> > 
> > The n-tuple stuff is not in 2.6.33's ethtool interface, so it hasn't
> > actually made it into a released kernel at this point.
> > 
> > It seems reasonable for 2.6.34 to either add ETHTOOL_GSTRINGS_COUNT or
> > update drvinfo.  Even if net-next has already been pulled into
> > post-2.6.33 merge window, that would IMO qualify as a reasonable
> > interface bugfix to get upstream.  The ethtool n-tuple ABI is not
> > locked into stone until 2.6.34 is released by Linus.
> 
> Right.

Sounds good.  I wanted to make sure before I changed the ethtool bits.
Expect a patch for both the kernel side and userspace side shortly.

-PJ

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Ben Hutchings]

On Thu, 2010-02-25 at 21:26 -0500, Jeff Garzik wrote:
> On 02/25/2010 08:49 PM, Waskiewicz Jr, Peter P wrote:
[...]
> > This will require a small kernel change.  Will something like this be
> > pulled in at this point, given that 2.6.33 just released?
> 
> The n-tuple stuff is not in 2.6.33's ethtool interface, so it hasn't 
> actually made it into a released kernel at this point.
> 
> It seems reasonable for 2.6.34 to either add ETHTOOL_GSTRINGS_COUNT or 
> update drvinfo.  Even if net-next has already been pulled into 
> post-2.6.33 merge window, that would IMO qualify as a reasonable 
> interface bugfix to get upstream.  The ethtool n-tuple ABI is not locked 
> into stone until 2.6.34 is released by Linus.

Given that, I would really like to see the tuple strings replaced with
binary structures.  Setting them in binary format and retrieving them in
text format is inelegant.  Also we should not assume that ethtool is the
only user of the ethtool API.  If someone wanted to write a GUI to
manage the filter tuples, or a tool that would programmatically
reconfigure filters, they would have to parse the strings.

Even the lookup of number of strings seems to be completely
misconceived.  It's delegated to the driver thus:

        ret = ops->get_sset_count(dev, gstrings.string_set);
        if (ret < 0)
                return ret;

What if gstrings.string_set != ETH_SS_NTUPLE_FILTERS?  Why is that even
a parameter to the operation?

Further, each filter can be formatted as multiple strings.  So is the
driver supposed to know how many strings the ethtool core might
generate?

Ben.

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

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Jeff Garzik]

On 02/26/2010 02:59 PM, Ben Hutchings wrote:
> On Thu, 2010-02-25 at 21:26 -0500, Jeff Garzik wrote:
>> On 02/25/2010 08:49 PM, Waskiewicz Jr, Peter P wrote:
> [...]
>>> This will require a small kernel change.  Will something like this be
>>> pulled in at this point, given that 2.6.33 just released?
>>
>> The n-tuple stuff is not in 2.6.33's ethtool interface, so it hasn't
>> actually made it into a released kernel at this point.
>>
>> It seems reasonable for 2.6.34 to either add ETHTOOL_GSTRINGS_COUNT or
>> update drvinfo.  Even if net-next has already been pulled into
>> post-2.6.33 merge window, that would IMO qualify as a reasonable
>> interface bugfix to get upstream.  The ethtool n-tuple ABI is not locked
>> into stone until 2.6.34 is released by Linus.
>
> Given that, I would really like to see the tuple strings replaced with
> binary structures.  Setting them in binary format and retrieving them in
> text format is inelegant.  Also we should not assume that ethtool is the
> only user of the ethtool API.  If someone wanted to write a GUI to
> manage the filter tuples, or a tool that would programmatically
> reconfigure filters, they would have to parse the strings.
>
> Even the lookup of number of strings seems to be completely
> misconceived.  It's delegated to the driver thus:
>
>          ret = ops->get_sset_count(dev, gstrings.string_set);
>          if (ret<  0)
>                  return ret;
>
> What if gstrings.string_set != ETH_SS_NTUPLE_FILTERS?  Why is that even
> a parameter to the operation?
>
> Further, each filter can be formatted as multiple strings.  So is the
> driver supposed to know how many strings the ethtool core might
> generate?

Speaking _generally_, not specifically about n-tuple filters, text 
strings are much more flexible across varied types of hardware.  For 
example, the "private flags" ethtool interface is intended to be used as 
a generic means for a driver to export a set of hardware-specific 
boolean knobs.

For example, e1000e's SmartPowerDownEnable need not be a module-wide 
parameter applying to all e1000e devices in the system.  ethtool's 
private-flags interface could enable per-net_device runtime setting of 
this Intel-specific feature, simply by exporting a string 
"SmartPowerDownEnable" as a private flag.

Thus, text strings isolate the problem of hardware-specific settings, 
features and structures entirely in the kernel driver itself.

Getting back to n-tuple filters, the question becomes:  how 
hardware-specific are the filter formats?

Using binary structures is generally more efficient...   but efficiency 
is not really a primary goal.  Working across a broad spectrum of 
hardware, future-proof-edness, is more important.  Binary structures may 
lock ethtool users into a very specific filter specification -- one that 
might not be as easily applicable to other vendors' hardware, or even 
future hardware from the same vendor.

procfs and sysfs exist in their current, ASCII-friendly form in part 
because Linus has often observed that text strings are often easier and 
more resilient kernel<->userspace interfaces than binary interfaces.

All that said, I cannot argue that there are elements of the current 
Intel implementation that are quite inelegant, and binary structures may 
simply be easier.

Like everything else in life, it's a balance...   ;-)

	Jeff

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Ben Hutchings]

On Wed, 2010-02-03 at 23:51 -0800, Jeff Kirsher wrote:
> From: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> 
> Program underlying ethernet devices with n-tuple flow classification
> filters.
> 
> This also adds a new flag to ethtool_flags, allowing n-tuple
> programming to be toggled using the set_flags call.

I just noticed a problem with the implementation which makes me wonder
whether this was tested at all:

[...]
> +static struct cmdline_info cmdline_ntuple[] = {
> +	{ "src-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4src, NULL },
> +	{ "src-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4src, NULL },
> +	{ "dst-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4dst, NULL },
> +	{ "dst-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4dst, NULL },
> +	{ "src-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.psrc, NULL },
> +	{ "src-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.psrc, NULL },
> +	{ "dst-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.pdst, NULL },
> +	{ "dst-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.pdst, NULL },
> +	{ "vlan", CMDL_INT, &ntuple_fs.vlan_tag, NULL },
> +	{ "vlan-mask", CMDL_UINT, &ntuple_fs.vlan_tag_mask, NULL },
> +	{ "user-def", CMDL_INT, &ntuple_fs.data, NULL },
> +	{ "user-def-mask", CMDL_UINT, &ntuple_fs.data_mask, NULL },
> +	{ "action", CMDL_INT, &ntuple_fs.action, NULL },
> +};
[...]
> +                       if (mode == MODE_SNTUPLE) {
> +                               if (!strcmp(argp[i], "flow-type")) {
> +                                       i += 1;
> +                                       if (i >= argc) {
> +                                               show_usage(1);
> +                                               break;
> +                                       }
> +                                       ntuple_fs.flow_type =
> +                                                   rxflow_str_to_type(argp[i]);
> +                                       i += 1;
> +                                       parse_generic_cmdline(argc, argp, i,
> +                                               &sntuple_changed,
> +                                               cmdline_ntuple,
> +                                               ARRAY_SIZE(cmdline_ntuple));
> +                                       i = argc;
> +                                       break;
> +                               } else {
> +                                       show_usage(1);
> +                               }
> +                               break;
> +                       }
[...]

parse_generic_cmdline() will write an int for each argument defined with
type CMDL_INT or CMDL_UINT.  But the fields in ntuple_fs are not all of
type int (or even 32-bit) - some of them are 16-bit or 64-bit, and some
of them are big-endian.  I also wonder whether anyone really wants to
enter an IPv4 address as a single integer.

Ben.

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

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Peter P Waskiewicz Jr]

On Mon, 21 Jun 2010, Ben Hutchings wrote:

> On Wed, 2010-02-03 at 23:51 -0800, Jeff Kirsher wrote:
>> From: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
>>
>> Program underlying ethernet devices with n-tuple flow classification
>> filters.
>>
>> This also adds a new flag to ethtool_flags, allowing n-tuple
>> programming to be toggled using the set_flags call.
>
> I just noticed a problem with the implementation which makes me wonder
> whether this was tested at all:

Yes, it was tested.  We didn't hit every corner case, which I think your 
catch below is a corner case issue.  Our hardware can only do so much.

>
> [...]
>> +static struct cmdline_info cmdline_ntuple[] = {
>> +	{ "src-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4src, NULL },
>> +	{ "src-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4src, NULL },
>> +	{ "dst-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4dst, NULL },
>> +	{ "dst-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4dst, NULL },
>> +	{ "src-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.psrc, NULL },
>> +	{ "src-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.psrc, NULL },
>> +	{ "dst-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.pdst, NULL },
>> +	{ "dst-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.pdst, NULL },
>> +	{ "vlan", CMDL_INT, &ntuple_fs.vlan_tag, NULL },
>> +	{ "vlan-mask", CMDL_UINT, &ntuple_fs.vlan_tag_mask, NULL },
>> +	{ "user-def", CMDL_INT, &ntuple_fs.data, NULL },
>> +	{ "user-def-mask", CMDL_UINT, &ntuple_fs.data_mask, NULL },
>> +	{ "action", CMDL_INT, &ntuple_fs.action, NULL },
>> +};
> [...]
>> +                       if (mode == MODE_SNTUPLE) {
>> +                               if (!strcmp(argp[i], "flow-type")) {
>> +                                       i += 1;
>> +                                       if (i >= argc) {
>> +                                               show_usage(1);
>> +                                               break;
>> +                                       }
>> +                                       ntuple_fs.flow_type =
>> +                                                   rxflow_str_to_type(argp[i]);
>> +                                       i += 1;
>> +                                       parse_generic_cmdline(argc, argp, i,
>> +                                               &sntuple_changed,
>> +                                               cmdline_ntuple,
>> +                                               ARRAY_SIZE(cmdline_ntuple));
>> +                                       i = argc;
>> +                                       break;
>> +                               } else {
>> +                                       show_usage(1);
>> +                               }
>> +                               break;
>> +                       }
> [...]
>
> parse_generic_cmdline() will write an int for each argument defined with
> type CMDL_INT or CMDL_UINT.  But the fields in ntuple_fs are not all of
> type int (or even 32-bit) - some of them are 16-bit or 64-bit, and some
> of them are big-endian.  I also wonder whether anyone really wants to
> enter an IPv4 address as a single integer.

The assignment is broken since 'p' is an int.  That can be fixed.  Also, 
we can fix the 64-bit field.  I added the user-defined field to be 64-bit 
so that we weren't locking anyone down.  My hardware only uses 2 bytes, so 
I was only able to test that.

When this was proposed, we added the IPv4 address as a single int.  People 
seemed ok with it at the time, so we went with it.  If you have a 
different approach, please present it.

Cheers,
-PJ

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Mitchell Erblich]

On Jun 21, 2010, at 1:31 PM, Peter P Waskiewicz Jr wrote:

> On Mon, 21 Jun 2010, Ben Hutchings wrote:
> 
>> On Wed, 2010-02-03 at 23:51 -0800, Jeff Kirsher wrote:
>>> From: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
>>> 
>>> Program underlying ethernet devices with n-tuple flow classification
>>> filters.
>>> 
>>> This also adds a new flag to ethtool_flags, allowing n-tuple
>>> programming to be toggled using the set_flags call.
>> 
>> I just noticed a problem with the implementation which makes me wonder
>> whether this was tested at all:
> 
> Yes, it was tested.  We didn't hit every corner case, which I think your catch below is a corner case issue.  Our hardware can only do so much.
> 
>> 
>> [...]
>>> +static struct cmdline_info cmdline_ntuple[] = {
>>> +	{ "src-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4src, NULL },
>>> +	{ "src-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4src, NULL },
>>> +	{ "dst-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4dst, NULL },
>>> +	{ "dst-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4dst, NULL },
>>> +	{ "src-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.psrc, NULL },
>>> +	{ "src-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.psrc, NULL },
>>> +	{ "dst-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.pdst, NULL },
>>> +	{ "dst-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.pdst, NULL },
>>> +	{ "vlan", CMDL_INT, &ntuple_fs.vlan_tag, NULL },
>>> +	{ "vlan-mask", CMDL_UINT, &ntuple_fs.vlan_tag_mask, NULL },
>>> +	{ "user-def", CMDL_INT, &ntuple_fs.data, NULL },
>>> +	{ "user-def-mask", CMDL_UINT, &ntuple_fs.data_mask, NULL },
>>> +	{ "action", CMDL_INT, &ntuple_fs.action, NULL },
>>> +};
>> [...]
>>> +                       if (mode == MODE_SNTUPLE) {
>>> +                               if (!strcmp(argp[i], "flow-type")) {
>>> +                                       i += 1;

			Why not " i++;  " ?

>>> +                                       if (i >= argc) {
>>> +                                               show_usage(1);
>>> +                                               break;
>>> +                                       }
>>> +                                       ntuple_fs.flow_type =
>>> +                                                   rxflow_str_to_type(argp[i]);
>>> +                                       i += 1;

			Why not "  i++;  " ?

>>> +                                       parse_generic_cmdline(argc, argp, i,
>>> +                                               &sntuple_changed,
>>> +                                               cmdline_ntuple,
>>> +                                               ARRAY_SIZE(cmdline_ntuple));
>>> +                                       i = argc;
>>> +                                       break;
>>> +                               } else {
>>> +                                       show_usage(1);
>>> +                               }
>>> +                               break;
>>> +                       }
>> [...]
>> 
>> parse_generic_cmdline() will write an int for each argument defined with
>> type CMDL_INT or CMDL_UINT.  But the fields in ntuple_fs are not all of
>> type int (or even 32-bit) - some of them are 16-bit or 64-bit, and some
>> of them are big-endian.  I also wonder whether anyone really wants to
>> enter an IPv4 address as a single integer.
> 
> The assignment is broken since 'p' is an int.  That can be fixed.  Also, we can fix the 64-bit field.  I added the user-defined field to be 64-bit so that we weren't locking anyone down.  My hardware only uses 2 bytes, so I was only able to test that.
> 
> When this was proposed, we added the IPv4 address as a single int.  People seemed ok with it at the time, so we went with it.  If you have a different approach, please present it.
> 
> Cheers,
> -PJ

Without changing the flow:

		NIT cleanup.
		See inline.

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

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Peter P Waskiewicz Jr]

On Mon, 21 Jun 2010, Mitchell Erblich wrote:

>
> On Jun 21, 2010, at 1:31 PM, Peter P Waskiewicz Jr wrote:
>
>> On Mon, 21 Jun 2010, Ben Hutchings wrote:
>>
>>> On Wed, 2010-02-03 at 23:51 -0800, Jeff Kirsher wrote:
>>>> From: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
>>>>
>>>> Program underlying ethernet devices with n-tuple flow classification
>>>> filters.
>>>>
>>>> This also adds a new flag to ethtool_flags, allowing n-tuple
>>>> programming to be toggled using the set_flags call.
>>>
>>> I just noticed a problem with the implementation which makes me wonder
>>> whether this was tested at all:
>>
>> Yes, it was tested.  We didn't hit every corner case, which I think your catch below is a corner case issue.  Our hardware can only do so much.
>>
>>>
>>> [...]
>>>> +static struct cmdline_info cmdline_ntuple[] = {
>>>> +	{ "src-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4src, NULL },
>>>> +	{ "src-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4src, NULL },
>>>> +	{ "dst-ip", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.ip4dst, NULL },
>>>> +	{ "dst-ip-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.ip4dst, NULL },
>>>> +	{ "src-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.psrc, NULL },
>>>> +	{ "src-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.psrc, NULL },
>>>> +	{ "dst-port", CMDL_INT, &ntuple_fs.h_u.tcp_ip4_spec.pdst, NULL },
>>>> +	{ "dst-port-mask", CMDL_UINT, &ntuple_fs.m_u.tcp_ip4_spec.pdst, NULL },
>>>> +	{ "vlan", CMDL_INT, &ntuple_fs.vlan_tag, NULL },
>>>> +	{ "vlan-mask", CMDL_UINT, &ntuple_fs.vlan_tag_mask, NULL },
>>>> +	{ "user-def", CMDL_INT, &ntuple_fs.data, NULL },
>>>> +	{ "user-def-mask", CMDL_UINT, &ntuple_fs.data_mask, NULL },
>>>> +	{ "action", CMDL_INT, &ntuple_fs.action, NULL },
>>>> +};
>>> [...]
>>>> +                       if (mode == MODE_SNTUPLE) {
>>>> +                               if (!strcmp(argp[i], "flow-type")) {
>>>> +                                       i += 1;
>
> 			Why not " i++;  " ?

I used previous style in the file.  Either is fine.  However, please note 
this code has already been committed and released.  If we want to change 
this, then a new patch should be submitted.  I don't think it's worth the 
thrash though.

>
>>>> +                                       if (i >= argc) {
>>>> +                                               show_usage(1);
>>>> +                                               break;
>>>> +                                       }
>>>> +                                       ntuple_fs.flow_type =
>>>> +                                                   rxflow_str_to_type(argp[i]);
>>>> +                                       i += 1;
>
> 			Why not "  i++;  " ?

See above.

>
>>>> +                                       parse_generic_cmdline(argc, argp, i,
>>>> +                                               &sntuple_changed,
>>>> +                                               cmdline_ntuple,
>>>> +                                               ARRAY_SIZE(cmdline_ntuple));
>>>> +                                       i = argc;
>>>> +                                       break;
>>>> +                               } else {
>>>> +                                       show_usage(1);
>>>> +                               }
>>>> +                               break;
>>>> +                       }
>>> [...]
>>>
>>> parse_generic_cmdline() will write an int for each argument defined with
>>> type CMDL_INT or CMDL_UINT.  But the fields in ntuple_fs are not all of
>>> type int (or even 32-bit) - some of them are 16-bit or 64-bit, and some
>>> of them are big-endian.  I also wonder whether anyone really wants to
>>> enter an IPv4 address as a single integer.
>>
>> The assignment is broken since 'p' is an int.  That can be fixed.  Also, we can fix the 64-bit field.  I added the user-defined field to be 64-bit so that we weren't locking anyone down.  My hardware only uses 2 bytes, so I was only able to test that.
>>
>> When this was proposed, we added the IPv4 address as a single int.  People seemed ok with it at the time, so we went with it.  If you have a different approach, please present it.
>>
>> Cheers,
>> -PJ
>
> Without changing the flow:
>
> 		NIT cleanup.
> 		See inline.
>
> 		Mitchell Erblich
>> --
>> 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
>
>

Re: [ethtool PATCH] ethtool: Support n-tuple filter programming

[Ben Hutchings]

On Mon, 2010-06-21 at 13:31 -0700, Peter P Waskiewicz Jr wrote:
> On Mon, 21 Jun 2010, Ben Hutchings wrote:
> 
> > On Wed, 2010-02-03 at 23:51 -0800, Jeff Kirsher wrote:
> >> From: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> >>
> >> Program underlying ethernet devices with n-tuple flow classification
> >> filters.
> >>
> >> This also adds a new flag to ethtool_flags, allowing n-tuple
> >> programming to be toggled using the set_flags call.
> >
> > I just noticed a problem with the implementation which makes me wonder
> > whether this was tested at all:
> 
> Yes, it was tested.  We didn't hit every corner case, which I think your 
> catch below is a corner case issue.  Our hardware can only do so much.

It's not a corner case.  You have added options that cannot work
correctly.

[...]
> When this was proposed, we added the IPv4 address as a single int.  People 
> seemed ok with it at the time, so we went with it.  If you have a 
> different approach, please present it.

And you don't think it matters that the address has to be written
differently on big- and little-endian architectures?

Ben.

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