[ethtool PATCH 0/6] Network flow classifier

[ethtool PATCH 0/6] Network flow classifier

[Alexander Duyck]

The following patches are meant to add support for the network flow classifier
interface built into the ethtool_rxnfc.

Once this has been accepted I plan to submit the kernel portion to Jeff
Kirsher as it is almost all ixgbe code changes with the exception of the
removal of support for ethtool_get_rx_ntuple from the kernel.

Thanks,

Alex

---

Alexander Duyck (5):
      Update documentation for -u/-U operations
      Add support for __be64 and bitops to ethtool
      ethtool: remove strings based approach for displaying n-tuple
      Add support for NFC flow classifier extensions
      Add support for ESP as a separate protocol from AH

Santwona Behera (1):
      v4 Add RX packet classification interface


 Makefile.am      |    3 
 ethtool-bitops.h |   25 +
 ethtool-copy.h   |   39 +-
 ethtool-util.h   |   44 ++
 ethtool.8.in     |  193 +++++----
 ethtool.c        |  449 +++++++++++-----------
 rxclass.c        | 1106 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 1519 insertions(+), 340 deletions(-)
 create mode 100644 ethtool-bitops.h
 create mode 100644 rxclass.c

-- 

[ethtool PATCH 1/6] Add support for ESP as a separate protocol from AH

[Alexander Duyck]

This change is mostly cosmetic.  NIU had supported AH and ESP seperately.
As such it is possible that a return value of ESP or AH may be returned
for a has request instead of the AH_ESP combined value.  To resolve that
the inputs are combined for AH and ESP into the AH_ESP value and return
values for AH and ESP will display the combined string info.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

 ethtool.8.in |    8 +++++---
 ethtool.c    |   21 ++++++++++++---------
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index 714486e..12a1d1d 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -52,7 +52,7 @@
 .\"
 .\"	\(*FL - flow type values
 .\"
-.ds FL \fBtcp4\fP|\fBudp4\fP|\fBah4\fP|\fBsctp4\fP|\fBtcp6\fP|\fBudp6\fP|\fBah6\fP|\fBsctp6\fP
+.ds FL \fBtcp4\fP|\fBudp4\fP|\fBah4\fP|\fBesp4\fP|\fBsctp4\fP|\fBtcp6\fP|\fBudp6\fP|\fBah6\fP|\fBesp6\fP|\fBsctp6\fP
 .\"
 .\"	\(*HO - hash options
 .\"
@@ -572,11 +572,13 @@ nokeep;
 lB	l.
 tcp4	TCP over IPv4
 udp4	UDP over IPv4
-ah4	IPSEC AH/ESP over IPv4
+ah4	IPSEC AH over IPv4
+esp4	IPSEC ESP over IPv4
 sctp4	SCTP over IPv4
 tcp6	TCP over IPv6
 udp6	UDP over IPv6
-ah6	IPSEC AH/ESP over IPv6
+ah6	IPSEC AH over IPv6
+esp6	IPSEC ESP over IPv6
 sctp6	SCTP over IPv6
 .TE
 .TP
diff --git a/ethtool.c b/ethtool.c
index 32df0ee..cfdac65 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -32,7 +32,6 @@
 #include <sys/ioctl.h>
 #include <sys/stat.h>
 #include <stdio.h>
-#include <string.h>
 #include <errno.h>
 #include <net/if.h>
 #include <sys/utsname.h>
@@ -233,15 +232,15 @@ static struct option {
     { "-S", "--statistics", MODE_GSTATS, "Show adapter statistics" },
     { "-n", "--show-nfc", MODE_GNFC, "Show Rx network flow classification "
 		"options",
-		"		[ rx-flow-hash tcp4|udp4|ah4|sctp4|"
-		"tcp6|udp6|ah6|sctp6 ]\n" },
+		"		[ rx-flow-hash tcp4|udp4|ah4|esp4|sctp4|"
+		"tcp6|udp6|ah6|esp6|sctp6 ]\n" },
     { "-f", "--flash", MODE_FLASHDEV, "FILENAME " "Flash firmware image "
     		"from the specified file to a region on the device",
 		"               [ REGION-NUMBER-TO-FLASH ]\n" },
     { "-N", "--config-nfc", MODE_SNFC, "Configure Rx network flow "
 		"classification options",
-		"		[ rx-flow-hash tcp4|udp4|ah4|sctp4|"
-		"tcp6|udp6|ah6|sctp6 m|v|t|s|d|f|n|r... ]\n" },
+		"		[ rx-flow-hash tcp4|udp4|ah4|esp4|sctp4|"
+		"tcp6|udp6|ah6|esp6|sctp6 m|v|t|s|d|f|n|r... ]\n" },
     { "-x", "--show-rxfh-indir", MODE_GRXFHINDIR, "Show Rx flow hash "
 		"indirection" },
     { "-X", "--set-rxfh-indir", MODE_SRXFHINDIR, "Set Rx flow hash indirection",
@@ -783,7 +782,7 @@ static int rxflow_str_to_type(const char *str)
 		flow_type = TCP_V4_FLOW;
 	else if (!strcmp(str, "udp4"))
 		flow_type = UDP_V4_FLOW;
-	else if (!strcmp(str, "ah4"))
+	else if (!strcmp(str, "ah4") || !strcmp(str, "esp4"))
 		flow_type = AH_ESP_V4_FLOW;
 	else if (!strcmp(str, "sctp4"))
 		flow_type = SCTP_V4_FLOW;
@@ -791,7 +790,7 @@ static int rxflow_str_to_type(const char *str)
 		flow_type = TCP_V6_FLOW;
 	else if (!strcmp(str, "udp6"))
 		flow_type = UDP_V6_FLOW;
-	else if (!strcmp(str, "ah6"))
+	else if (!strcmp(str, "ah6") || !strcmp(str, "esp6"))
 		flow_type = AH_ESP_V6_FLOW;
 	else if (!strcmp(str, "sctp6"))
 		flow_type = SCTP_V6_FLOW;
@@ -1941,7 +1940,9 @@ static int dump_rxfhash(int fhash, u64 val)
 		fprintf(stdout, "SCTP over IPV4 flows");
 		break;
 	case AH_ESP_V4_FLOW:
-		fprintf(stdout, "IPSEC AH over IPV4 flows");
+	case AH_V4_FLOW:
+	case ESP_V4_FLOW:
+		fprintf(stdout, "IPSEC AH/ESP over IPV4 flows");
 		break;
 	case TCP_V6_FLOW:
 		fprintf(stdout, "TCP over IPV6 flows");
@@ -1953,7 +1954,9 @@ static int dump_rxfhash(int fhash, u64 val)
 		fprintf(stdout, "SCTP over IPV6 flows");
 		break;
 	case AH_ESP_V6_FLOW:
-		fprintf(stdout, "IPSEC AH over IPV6 flows");
+	case AH_V6_FLOW:
+	case ESP_V6_FLOW:
+		fprintf(stdout, "IPSEC AH/ESP over IPV6 flows");
 		break;
 	default:
 		break;

Re: [ethtool PATCH 1/6] Add support for ESP as a separate protocol from AH

[Ben Hutchings]

On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
> This change is mostly cosmetic.  NIU had supported AH and ESP seperately.
> As such it is possible that a return value of ESP or AH may be returned
> for a has request instead of the AH_ESP combined value.  To resolve that
> the inputs are combined for AH and ESP into the AH_ESP value and return
> values for AH and ESP will display the combined string info.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>

Applied, but I moved this into a separate commit:

[...]
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -32,7 +32,6 @@
>  #include <sys/ioctl.h>
>  #include <sys/stat.h>
>  #include <stdio.h>
> -#include <string.h>
>  #include <errno.h>
>  #include <net/if.h>
>  #include <sys/utsname.h>
[...]

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.

[ethtool PATCH 2/6] Add support for NFC flow classifier extensions

[Alexander Duyck]

This change makes it so that we can add VLAN Ethertype, TCI, and 64bits of
driver defined data to a network flow classifier allowing us to handle a
n-tuple flow contained within a network flow classifier.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

 ethtool-copy.h |   39 ++++++++++++++++++++++++++++-----------
 1 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 75c3ae7..5b45442 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -376,27 +376,42 @@ struct ethtool_usrip4_spec {
 	__u8    proto;
 };
 
+union ethtool_flow_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_usrip4_spec		usr_ip4_spec;
+	struct ethhdr				ether_spec;
+	__u8					hdata[60];
+};
+
+struct ethtool_flow_ext {
+	__be16	vlan_etype;
+	__be16	vlan_tci;
+	__be32	data[2];
+};
+
 /**
  * struct ethtool_rx_flow_spec - specification for RX flow filter
  * @flow_type: Type of match to perform, e.g. %TCP_V4_FLOW
  * @h_u: Flow fields to match (dependent on @flow_type)
- * @m_u: Masks for flow field bits to be ignored
+ * @h_ext: Additional fields to match
+ * @m_u: Masks for flow field bits to be matched
+ * @m_ext: Masks for additional field bits to be matched
+ *	Note, all additional fields must be ignored unless @flow_type
+ *	includes the %FLOW_EXT flag.
  * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC
  *	if packets should be discarded
  * @location: Index of filter in hardware table
  */
 struct ethtool_rx_flow_spec {
 	__u32		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_usrip4_spec		usr_ip4_spec;
-		struct ethhdr				ether_spec;
-		__u8					hdata[72];
-	} h_u, m_u;
+	union ethtool_flow_union h_u;
+	struct ethtool_flow_ext h_ext;
+	union ethtool_flow_union m_u;
+	struct ethtool_flow_ext m_ext;
 	__u64		ring_cookie;
 	__u32		location;
 };
@@ -706,6 +721,8 @@ struct ethtool_flash {
 #define	IPV4_FLOW	0x10	/* hash only */
 #define	IPV6_FLOW	0x11	/* hash only */
 #define	ETHER_FLOW	0x12	/* spec only (ether_spec) */
+/* Flag to enable additional fields in struct ethtool_rx_flow_spec */
+#define	FLOW_EXT	0x80000000
 
 /* L3-L4 network traffic flow hash options */
 #define	RXH_L2DA	(1 << 1)

Re: [ethtool PATCH 2/6] Add support for NFC flow classifier extensions

[Ben Hutchings]

On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
> This change makes it so that we can add VLAN Ethertype, TCI, and 64bits of
> driver defined data to a network flow classifier allowing us to handle a
> n-tuple flow contained within a network flow classifier.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
[...]

This does not correspond to any version of ethtool.h in kernel history.
I sync'd from current net-next-2.6 instead.

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.

[ethtool PATCH 3/6] ethtool: remove strings based approach for displaying n-tuple

[Alexander Duyck]

This change is meant to remove the strings based approach for displaying
n-tuple filters.  A follow-on patch will replace that functionality with a
network flow classification based approach that will get the number of
filters, get their locations, and then request and display them
individually.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

 ethtool.c |   44 --------------------------------------------
 1 files changed, 0 insertions(+), 44 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index cfdac65..9ad7000 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -3194,50 +3194,6 @@ static int do_srxntuple(int fd, struct ifreq *ifr)
 
 static int do_grxntuple(int fd, struct ifreq *ifr)
 {
-	struct ethtool_sset_info *sset_info;
-	struct ethtool_gstrings *strings;
-	int sz_str, n_strings, err, i;
-
-	sset_info = malloc(sizeof(struct ethtool_sset_info) + sizeof(u32));
-	sset_info->cmd = ETHTOOL_GSSET_INFO;
-	sset_info->sset_mask = (1ULL << ETH_SS_NTUPLE_FILTERS);
-	ifr->ifr_data = (caddr_t)sset_info;
-	err = send_ioctl(fd, ifr);
-
-	if ((err < 0) ||
-	    (!(sset_info->sset_mask & (1ULL << ETH_SS_NTUPLE_FILTERS)))) {
-		perror("Cannot get driver strings info");
-		return 100;
-	}
-
-	n_strings = sset_info->data[0];
-	free(sset_info);
-	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 101;
-	}
-
-	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;
 }
 

Re: [ethtool PATCH 3/6] ethtool: remove strings based approach for displaying n-tuple

[Ben Hutchings]

On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
> This change is meant to remove the strings based approach for displaying
> n-tuple filters.  A follow-on patch will replace that functionality with a
> network flow classification based approach that will get the number of
> filters, get their locations, and then request and display them
> individually.
[...]

I'm deferring this until the other patches are updated.

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.

[ethtool PATCH 4/6] Add support for __be64 and bitops to ethtool

[Alexander Duyck]

This change is meant to add support for __be64 values and bitops to
ethtool.  These changes will be needed in order to support network flow
classifier rule configuration.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

 ethtool-bitops.h |   25 +++++++++++++++++++++++++
 ethtool-util.h   |   30 ++++++++++++++++++++++++++----
 ethtool.c        |    7 -------
 3 files changed, 51 insertions(+), 11 deletions(-)
 create mode 100644 ethtool-bitops.h

diff --git a/ethtool-bitops.h b/ethtool-bitops.h
new file mode 100644
index 0000000..0ff14f1
--- /dev/null
+++ b/ethtool-bitops.h
@@ -0,0 +1,25 @@
+#ifndef ETHTOOL_BITOPS_H__
+#define ETHTOOL_BITOPS_H__
+
+#define BITS_PER_BYTE		8
+#define BITS_PER_LONG		(BITS_PER_BYTE * sizeof(long))
+#define DIV_ROUND_UP(n, d)	(((n) + (d) - 1) / (d))
+#define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_LONG)
+
+static inline void set_bit(int nr, unsigned long *addr)
+{
+	addr[nr / BITS_PER_LONG] |= 1UL << (nr % BITS_PER_LONG);
+}
+
+static inline void clear_bit(int nr, unsigned long *addr)
+{
+	addr[nr / BITS_PER_LONG] &= ~(1UL << (nr % BITS_PER_LONG));
+}
+
+static __always_inline int test_bit(unsigned int nr, const unsigned long *addr)
+{
+	return !!((1UL << (nr % BITS_PER_LONG)) &
+		  (((unsigned long *)addr)[nr / BITS_PER_LONG]));
+}
+
+#endif
diff --git a/ethtool-util.h b/ethtool-util.h
index f053028..3d46faf 100644
--- a/ethtool-util.h
+++ b/ethtool-util.h
@@ -5,15 +5,18 @@
 
 #include <sys/types.h>
 #include <endian.h>
+#include <sys/ioctl.h>
+#include <net/if.h>
+#include "ethtool-config.h"
+#include "ethtool-copy.h"
 
 /* ethtool.h expects these to be defined by <linux/types.h> */
 #ifndef HAVE_BE_TYPES
 typedef __uint16_t __be16;
 typedef __uint32_t __be32;
+typedef unsigned long long __be64;
 #endif
 
-#include "ethtool-copy.h"
-
 typedef unsigned long long u64;
 typedef __uint32_t u32;
 typedef __uint16_t u16;
@@ -23,11 +26,15 @@ typedef __int32_t s32;
 #if __BYTE_ORDER == __BIG_ENDIAN
 static inline u16 cpu_to_be16(u16 value)
 {
-    return value;
+	return value;
 }
 static inline u32 cpu_to_be32(u32 value)
 {
-    return value;
+	return value;
+}
+static inline u64 cpu_to_be64(u64 value)
+{
+	return value;
 }
 #else
 static inline u16 cpu_to_be16(u16 value)
@@ -38,6 +45,21 @@ static inline u32 cpu_to_be32(u32 value)
 {
 	return cpu_to_be16(value >> 16) | (cpu_to_be16(value) << 16);
 }
+static inline u64 cpu_to_be64(u64 value)
+{
+	return cpu_to_be32(value >> 32) | ((u64)cpu_to_be32(value) << 32);
+}
+#endif
+
+#define ntohll cpu_to_be64
+#define htonll cpu_to_be64
+
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif
+
+#ifndef SIOCETHTOOL
+#define SIOCETHTOOL     0x8946
 #endif
 
 /* National Semiconductor DP83815, DP83816 */
diff --git a/ethtool.c b/ethtool.c
index 9ad7000..15af86a 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -45,16 +45,9 @@
 #include <linux/sockios.h>
 #include "ethtool-util.h"
 
-
-#ifndef SIOCETHTOOL
-#define SIOCETHTOOL     0x8946
-#endif
 #ifndef MAX_ADDR_LEN
 #define MAX_ADDR_LEN	32
 #endif
-#ifndef ARRAY_SIZE
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
-#endif
 
 #ifndef HAVE_NETIF_MSG
 enum {

Re: [ethtool PATCH 4/6] Add support for __be64 and bitops to ethtool

[Ben Hutchings]

On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
> This change is meant to add support for __be64 values and bitops to
> ethtool.  These changes will be needed in order to support network flow
> classifier rule configuration.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> 
>  ethtool-bitops.h |   25 +++++++++++++++++++++++++
>  ethtool-util.h   |   30 ++++++++++++++++++++++++++----
>  ethtool.c        |    7 -------
>  3 files changed, 51 insertions(+), 11 deletions(-)
>  create mode 100644 ethtool-bitops.h
> 
> diff --git a/ethtool-bitops.h b/ethtool-bitops.h
> new file mode 100644
> index 0000000..0ff14f1
> --- /dev/null
> +++ b/ethtool-bitops.h
> @@ -0,0 +1,25 @@
> +#ifndef ETHTOOL_BITOPS_H__
> +#define ETHTOOL_BITOPS_H__
> +
> +#define BITS_PER_BYTE		8
> +#define BITS_PER_LONG		(BITS_PER_BYTE * sizeof(long))
> +#define DIV_ROUND_UP(n, d)	(((n) + (d) - 1) / (d))
> +#define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_LONG)
> +
> +static inline void set_bit(int nr, unsigned long *addr)
> +{
> +	addr[nr / BITS_PER_LONG] |= 1UL << (nr % BITS_PER_LONG);
> +}
> +
> +static inline void clear_bit(int nr, unsigned long *addr)
> +{
> +	addr[nr / BITS_PER_LONG] &= ~(1UL << (nr % BITS_PER_LONG));
> +}
> +
> +static __always_inline int test_bit(unsigned int nr, const unsigned long *addr)
> +{
> +	return !!((1UL << (nr % BITS_PER_LONG)) &
> +		  (((unsigned long *)addr)[nr / BITS_PER_LONG]));
> +}

Where is __always_inline supposed to be defined?

> +#endif
> diff --git a/ethtool-util.h b/ethtool-util.h
> index f053028..3d46faf 100644
> --- a/ethtool-util.h
> +++ b/ethtool-util.h
> @@ -5,15 +5,18 @@
>  
>  #include <sys/types.h>
>  #include <endian.h>
> +#include <sys/ioctl.h>
> +#include <net/if.h>
> +#include "ethtool-config.h"
> +#include "ethtool-copy.h"
>  
>  /* ethtool.h expects these to be defined by <linux/types.h> */
>  #ifndef HAVE_BE_TYPES
>  typedef __uint16_t __be16;
>  typedef __uint32_t __be32;
> +typedef unsigned long long __be64;
>  #endif
>  
> -#include "ethtool-copy.h"
> -

You can't move the inclusion of ethtool-copy.h; that defeats the whole
purpose of the HAVE_BE_TYPES feature test.

[...]
> +#ifndef ARRAY_SIZE
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +#endif
> +
> +#ifndef SIOCETHTOOL
> +#define SIOCETHTOOL     0x8946
>  #endif
>  
>  /* National Semiconductor DP83815, DP83816 */
> diff --git a/ethtool.c b/ethtool.c
> index 9ad7000..15af86a 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -45,16 +45,9 @@
>  #include <linux/sockios.h>
>  #include "ethtool-util.h"
>  
> -
> -#ifndef SIOCETHTOOL
> -#define SIOCETHTOOL     0x8946
> -#endif
>  #ifndef MAX_ADDR_LEN
>  #define MAX_ADDR_LEN	32
>  #endif
> -#ifndef ARRAY_SIZE
> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> -#endif
>  
>  #ifndef HAVE_NETIF_MSG
>  enum {
> 

Presumably this is needed by the next patch, but it has nothing to do
with what the commit message says.

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: [ethtool PATCH 4/6] Add support for __be64 and bitops to ethtool

[Alexander Duyck]

On 4/27/2011 8:54 AM, Ben Hutchings wrote:
> On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
>> This change is meant to add support for __be64 values and bitops to
>> ethtool.  These changes will be needed in order to support network flow
>> classifier rule configuration.
>>
>> Signed-off-by: Alexander Duyck<alexander.h.duyck@intel.com>
>> ---
>>
>>   ethtool-bitops.h |   25 +++++++++++++++++++++++++
>>   ethtool-util.h   |   30 ++++++++++++++++++++++++++----
>>   ethtool.c        |    7 -------
>>   3 files changed, 51 insertions(+), 11 deletions(-)
>>   create mode 100644 ethtool-bitops.h
>>
>> diff --git a/ethtool-bitops.h b/ethtool-bitops.h
>> new file mode 100644
>> index 0000000..0ff14f1
>> --- /dev/null
>> +++ b/ethtool-bitops.h
>> @@ -0,0 +1,25 @@
>> +#ifndef ETHTOOL_BITOPS_H__
>> +#define ETHTOOL_BITOPS_H__
>> +
>> +#define BITS_PER_BYTE		8
>> +#define BITS_PER_LONG		(BITS_PER_BYTE * sizeof(long))
>> +#define DIV_ROUND_UP(n, d)	(((n) + (d) - 1) / (d))
>> +#define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_LONG)
>> +
>> +static inline void set_bit(int nr, unsigned long *addr)
>> +{
>> +	addr[nr / BITS_PER_LONG] |= 1UL<<  (nr % BITS_PER_LONG);
>> +}
>> +
>> +static inline void clear_bit(int nr, unsigned long *addr)
>> +{
>> +	addr[nr / BITS_PER_LONG]&= ~(1UL<<  (nr % BITS_PER_LONG));
>> +}
>> +
>> +static __always_inline int test_bit(unsigned int nr, const unsigned long *addr)
>> +{
>> +	return !!((1UL<<  (nr % BITS_PER_LONG))&
>> +		  (((unsigned long *)addr)[nr / BITS_PER_LONG]));
>> +}
>
> Where is __always_inline supposed to be defined?

Sorry that should have just been inline.  I forgot we have to take tools 
other than gcc into account.

>> +#endif
>> diff --git a/ethtool-util.h b/ethtool-util.h
>> index f053028..3d46faf 100644
>> --- a/ethtool-util.h
>> +++ b/ethtool-util.h
>> @@ -5,15 +5,18 @@
>>
>>   #include<sys/types.h>
>>   #include<endian.h>
>> +#include<sys/ioctl.h>
>> +#include<net/if.h>
>> +#include "ethtool-config.h"
>> +#include "ethtool-copy.h"
>>
>>   /* ethtool.h expects these to be defined by<linux/types.h>  */
>>   #ifndef HAVE_BE_TYPES
>>   typedef __uint16_t __be16;
>>   typedef __uint32_t __be32;
>> +typedef unsigned long long __be64;
>>   #endif
>>
>> -#include "ethtool-copy.h"
>> -
>
> You can't move the inclusion of ethtool-copy.h; that defeats the whole
> purpose of the HAVE_BE_TYPES feature test.

You're correct. I will get that corrected so that it is located after 
the definitions of the types.  The key bit that mattered was getting 
ethtool-config.h in there before the type declarations.  I need it in 
place to address the test for HAVE_BE_TYPES.

> [...]
>> +#ifndef ARRAY_SIZE
>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> +#endif
>> +
>> +#ifndef SIOCETHTOOL
>> +#define SIOCETHTOOL     0x8946
>>   #endif
>>
>>   /* National Semiconductor DP83815, DP83816 */
>> diff --git a/ethtool.c b/ethtool.c
>> index 9ad7000..15af86a 100644
>> --- a/ethtool.c
>> +++ b/ethtool.c
>> @@ -45,16 +45,9 @@
>>   #include<linux/sockios.h>
>>   #include "ethtool-util.h"
>>
>> -
>> -#ifndef SIOCETHTOOL
>> -#define SIOCETHTOOL     0x8946
>> -#endif
>>   #ifndef MAX_ADDR_LEN
>>   #define MAX_ADDR_LEN	32
>>   #endif
>> -#ifndef ARRAY_SIZE
>> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> -#endif
>>
>>   #ifndef HAVE_NETIF_MSG
>>   enum {
>>
>
> Presumably this is needed by the next patch, but it has nothing to do
> with what the commit message says.
>
> Ben.
>

Yes. These two moves and the addition of certain headers to the 
ethtool-util.h were to address the needs of both the rxclass.c file and 
the ethtool.c file in one central location.  I will probably break those 
off into a separate patch.

On a side note, is there a git tree somewhere I can re-base off of?  At 
this point I know you have pulled in a number of patches and I figure it 
would be helpful for me to clean up my tree so I am not guessing what is 
there and what isn't.

Thanks,

Alex

Re: [ethtool PATCH 4/6] Add support for __be64 and bitops to ethtool

[Ben Hutchings]

On Wed, 2011-04-27 at 09:46 -0700, Alexander Duyck wrote:
> On 4/27/2011 8:54 AM, Ben Hutchings wrote:
> > On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
> >> This change is meant to add support for __be64 values and bitops to
> >> ethtool.  These changes will be needed in order to support network flow
> >> classifier rule configuration.
[...]
> > Where is __always_inline supposed to be defined?
> 
> Sorry that should have just been inline.  I forgot we have to take tools 
> other than gcc into account.

Oh, it's a gcc extension?  I read the code before trying to compile it.
I've never tested with anything other than gcc but I think it's worth
making a small effort to avoid gcc extensions.

[...]
> On a side note, is there a git tree somewhere I can re-base off of?  At 
> this point I know you have pulled in a number of patches and I figure it 
> would be helpful for me to clean up my tree so I am not guessing what is 
> there and what isn't.

git://git.kernel.org/pub/scm/network/ethtool/ethtool.git

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: [ethtool PATCH 4/6] Add support for __be64 and bitops to ethtool

[Alexander Duyck]

On 4/27/2011 10:09 AM, Ben Hutchings wrote:
> On Wed, 2011-04-27 at 09:46 -0700, Alexander Duyck wrote:
>> On 4/27/2011 8:54 AM, Ben Hutchings wrote:
>>> On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
>>>> This change is meant to add support for __be64 values and bitops to
>>>> ethtool.  These changes will be needed in order to support network flow
>>>> classifier rule configuration.
> [...]
>>> Where is __always_inline supposed to be defined?
>>
>> Sorry that should have just been inline.  I forgot we have to take tools
>> other than gcc into account.
>
> Oh, it's a gcc extension?  I read the code before trying to compile it.
> I've never tested with anything other than gcc but I think it's worth
> making a small effort to avoid gcc extensions.
>
> [...]
>> On a side note, is there a git tree somewhere I can re-base off of?  At
>> this point I know you have pulled in a number of patches and I figure it
>> would be helpful for me to clean up my tree so I am not guessing what is
>> there and what isn't.
>
> git://git.kernel.org/pub/scm/network/ethtool/ethtool.git
>
> Ben.
>

I just saw the updates pushed to the tree.  Thanks.  I'll start working 
on incorporating the suggestions you made and should have an updated 
version of the remaining patches ready in the next day or so.

Thanks,

Alex

[ethtool PATCH 5/6] v4 Add RX packet classification interface

[Alexander Duyck]

From: Santwona Behera <santwona.behera@sun.com>

This patch was originally introduced as:
  [PATCH 1/3] [ethtool] Add rx pkt classification interface
  Signed-off-by: Santwona Behera <santwona.behera@sun.com>
  http://patchwork.ozlabs.org/patch/23223/

v2:
I have updated it to address a number of issues.  As a result I removed the
local caching of rules due to the fact that there were memory leaks in this
code and the rule manager would consume over 1Mb of space for an 8K table
when all that was needed was 1K in order to store which rules were active
and which were not.

In addition I dropped the use of regions as there were multiple issue found
including the fact that the regions were not properly expanding beyond 2
and the fact that the regions required reading all of the rules in order to
correctly expand beyond 2.  By dropping the regions from the rule manager
it is possible to write a much cleaner interface leaving region management
to be done by either the driver or by external management scripts.

v3:
The latest update to this patch now inverts the masks to match the mask
types used for n-tuple.  As such a network flow classifier is defined using
the exact same syntax as n-tuple, and the tool will correct for the fact
that NFC uses the 1's compliment of the n-tuple mask.

I also updated the ordering of new rules being added.  All new rules will
take the highest numbered open rule when no location is specified.

Since NFC now uses the same syntax as n-tuple I added code such that now
when location is not specified the -U option will first try to add a new
n-tuple rule, and if that fails with a ENOTSUPP it will then try to add the
rule via the NFC interface.

Finally I split out the addition of bitops and the updates to documentation
into separate patches.  This makes the total patch size a bit more
manageable since the addition of NFC and the merging of it with n-tuple were
combined into this patch.

v4:
This change merges the ntuple and network flow classifier rules so that if
we setup a rule and the device has the NTUPLE flag set we will first try to
use set_rx_ntuple.  If that fails with EOPNOTSUPP we then will attempt to
use the network flow classifier rule insertion.  This way we can support
legacy configurations such as niu on kernels prior to 2.6.40 that support
network flow classifier but not ntuple, but for drivers such as ixgbe we
can test for ntuple first, and then network flow classifier.

This patch has also updated the output to make use of the updated network
flow classifier extensions that have been accepted into the kernel.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

 Makefile.am    |    3 
 ethtool-util.h |   14 +
 ethtool.c      |  357 ++++++++++--------
 rxclass.c      | 1106 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1318 insertions(+), 162 deletions(-)
 create mode 100644 rxclass.c

diff --git a/Makefile.am b/Makefile.am
index a0d2116..0262c31 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -8,7 +8,8 @@ ethtool_SOURCES = ethtool.c ethtool-copy.h ethtool-util.h	\
 		  amd8111e.c de2104x.c e100.c e1000.c igb.c	\
 		  fec_8xx.c ibm_emac.c ixgb.c ixgbe.c natsemi.c	\
 		  pcnet32.c realtek.c tg3.c marvell.c vioc.c	\
-		  smsc911x.c at76c50x-usb.c sfc.c stmmac.c
+		  smsc911x.c at76c50x-usb.c sfc.c stmmac.c	\
+		  rxclass.c
 
 dist-hook:
 	cp $(top_srcdir)/ethtool.spec $(distdir)
diff --git a/ethtool-util.h b/ethtool-util.h
index 3d46faf..adf8fcc 100644
--- a/ethtool-util.h
+++ b/ethtool-util.h
@@ -62,6 +62,8 @@ static inline u64 cpu_to_be64(u64 value)
 #define SIOCETHTOOL     0x8946
 #endif
 
+#define	RX_CLS_LOC_UNSPEC	0xffffffffUL
+
 /* National Semiconductor DP83815, DP83816 */
 int natsemi_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
 int natsemi_dump_eeprom(struct ethtool_drvinfo *info,
@@ -125,4 +127,14 @@ int sfc_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
 int st_mac100_dump_regs(struct ethtool_drvinfo *info,
 			struct ethtool_regs *regs);
 int st_gmac_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
-#endif
+
+/* Rx flow classification */
+int rxclass_parse_ruleopts(char **optstr, int opt_cnt,
+			   struct ethtool_rx_flow_spec *fsp);
+int rxclass_rule_getall(int fd, struct ifreq *ifr);
+int rxclass_rule_get(int fd, struct ifreq *ifr, __u32 loc);
+int rxclass_rule_ins(int fd, struct ifreq *ifr,
+		     struct ethtool_rx_flow_spec *fsp);
+int rxclass_rule_del(int fd, struct ifreq *ifr, __u32 loc);
+
+#endif /* ETHTOOL_UTIL_H__ */
diff --git a/ethtool.c b/ethtool.c
index 15af86a..421fe20 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -6,6 +6,7 @@
  * Kernel 2.4 update Copyright 2001 Jeff Garzik <jgarzik@mandrakesoft.com>
  * Wake-on-LAN,natsemi,misc support by Tim Hockin <thockin@sun.com>
  * Portions Copyright 2002 Intel
+ * Portions Copyright (C) Sun Microsystems 2008
  * do_test support by Eli Kupermann <eli.kupermann@intel.com>
  * ETHTOOL_PHYS_ID support by Chris Leech <christopher.leech@intel.com>
  * e1000 support by Scott Feldman <scott.feldman@intel.com>
@@ -14,6 +15,7 @@
  * amd8111e support by Reeja John <reeja.john@amd.com>
  * long arguments by Andi Kleen.
  * SMSC LAN911x support by Steve Glendinning <steve.glendinning@smsc.com>
+ * Rx Network Flow Control configuration support <santwona.behera@sun.com>
  * Various features by Ben Hutchings <bhutchings@solarflare.com>;
  *	Copyright 2009, 2010 Solarflare Communications
  *
@@ -43,6 +45,8 @@
 #include <arpa/inet.h>
 
 #include <linux/sockios.h>
+#include <sys/socket.h>
+#include <arpa/inet.h>
 #include "ethtool-util.h"
 
 #ifndef MAX_ADDR_LEN
@@ -93,14 +97,13 @@ static int do_gstats(int fd, struct ifreq *ifr);
 static int rxflow_str_to_type(const char *str);
 static int parse_rxfhashopts(char *optstr, u32 *data);
 static char *unparse_rxfhashopts(u64 opts);
-static void parse_rxntupleopts(int argc, char **argp, int first_arg);
 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_grxfhindir(int fd, struct ifreq *ifr);
 static int do_srxfhindir(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_srxclsrule(int fd, struct ifreq *ifr);
+static int do_grxclsrule(int fd, struct ifreq *ifr);
 static int do_flash(int fd, struct ifreq *ifr);
 static int do_permaddr(int fd, struct ifreq *ifr);
 
@@ -131,8 +134,8 @@ static enum {
 	MODE_SNFC,
 	MODE_GRXFHINDIR,
 	MODE_SRXFHINDIR,
-	MODE_SNTUPLE,
-	MODE_GNTUPLE,
+	MODE_SCLSRULE,
+	MODE_GCLSRULE,
 	MODE_FLASHDEV,
 	MODE_PERMADDR,
 } mode = MODE_GSET;
@@ -238,7 +241,7 @@ static struct option {
 		"indirection" },
     { "-X", "--set-rxfh-indir", MODE_SRXFHINDIR, "Set Rx flow hash indirection",
 		"		equal N | weight W0 W1 ...\n" },
-    { "-U", "--config-ntuple", MODE_SNTUPLE, "Configure Rx ntuple filters "
+    { "-U", "--config-ntuple", MODE_SCLSRULE, "Configure Rx ntuple filters "
 		"and actions",
 		"		{ flow-type tcp4|udp4|sctp4\n"
 		"		  [ src-ip ADDR [src-ip-mask MASK] ]\n"
@@ -252,7 +255,7 @@ static struct option {
 		"		[ vlan VLAN-TAG [vlan-mask MASK] ]\n"
 		"		[ user-def DATA [user-def-mask MASK] ]\n"
 		"		action N\n" },
-    { "-u", "--show-ntuple", MODE_GNTUPLE,
+    { "-u", "--show-ntuple", MODE_GCLSRULE,
 		"Get Rx ntuple filters and actions\n" },
     { "-P", "--show-permaddr", MODE_PERMADDR,
 		"Show permanent hardware address" },
@@ -379,26 +382,6 @@ static u32 rx_fhash_val = 0;
 static int rx_fhash_changed = 0;
 static int rxfhindir_equal = 0;
 static char **rxfhindir_weight = NULL;
-static int sntuple_changed = 0;
-static struct ethtool_rx_ntuple_flow_spec ntuple_fs;
-static int ntuple_ip4src_seen = 0;
-static int ntuple_ip4src_mask_seen = 0;
-static int ntuple_ip4dst_seen = 0;
-static int ntuple_ip4dst_mask_seen = 0;
-static int ntuple_psrc_seen = 0;
-static int ntuple_psrc_mask_seen = 0;
-static int ntuple_pdst_seen = 0;
-static int ntuple_pdst_mask_seen = 0;
-static int ntuple_ether_dst_seen = 0;
-static int ntuple_ether_dst_mask_seen = 0;
-static int ntuple_ether_src_seen = 0;
-static int ntuple_ether_src_mask_seen = 0;
-static int ntuple_ether_proto_seen = 0;
-static int ntuple_ether_proto_mask_seen = 0;
-static int ntuple_vlan_tag_seen = 0;
-static int ntuple_vlan_tag_mask_seen = 0;
-static int ntuple_user_def_seen = 0;
-static int ntuple_user_def_mask_seen = 0;
 static char *flash_file = NULL;
 static int flash = -1;
 static int flash_region = -1;
@@ -407,6 +390,11 @@ static int msglvl_changed;
 static u32 msglvl_wanted = 0;
 static u32 msglvl_mask = 0;
 
+static int rx_class_rule_get = -1;
+static int rx_class_rule_del = -1;
+static int rx_class_rule_added = 0;
+static struct ethtool_rx_flow_spec rx_rule_fs;
+
 static enum {
 	ONLINE=0,
 	OFFLINE,
@@ -519,58 +507,6 @@ static struct cmdline_info cmdline_coalesce[] = {
 	{ "tx-frames-high", CMDL_S32, &coal_tx_frames_high_wanted, &ecoal.tx_max_coalesced_frames_high },
 };
 
-static struct cmdline_info cmdline_ntuple_tcp_ip4[] = {
-	{ "src-ip", CMDL_IP4, &ntuple_fs.h_u.tcp_ip4_spec.ip4src, NULL,
-	  0, &ntuple_ip4src_seen },
-	{ "src-ip-mask", CMDL_IP4, &ntuple_fs.m_u.tcp_ip4_spec.ip4src, NULL,
-	  0, &ntuple_ip4src_mask_seen },
-	{ "dst-ip", CMDL_IP4, &ntuple_fs.h_u.tcp_ip4_spec.ip4dst, NULL,
-	  0, &ntuple_ip4dst_seen },
-	{ "dst-ip-mask", CMDL_IP4, &ntuple_fs.m_u.tcp_ip4_spec.ip4dst, NULL,
-	  0, &ntuple_ip4dst_mask_seen },
-	{ "src-port", CMDL_BE16, &ntuple_fs.h_u.tcp_ip4_spec.psrc, NULL,
-	  0, &ntuple_psrc_seen },
-	{ "src-port-mask", CMDL_BE16, &ntuple_fs.m_u.tcp_ip4_spec.psrc, NULL,
-	  0, &ntuple_psrc_mask_seen },
-	{ "dst-port", CMDL_BE16, &ntuple_fs.h_u.tcp_ip4_spec.pdst, NULL,
-	  0, &ntuple_pdst_seen },
-	{ "dst-port-mask", CMDL_BE16, &ntuple_fs.m_u.tcp_ip4_spec.pdst, NULL,
-	  0, &ntuple_pdst_mask_seen },
-	{ "vlan", CMDL_U16, &ntuple_fs.vlan_tag, NULL,
-	  0, &ntuple_vlan_tag_seen },
-	{ "vlan-mask", CMDL_U16, &ntuple_fs.vlan_tag_mask, NULL,
-	  0, &ntuple_vlan_tag_mask_seen },
-	{ "user-def", CMDL_U64, &ntuple_fs.data, NULL,
-	  0, &ntuple_user_def_seen },
-	{ "user-def-mask", CMDL_U64, &ntuple_fs.data_mask, NULL,
-	  0, &ntuple_user_def_mask_seen },
-	{ "action", CMDL_S32, &ntuple_fs.action, NULL },
-};
-
-static struct cmdline_info cmdline_ntuple_ether[] = {
-	{ "dst", CMDL_MAC, ntuple_fs.h_u.ether_spec.h_dest, NULL,
-	  0, &ntuple_ether_dst_seen },
-	{ "dst-mask", CMDL_MAC, ntuple_fs.m_u.ether_spec.h_dest, NULL,
-	  0, &ntuple_ether_dst_mask_seen },
-	{ "src", CMDL_MAC, ntuple_fs.h_u.ether_spec.h_source, NULL,
-	  0, &ntuple_ether_src_seen },
-	{ "src-mask", CMDL_MAC, ntuple_fs.m_u.ether_spec.h_source, NULL,
-	  0, &ntuple_ether_src_mask_seen },
-	{ "proto", CMDL_BE16, &ntuple_fs.h_u.ether_spec.h_proto, NULL,
-	  0, &ntuple_ether_proto_seen },
-	{ "proto-mask", CMDL_BE16, &ntuple_fs.m_u.ether_spec.h_proto, NULL,
-	  0, &ntuple_ether_proto_mask_seen },
-	{ "vlan", CMDL_U16, &ntuple_fs.vlan_tag, NULL,
-	  0, &ntuple_vlan_tag_seen },
-	{ "vlan-mask", CMDL_U16, &ntuple_fs.vlan_tag_mask, NULL,
-	  0, &ntuple_vlan_tag_mask_seen },
-	{ "user-def", CMDL_U64, &ntuple_fs.data, NULL,
-	  0, &ntuple_user_def_seen },
-	{ "user-def-mask", CMDL_U64, &ntuple_fs.data_mask, NULL,
-	  0, &ntuple_user_def_mask_seen },
-	{ "action", CMDL_S32, &ntuple_fs.action, NULL },
-};
-
 static struct cmdline_info cmdline_msglvl[] = {
 	{ "drv", CMDL_FLAG, &msglvl_wanted, NULL,
 	  NETIF_MSG_DRV, &msglvl_mask },
@@ -841,8 +777,8 @@ static void parse_cmdline(int argc, char **argp)
 			    (mode == MODE_SNFC) ||
 			    (mode == MODE_GRXFHINDIR) ||
 			    (mode == MODE_SRXFHINDIR) ||
-			    (mode == MODE_SNTUPLE) ||
-			    (mode == MODE_GNTUPLE) ||
+			    (mode == MODE_SCLSRULE) ||
+			    (mode == MODE_GCLSRULE) ||
 			    (mode == MODE_PHYS_ID) ||
 			    (mode == MODE_FLASHDEV) ||
 			    (mode == MODE_PERMADDR)) {
@@ -926,16 +862,45 @@ static void parse_cmdline(int argc, char **argp)
 				i = argc;
 				break;
 			}
-			if (mode == MODE_SNTUPLE) {
+			if (mode == MODE_SCLSRULE) {
 				if (!strcmp(argp[i], "flow-type")) {
 					i += 1;
 					if (i >= argc) {
 						exit_bad_args();
 						break;
 					}
-					parse_rxntupleopts(argc, argp, i);
-					i = argc;
-					break;
+					if (rxclass_parse_ruleopts(&argp[i],
+							argc - i,
+							&rx_rule_fs) < 0) {
+						exit_bad_args();
+					} else {
+						i = argc;
+						rx_class_rule_added = 1;
+					}
+				} else if (!strcmp(argp[i], "class-rule-del")) {
+					i += 1;
+					if (i >= argc) {
+						exit_bad_args();
+						break;
+					}
+					rx_class_rule_del =
+						get_uint_range(argp[i], 0,
+							       INT_MAX);
+				} else {
+					exit_bad_args();
+				}
+				break;
+			}
+			if (mode == MODE_GCLSRULE) {
+				if (!strcmp(argp[i], "class-rule")) {
+					i += 1;
+					if (i >= argc) {
+						exit_bad_args();
+						break;
+					}
+					rx_class_rule_get =
+						get_uint_range(argp[i], 0,
+							       INT_MAX);
 				} else {
 					exit_bad_args();
 				}
@@ -1606,66 +1571,6 @@ static char *unparse_rxfhashopts(u64 opts)
 	return buf;
 }
 
-static void parse_rxntupleopts(int argc, char **argp, int i)
-{
-	ntuple_fs.flow_type = rxflow_str_to_type(argp[i]);
-
-	switch (ntuple_fs.flow_type) {
-	case TCP_V4_FLOW:
-	case UDP_V4_FLOW:
-	case SCTP_V4_FLOW:
-		parse_generic_cmdline(argc, argp, i + 1,
-				      &sntuple_changed,
-				      cmdline_ntuple_tcp_ip4,
-				      ARRAY_SIZE(cmdline_ntuple_tcp_ip4));
-		if (!ntuple_ip4src_seen)
-			ntuple_fs.m_u.tcp_ip4_spec.ip4src = 0xffffffff;
-		if (!ntuple_ip4dst_seen)
-			ntuple_fs.m_u.tcp_ip4_spec.ip4dst = 0xffffffff;
-		if (!ntuple_psrc_seen)
-			ntuple_fs.m_u.tcp_ip4_spec.psrc = 0xffff;
-		if (!ntuple_pdst_seen)
-			ntuple_fs.m_u.tcp_ip4_spec.pdst = 0xffff;
-		ntuple_fs.m_u.tcp_ip4_spec.tos = 0xff;
-		break;
-	case ETHER_FLOW:
-		parse_generic_cmdline(argc, argp, i + 1,
-				      &sntuple_changed,
-				      cmdline_ntuple_ether,
-				      ARRAY_SIZE(cmdline_ntuple_ether));
-		if (!ntuple_ether_dst_seen)
-			memset(ntuple_fs.m_u.ether_spec.h_dest, 0xff, ETH_ALEN);
-		if (!ntuple_ether_src_seen)
-			memset(ntuple_fs.m_u.ether_spec.h_source, 0xff,
-			       ETH_ALEN);
-		if (!ntuple_ether_proto_seen)
-			ntuple_fs.m_u.ether_spec.h_proto = 0xffff;
-		break;
-	default:
-		fprintf(stderr, "Unsupported flow type \"%s\"\n", argp[i]);
-		exit(106);
-		break;
-	}
-
-	if (!ntuple_vlan_tag_seen)
-		ntuple_fs.vlan_tag_mask = 0xffff;
-	if (!ntuple_user_def_seen)
-		ntuple_fs.data_mask = 0xffffffffffffffffULL;
-
-	if ((ntuple_ip4src_mask_seen && !ntuple_ip4src_seen) ||
-	    (ntuple_ip4dst_mask_seen && !ntuple_ip4dst_seen) ||
-	    (ntuple_psrc_mask_seen && !ntuple_psrc_seen) ||
-	    (ntuple_pdst_mask_seen && !ntuple_pdst_seen) ||
-	    (ntuple_ether_dst_mask_seen && !ntuple_ether_dst_seen) ||
-	    (ntuple_ether_src_mask_seen && !ntuple_ether_src_seen) ||
-	    (ntuple_ether_proto_mask_seen && !ntuple_ether_proto_seen) ||
-	    (ntuple_vlan_tag_mask_seen && !ntuple_vlan_tag_seen) ||
-	    (ntuple_user_def_mask_seen && !ntuple_user_def_seen)) {
-		fprintf(stderr, "Cannot specify mask without value\n");
-		exit(107);
-	}
-}
-
 static struct {
 	const char *name;
 	int (*func)(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
@@ -2027,10 +1932,10 @@ static int doit(void)
 		return do_grxfhindir(fd, &ifr);
 	} else if (mode == MODE_SRXFHINDIR) {
 		return do_srxfhindir(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_SCLSRULE) {
+		return do_srxclsrule(fd, &ifr);
+	} else if (mode == MODE_GCLSRULE) {
+		return do_grxclsrule(fd, &ifr);
 	} else if (mode == MODE_FLASHDEV) {
 		return do_flash(fd, &ifr);
 	} else if (mode == MODE_PERMADDR) {
@@ -3163,21 +3068,130 @@ static int do_permaddr(int fd, struct ifreq *ifr)
 	return err;
 }
 
+static int flow_spec_to_ntuple(struct ethtool_rx_flow_spec *fsp,
+			       struct ethtool_rx_ntuple_flow_spec *ntuple)
+{
+	int i;
+
+	/* verify location is not specified */
+	if (fsp->location != RX_CLS_LOC_UNSPEC)
+		return -1;
+
+	/* verify ring cookie can transfer to action */
+	if (fsp->ring_cookie > INT_MAX &&
+	    ~fsp->ring_cookie > 1)
+		return -1;
+
+	/* verify only one field is setting data field */
+	if ((fsp->flow_type & FLOW_EXT) &&
+	    (fsp->m_ext.data[0] || fsp->m_ext.data[1]) &&
+	    fsp->m_ext.vlan_etype)
+		return -1;
+
+	/* initialize entire ntuple to all 0xFF */
+	memset(ntuple, ~0, sizeof(*ntuple));
+
+	/* set non-filter values */
+	ntuple->flow_type = fsp->flow_type;
+	ntuple->action = fsp->ring_cookie;
+
+	/* copy header portion over */
+	memcpy(&ntuple->h_u, &fsp->h_u, sizeof(fsp->h_u));
+
+	/* copy mask portion over and invert */
+	memcpy(&ntuple->m_u, &fsp->m_u, sizeof(fsp->m_u));
+	for (i = 0; i < sizeof(fsp->m_u); i++)
+		ntuple->m_u.hdata[i] ^= 0xFF;
+
+	/* copy extended fields */
+	if (fsp->flow_type & FLOW_EXT) {
+		ntuple->vlan_tag =
+			ntohs(fsp->h_ext.vlan_tci);
+		ntuple->vlan_tag_mask =
+			~ntohs(fsp->m_ext.vlan_tci);
+		if (fsp->m_ext.vlan_etype) {
+			ntuple->data =
+				ntohl(fsp->h_ext.vlan_etype);
+			ntuple->data_mask =
+				~(u64)ntohl(fsp->m_ext.vlan_etype);
+		} else {
+			ntuple->data =
+				(u64)ntohl(fsp->h_ext.data[0]);
+			ntuple->data |=
+				(u64)ntohl(fsp->h_ext.data[1]) << 32;
+			ntuple->data_mask =
+				(u64)ntohl(~fsp->m_ext.data[0]);
+			ntuple->data_mask |=
+				(u64)ntohl(~fsp->m_ext.data[1]) << 32;
+		}
+	}
+
+	return 0;
+}
+
 static int do_srxntuple(int fd, struct ifreq *ifr)
 {
+	struct ethtool_rx_ntuple ntuplecmd;
+	struct ethtool_value eval;
 	int err;
 
-	if (sntuple_changed) {
-		struct ethtool_rx_ntuple ntuplecmd;
+	/* verify if Ntuple is supported on the HW */
+	err = flow_spec_to_ntuple(&rx_rule_fs, &ntuplecmd.fs);
+	if (err)
+		return -1;
+
+	/*
+	 * Check to see if the flag is set for N-tuple, this allows
+	 * us to avoid the possible EINVAL response for the N-tuple
+	 * flag not being set on the device
+	 */
+	eval.cmd = ETHTOOL_GFLAGS;
+	ifr->ifr_data = (caddr_t)&eval;
+	err = ioctl(fd, SIOCETHTOOL, ifr);
+	if (err || !(eval.data & ETH_FLAG_NTUPLE))
+		return -1;
 
-		ntuplecmd.cmd = ETHTOOL_SRXNTUPLE;
-		memcpy(&ntuplecmd.fs, &ntuple_fs,
-		       sizeof(struct ethtool_rx_ntuple_flow_spec));
+	/* send rule via N-tuple */
+	ntuplecmd.cmd = ETHTOOL_SRXNTUPLE;
+	ifr->ifr_data = (caddr_t)&ntuplecmd;
+	err = ioctl(fd, SIOCETHTOOL, ifr);
 
-		ifr->ifr_data = (caddr_t)&ntuplecmd;
-		err = ioctl(fd, SIOCETHTOOL, ifr);
-		if (err < 0)
-			perror("Cannot add new RX n-tuple filter");
+	/*
+	 * Display error only if reponse is something other than op not
+	 * supported.  It is possible that the interface uses the network
+	 * flow classifier interface instead of N-tuple. 
+	 */ 
+	if (err && errno != EOPNOTSUPP)
+		perror("Cannot add new rule via N-tuple");
+
+	return err;
+}
+
+static int do_srxclsrule(int fd, struct ifreq *ifr)
+{
+	int err;
+
+	if (rx_class_rule_added) {
+		/* attempt to add rule via N-tuple specifier */
+		err = do_srxntuple(fd, ifr);
+		if (!err)
+			return 0;
+
+		/* attempt to add rule via network flow classifier */
+		err = rxclass_rule_ins(fd, ifr, &rx_rule_fs);
+		if (err < 0) {
+			fprintf(stderr, "Cannot insert"
+				" classification rule\n");
+			return 1;
+		}
+	} else if (rx_class_rule_del >= 0) {
+		err = rxclass_rule_del(fd, ifr, rx_class_rule_del);
+
+		if (err < 0) {
+			fprintf(stderr, "Cannot delete"
+				" classification rule\n");
+			return 1;
+		}
 	} else {
 		exit_bad_args();
 	}
@@ -3185,9 +3199,32 @@ static int do_srxntuple(int fd, struct ifreq *ifr)
 	return 0;
 }
 
-static int do_grxntuple(int fd, struct ifreq *ifr)
+static int do_grxclsrule(int fd, struct ifreq *ifr)
 {
-	return 0;
+	struct ethtool_rxnfc nfccmd;
+	int err;
+
+	if (rx_class_rule_get >= 0) {
+		err = rxclass_rule_get(fd, ifr, rx_class_rule_get);
+		if (err < 0)
+			fprintf(stderr, "Cannot get RX classification rule\n");
+		return err ? 1 : 0;
+	}
+
+	nfccmd.cmd = ETHTOOL_GRXRINGS;
+	ifr->ifr_data = (caddr_t)&nfccmd;
+	err = ioctl(fd, SIOCETHTOOL, ifr);
+	if (err < 0)
+		perror("Cannot get RX rings");
+	else
+		fprintf(stdout, "%d RX rings available\n",
+			(int)nfccmd.data);
+
+	err = rxclass_rule_getall(fd, ifr);
+	if (err < 0)
+		fprintf(stderr, "RX classification rule retrieval failed\n");
+
+	return err ? 1 : 0;
 }
 
 static int send_ioctl(int fd, struct ifreq *ifr)
diff --git a/rxclass.c b/rxclass.c
new file mode 100644
index 0000000..5ad0639
--- /dev/null
+++ b/rxclass.c
@@ -0,0 +1,1106 @@
+/*
+ * Copyright (C) 2008 Sun Microsystems, Inc. All rights reserved.
+ */
+#include <stdio.h>
+#include <stdint.h>
+#include <stddef.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+
+#include <linux/sockios.h>
+#include <arpa/inet.h>
+#include "ethtool-util.h"
+#include "ethtool-bitops.h"
+
+/*
+ * This is a rule manager implementation for ordering rx flow
+ * classification rules in a longest prefix first match order.
+ * The assumption is that this rule manager is the only one adding rules to
+ * the device's hardware classifier.
+ */
+
+struct rmgr_ctrl {
+	/* slot contains a bitmap indicating which filters are valid */
+	unsigned long		*slot;
+	__u32			n_rules;
+	__u32			size;
+};
+
+static struct rmgr_ctrl rmgr;
+static int rmgr_init_done = 0;
+
+static void invert_flow_mask(struct ethtool_rx_flow_spec *fsp)
+{
+	int i;
+
+	for (i = 0; i < sizeof(fsp->m_u); i++)
+		fsp->m_u.hdata[i] ^= 0xFF;
+}
+
+static void rmgr_print_nfc_spec_ext(struct ethtool_rx_flow_spec *fsp)
+{
+	u64 data, datam;
+	__u16 etype, etypem, tci, tcim;
+
+	if (!(fsp->flow_type & FLOW_EXT))
+		return;
+
+	etype = ntohs(fsp->h_ext.vlan_etype);
+	etypem = ntohs(~fsp->m_ext.vlan_etype);
+	tci = ntohs(fsp->h_ext.vlan_tci);
+	tcim = ntohs(~fsp->m_ext.vlan_tci);
+	data = (u64)ntohl(fsp->h_ext.data[0]) << 32;
+	data = (u64)ntohl(fsp->h_ext.data[1]);
+	datam = (u64)ntohl(~fsp->m_ext.data[0]) << 32;
+	datam |= (u64)ntohl(~fsp->m_ext.data[1]);
+
+	fprintf(stdout,
+		"\tVLAN EtherType: 0x%x mask: 0x%x\n"
+		"\tVLAN: 0x%x mask: 0x%x\n"
+		"\tUser-defined: 0x%Lx mask: 0x%Lx\n",
+		etype, etypem, tci, tcim, data, datam);
+}
+
+static void rmgr_print_nfc_rule(struct ethtool_rx_flow_spec *fsp)
+{
+	unsigned char	*smac, *smacm, *dmac, *dmacm;
+	__u32		sip, dip, sipm, dipm, flow_type;
+	__u16		proto, protom;
+
+	if (fsp->location != RX_CLS_LOC_UNSPEC)
+		fprintf(stdout,	"Filter: %d\n", fsp->location);
+	else
+		fprintf(stdout,	"Filter: Unspecified\n");
+
+	flow_type = fsp->flow_type & ~FLOW_EXT;
+
+	invert_flow_mask(fsp);
+
+	switch (flow_type) {
+	case TCP_V4_FLOW:
+	case UDP_V4_FLOW:
+	case SCTP_V4_FLOW:
+	case AH_V4_FLOW:
+	case ESP_V4_FLOW:
+	case IP_USER_FLOW:
+		sip = ntohl(fsp->h_u.tcp_ip4_spec.ip4src);
+		dip = ntohl(fsp->h_u.tcp_ip4_spec.ip4dst);
+		sipm = ntohl(fsp->m_u.tcp_ip4_spec.ip4src);
+		dipm = ntohl(fsp->m_u.tcp_ip4_spec.ip4dst);
+
+		switch (flow_type) {
+		case TCP_V4_FLOW:
+			fprintf(stdout, "\tRule Type: TCP over IPv4\n");
+			break;
+		case UDP_V4_FLOW:
+			fprintf(stdout, "\tRule Type: UDP over IPv4\n");
+			break;
+		case SCTP_V4_FLOW:
+			fprintf(stdout, "\tRule Type: SCTP over IPv4\n");
+			break;
+		case AH_V4_FLOW:
+			fprintf(stdout, "\tRule Type: IPSEC AH over IPv4\n");
+			break;
+		case ESP_V4_FLOW:
+			fprintf(stdout, "\tRule Type: IPSEC ESP over IPv4\n");
+			break;
+		case IP_USER_FLOW:
+			fprintf(stdout, "\tRule Type: Raw IPv4\n");
+			break;
+		default:
+			break;
+		}
+
+		fprintf(stdout,
+			"\tSrc IP addr: %d.%d.%d.%d mask: %d.%d.%d.%d\n"
+			"\tDest IP addr: %d.%d.%d.%d mask: %d.%d.%d.%d\n"
+			"\tTOS: 0x%x mask: 0x%x\n",
+			(sip & 0xff000000) >> 24,
+			(sip & 0xff0000) >> 16,
+			(sip & 0xff00) >> 8,
+			sip & 0xff,
+			(sipm & 0xff000000) >> 24,
+			(sipm & 0xff0000) >> 16,
+			(sipm & 0xff00) >> 8,
+			sipm & 0xff,
+			(dip & 0xff000000) >> 24,
+			(dip & 0xff0000) >> 16,
+			(dip & 0xff00) >> 8,
+			dip & 0xff,
+			(dipm & 0xff000000) >> 24,
+			(dipm & 0xff0000) >> 16,
+			(dipm & 0xff00) >> 8,
+			dipm & 0xff,
+			fsp->h_u.tcp_ip4_spec.tos,
+			fsp->m_u.tcp_ip4_spec.tos);
+
+		switch (flow_type) {
+		case TCP_V4_FLOW:
+		case UDP_V4_FLOW:
+		case SCTP_V4_FLOW:
+			fprintf(stdout,
+				"\tSrc port: %d mask: 0x%x\n"
+				"\tDest port: %d mask: 0x%x\n",
+				ntohs(fsp->h_u.tcp_ip4_spec.psrc),
+				ntohs(fsp->m_u.tcp_ip4_spec.psrc),
+				ntohs(fsp->h_u.tcp_ip4_spec.pdst),
+				ntohs(fsp->m_u.tcp_ip4_spec.pdst));
+			break;
+		case AH_V4_FLOW:
+		case ESP_V4_FLOW:
+			fprintf(stdout,
+				"\tSPI: %d mask: 0x%x\n",
+				ntohl(fsp->h_u.esp_ip4_spec.spi),
+				ntohl(fsp->m_u.esp_ip4_spec.spi));
+			break;
+		case IP_USER_FLOW:
+			fprintf(stdout,
+				"\tProtocol: %d mask: 0x%x\n"
+				"\tL4 bytes: 0x%x mask: 0x%x\n",
+				fsp->h_u.usr_ip4_spec.proto,
+				fsp->m_u.usr_ip4_spec.proto,
+				ntohl(fsp->h_u.usr_ip4_spec.l4_4_bytes),
+				ntohl(fsp->m_u.usr_ip4_spec.l4_4_bytes));
+			break;
+		default:
+			break;
+		}
+		rmgr_print_nfc_spec_ext(fsp);
+		break;
+	case ETHER_FLOW:
+		dmac = fsp->h_u.ether_spec.h_dest;
+		dmacm = fsp->m_u.ether_spec.h_dest;
+		smac = fsp->h_u.ether_spec.h_source;
+		smacm = fsp->m_u.ether_spec.h_source;
+		proto = ntohs(fsp->h_u.ether_spec.h_proto);
+		protom = ntohs(fsp->m_u.ether_spec.h_proto);
+
+		fprintf(stdout,
+			"\tFlow Type: Raw Ethernet\n"
+			"\tSrc MAC addr: %02X:%02X:%02X:%02X:%02X:%02X"
+			" mask: %02X:%02X:%02X:%02X:%02X:%02X\n"
+			"\tDest MAC addr: %02X:%02X:%02X:%02X:%02X:%02X"
+			" mask: %02X:%02X:%02X:%02X:%02X:%02X\n"
+			"\tEthertype: 0x%X mask: 0x%X\n",
+			smac[0], smac[1], smac[2], smac[3], smac[4], smac[5],
+			smacm[0], smacm[1], smacm[2], smacm[3], smacm[4],
+			smacm[5], dmac[0], dmac[1], dmac[2], dmac[3], dmac[4],
+			dmac[5], dmacm[0], dmacm[1], dmacm[2], dmacm[3],
+			dmacm[4], dmacm[5], proto, protom);
+		rmgr_print_nfc_spec_ext(fsp);
+		break;
+	default:
+		fprintf(stdout,
+			"\tUnknown Flow type: %d\n", flow_type);
+		break;
+	}
+
+	if (fsp->ring_cookie != RX_CLS_FLOW_DISC)
+		fprintf(stdout, "\tAction: Direct to queue %llu\n",
+			fsp->ring_cookie);
+	else
+		fprintf(stdout, "\tAction: Drop\n");
+
+	fprintf(stdout, "\n");
+}
+
+static void rmgr_print_rule(struct ethtool_rx_flow_spec *fsp)
+{
+	/* print the rule in this location */
+	switch (fsp->flow_type & ~FLOW_EXT) {
+	case TCP_V4_FLOW:
+	case UDP_V4_FLOW:
+	case SCTP_V4_FLOW:
+	case AH_V4_FLOW:
+	case ESP_V4_FLOW:
+	case ETHER_FLOW:
+		rmgr_print_nfc_rule(fsp);
+		break;
+	case IP_USER_FLOW:
+		if (fsp->h_u.usr_ip4_spec.ip_ver == ETH_RX_NFC_IP4) {
+			rmgr_print_nfc_rule(fsp);
+			break;
+		}
+		/* IPv6 User Flow falls through to the case below */
+	case TCP_V6_FLOW:
+	case UDP_V6_FLOW:
+	case SCTP_V6_FLOW:
+	case AH_V6_FLOW:
+	case ESP_V6_FLOW:
+		fprintf(stderr, "IPv6 flows not implemented\n");
+		break;
+	default:
+		fprintf(stderr, "rmgr: Unknown flow type\n");
+		break;
+	}
+}
+
+static int rmgr_ins(__u32 loc)
+{
+	/* verify location is in rule manager range */
+	if ((loc < 0) || (loc >= rmgr.size)) {
+		fprintf(stderr, "rmgr: Location out of range\n");
+		return -1;
+	}
+
+	/* set bit for the rule */
+	set_bit(loc, rmgr.slot);
+
+	return 0;
+}
+
+static int rmgr_find(__u32 loc)
+{
+	/* verify location is in rule manager range */
+	if ((loc < 0) || (loc >= rmgr.size)) {
+		fprintf(stderr, "rmgr: Location out of range\n");
+		return -1;
+	}
+
+	/* if slot is found return 0 indicating success */
+	if (test_bit(loc, rmgr.slot))
+		return 0;
+
+	/* rule not found */
+	fprintf(stderr, "rmgr: No such rule\n");
+	return -1;
+}
+
+static int rmgr_del(__u32 loc)
+{
+	/* verify rule exists before attempting to delete */
+	int err = rmgr_find(loc);
+	if (err)
+		return err;
+
+	/* clear bit for the rule */
+	clear_bit(loc, rmgr.slot);
+
+	return 0;
+}
+
+static int rmgr_add(struct ethtool_rx_flow_spec *fsp)
+{
+	__u32 loc = fsp->location;
+
+	/* location provided, insert rule and update regions to match rule */
+	if (loc != RX_CLS_LOC_UNSPEC)
+		return rmgr_ins(loc);
+
+	/* start at the end of the list since it is lowest priority */
+	loc = rmgr.size - 1;
+
+	/* only part of last word is set so fill in remaining bits and test */
+	if (!~(rmgr.slot[loc / BITS_PER_LONG] |
+	       (~1UL << (loc % BITS_PER_LONG))))
+		loc -= 1 + (loc % BITS_PER_LONG);
+
+	/* find an open slot */
+	while (loc != RX_CLS_LOC_UNSPEC && !~rmgr.slot[loc / BITS_PER_LONG])
+		loc -= BITS_PER_LONG;
+
+	/* find and use available location in slot */
+	while (loc != RX_CLS_LOC_UNSPEC && test_bit(loc, rmgr.slot))
+		loc--;
+
+	/* location found, insert rule */
+	if (loc != RX_CLS_LOC_UNSPEC) {
+		fsp->location = loc;
+		return rmgr_ins(loc);
+	}
+
+	/* No space to add this rule */
+	fprintf(stderr, "rmgr: Cannot find appropriate slot to insert rule\n");
+
+	return -1;
+}
+
+static int rmgr_init(int fd, struct ifreq *ifr)
+{
+	struct ethtool_rxnfc *nfccmd;
+	int err, i;
+	__u32 *rule_locs;
+
+	if (rmgr_init_done)
+		return 0;
+
+	/* clear rule manager settings */
+	memset(&rmgr, 0, sizeof(struct rmgr_ctrl));
+
+	/* allocate memory for count request */
+	nfccmd = calloc(1, sizeof(*nfccmd));
+	if (!nfccmd) {
+		perror("rmgr: Cannot allocate memory for RX class rule data");
+		return -1;
+	}
+
+	/* request count and store in rmgr.n_rules */
+	nfccmd->cmd = ETHTOOL_GRXCLSRLCNT;
+	ifr->ifr_data = (caddr_t)nfccmd;
+	err = ioctl(fd, SIOCETHTOOL, ifr);
+	rmgr.n_rules = nfccmd->rule_cnt;
+	free(nfccmd);
+	if (err < 0) {
+		perror("rmgr: Cannot get RX class rule count");
+		return -1;
+	}
+
+	/* alloc memory for request of location list */
+	nfccmd = calloc(1, sizeof(*nfccmd) + (rmgr.n_rules * sizeof(__u32)));
+	if (!nfccmd) {
+		perror("rmgr: Cannot allocate memory for"
+		       " RX class rule locations");
+		return -1;
+	}
+
+	/* request location list */
+	nfccmd->cmd = ETHTOOL_GRXCLSRLALL;
+	nfccmd->rule_cnt = rmgr.n_rules;
+	ifr->ifr_data = (caddr_t)nfccmd;
+	err = ioctl(fd, SIOCETHTOOL, ifr);
+	if (err < 0) {
+		perror("rmgr: Cannot get RX class rules");
+		free(nfccmd);
+		return -1;
+	}
+
+	/* intitialize bitmap for storage of valid locations */
+	rmgr.size = nfccmd->data;
+	rmgr.slot = calloc(1, BITS_TO_LONGS(rmgr.size) * sizeof(long));
+	if (!rmgr.slot) {
+		perror("rmgr: Cannot allocate memory for RX class rules");
+		return -1;
+	}
+
+	/* write locations to bitmap */
+	rule_locs = nfccmd->rule_locs;
+	for (i = 0; i < rmgr.n_rules; i++) {
+		err = rmgr_ins(rule_locs[i]);
+		if (err < 0)
+			break;
+	}
+
+	/* free memory and set flag to avoid reinit */
+	free(nfccmd);
+	rmgr_init_done = 1;
+
+	return err;
+}
+
+static void rmgr_cleanup(void)
+{
+	if (!rmgr_init_done)
+		return;
+
+	rmgr_init_done = 0;
+
+	free(rmgr.slot);
+	rmgr.slot = NULL;
+	rmgr.size = 0;
+}
+
+int rxclass_rule_getall(int fd, struct ifreq *ifr)
+{
+	struct ethtool_rxnfc nfccmd;
+	int err, i, j;
+
+	/* init table of available rules */
+	err = rmgr_init(fd, ifr);
+	if (err < 0)
+		return err;
+
+	fprintf(stdout, "Total %d rules\n\n", rmgr.n_rules);
+
+	/* fetch and display all available rules */
+	for (i = 0; i < rmgr.size; i += BITS_PER_LONG) {
+		if (!~rmgr.slot[i / BITS_PER_LONG])
+			continue;
+		for (j = 0; j < BITS_PER_LONG; j++) {
+			if (!test_bit(i + j, rmgr.slot))
+				continue;
+			nfccmd.cmd = ETHTOOL_GRXCLSRULE;
+			memset(&nfccmd.fs, 0,
+			       sizeof(struct ethtool_rx_flow_spec));
+			nfccmd.fs.location = i + j;
+			ifr->ifr_data = (caddr_t)&nfccmd;
+			err = ioctl(fd, SIOCETHTOOL, ifr);
+			if (err < 0) {
+				perror("rmgr: Cannot get RX class rule");
+				return -1;
+			}
+			rmgr_print_rule(&nfccmd.fs);
+		}
+	}
+
+	rmgr_cleanup();
+
+	return 0;
+}
+
+int rxclass_rule_get(int fd, struct ifreq *ifr, __u32 loc)
+{
+	struct ethtool_rxnfc nfccmd;
+	int err;
+
+	/* init table of available rules */
+	err = rmgr_init(fd, ifr);
+	if (err < 0)
+		return err;
+
+	/* verify rule exists before attempting to display */
+	err = rmgr_find(loc);
+	if (err < 0)
+		return err;
+
+	/* fetch rule from netdev and display */
+	nfccmd.cmd = ETHTOOL_GRXCLSRULE;
+	memset(&nfccmd.fs, 0, sizeof(struct ethtool_rx_flow_spec));
+	nfccmd.fs.location = loc;
+	ifr->ifr_data = (caddr_t)&nfccmd;
+	err = ioctl(fd, SIOCETHTOOL, ifr);
+	if (err < 0) {
+		perror("rmgr: Cannot get RX class rule");
+		return -1;
+	}
+	rmgr_print_rule(&nfccmd.fs);
+
+	rmgr_cleanup();
+
+	return 0;
+}
+
+int rxclass_rule_ins(int fd, struct ifreq *ifr,
+		     struct ethtool_rx_flow_spec *fsp)
+{
+	struct ethtool_rxnfc nfccmd;
+	int err;
+
+	/* init table of available rules */
+	err = rmgr_init(fd, ifr);
+	if (err < 0)
+		return err;
+
+	/* verify rule location */
+	err = rmgr_add(fsp);
+	if (err < 0)
+		return err;
+
+	/* notify netdev of new rule */
+	nfccmd.cmd = ETHTOOL_SRXCLSRLINS;
+	nfccmd.fs = *fsp;
+	ifr->ifr_data = (caddr_t)&nfccmd;
+	err = ioctl(fd, SIOCETHTOOL, ifr);
+	if (err < 0) {
+		perror("rmgr: Cannot insert RX class rule");
+		return -1;
+	}
+	rmgr.n_rules++;
+
+	printf("Added rule with ID %d\n", fsp->location);
+
+	rmgr_cleanup();
+
+	return 0;
+}
+
+int rxclass_rule_del(int fd, struct ifreq *ifr, __u32 loc)
+{
+	struct ethtool_rxnfc nfccmd;
+	int err;
+
+	/* init table of available rules */
+	err = rmgr_init(fd, ifr);
+	if (err < 0)
+		return err;
+
+	/* verify rule exists */
+	err = rmgr_del(loc);
+	if (err < 0)
+		return err;
+
+	/* notify netdev of rule removal */
+	nfccmd.cmd = ETHTOOL_SRXCLSRLDEL;
+	nfccmd.fs.location = loc;
+	ifr->ifr_data = (caddr_t)&nfccmd;
+	err = ioctl(fd, SIOCETHTOOL, ifr);
+	if (err < 0) {
+		perror("rmgr: Cannot delete RX class rule");
+		return -1;
+	}
+	rmgr.n_rules--;
+
+	rmgr_cleanup();
+
+	return 0;
+}
+
+typedef enum {
+	OPT_NONE = 0,
+	OPT_S32,
+	OPT_U8,
+	OPT_U16,
+	OPT_U32,
+	OPT_U64,
+	OPT_BE16,
+	OPT_BE32,
+	OPT_BE64,
+	OPT_IP4,
+	OPT_MAC,
+} rule_opt_type_t;
+
+#define NFC_FLAG_RING		0x001
+#define NFC_FLAG_LOC		0x002
+#define NFC_FLAG_SADDR		0x004
+#define NFC_FLAG_DADDR		0x008
+#define NFC_FLAG_SPORT		0x010
+#define NFC_FLAG_DPORT		0x020
+#define NFC_FLAG_SPI		0x030
+#define NFC_FLAG_TOS		0x040
+#define NFC_FLAG_PROTO		0x080
+#define NTUPLE_FLAG_VLAN	0x100
+#define NTUPLE_FLAG_UDEF	0x200
+#define NTUPLE_FLAG_VETH	0x400
+
+struct rule_opts {
+	const char	*name;
+	rule_opt_type_t	type;
+	u32		flag;
+	int		offset;
+	int		moffset;
+};
+
+static struct rule_opts rule_nfc_tcp_ip4[] = {
+	{ "src-ip", OPT_IP4, NFC_FLAG_SADDR,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.tcp_ip4_spec.ip4src),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.tcp_ip4_spec.ip4src) },
+	{ "dst-ip", OPT_IP4, NFC_FLAG_DADDR,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.tcp_ip4_spec.ip4dst),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.tcp_ip4_spec.ip4dst) },
+	{ "tos", OPT_U8, NFC_FLAG_TOS,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.tcp_ip4_spec.tos),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.tcp_ip4_spec.tos) },
+	{ "src-port", OPT_BE16, NFC_FLAG_SPORT,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.tcp_ip4_spec.psrc),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.tcp_ip4_spec.psrc) },
+	{ "dst-port", OPT_BE16, NFC_FLAG_DPORT,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.tcp_ip4_spec.pdst),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.tcp_ip4_spec.pdst) },
+	{ "action", OPT_U64, NFC_FLAG_RING,
+	  offsetof(struct ethtool_rx_flow_spec, ring_cookie), -1 },
+	{ "loc", OPT_U32, NFC_FLAG_LOC,
+	  offsetof(struct ethtool_rx_flow_spec, location), -1 },
+	{ "vlan-etype", OPT_BE16, NTUPLE_FLAG_VETH,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.vlan_etype),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.vlan_etype) },
+	{ "vlan", OPT_BE16, NTUPLE_FLAG_VLAN,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.vlan_tci),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.vlan_tci) },
+	{ "user-def", OPT_BE64, NTUPLE_FLAG_UDEF,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.data),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.data) },
+};
+
+static struct rule_opts rule_nfc_esp_ip4[] = {
+	{ "src-ip", OPT_IP4, NFC_FLAG_SADDR,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.esp_ip4_spec.ip4src),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.esp_ip4_spec.ip4src) },
+	{ "dst-ip", OPT_IP4, NFC_FLAG_DADDR,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.esp_ip4_spec.ip4dst),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.esp_ip4_spec.ip4dst) },
+	{ "tos", OPT_U8, NFC_FLAG_TOS,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.esp_ip4_spec.tos),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.esp_ip4_spec.tos) },
+	{ "spi", OPT_BE32, NFC_FLAG_SPI,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.esp_ip4_spec.spi),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.esp_ip4_spec.spi) },
+	{ "action", OPT_U64, NFC_FLAG_RING,
+	  offsetof(struct ethtool_rx_flow_spec, ring_cookie), -1 },
+	{ "loc", OPT_U32, NFC_FLAG_LOC,
+	  offsetof(struct ethtool_rx_flow_spec, location), -1 },
+	{ "vlan-etype", OPT_BE16, NTUPLE_FLAG_VETH,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.vlan_etype),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.vlan_etype) },
+	{ "vlan", OPT_BE16, NTUPLE_FLAG_VLAN,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.vlan_tci),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.vlan_tci) },
+	{ "user-def", OPT_BE64, NTUPLE_FLAG_UDEF,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.data),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.data) },
+};
+
+static struct rule_opts rule_nfc_usr_ip4[] = {
+	{ "src-ip", OPT_IP4, NFC_FLAG_SADDR,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.usr_ip4_spec.ip4src),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.usr_ip4_spec.ip4src) },
+	{ "dst-ip", OPT_IP4, NFC_FLAG_DADDR,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.usr_ip4_spec.ip4dst),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.usr_ip4_spec.ip4dst) },
+	{ "tos", OPT_U8, NFC_FLAG_TOS,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.usr_ip4_spec.tos),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.usr_ip4_spec.tos) },
+	{ "l4proto", OPT_U8, NFC_FLAG_PROTO,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.usr_ip4_spec.proto),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.usr_ip4_spec.proto) },
+	{ "spi", OPT_BE32, NFC_FLAG_SPI,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.usr_ip4_spec.l4_4_bytes),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.usr_ip4_spec.l4_4_bytes) },
+	{ "src-port", OPT_BE16, NFC_FLAG_SPORT,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.usr_ip4_spec.l4_4_bytes),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.usr_ip4_spec.l4_4_bytes) },
+	{ "dst-port", OPT_BE16, NFC_FLAG_DPORT,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.usr_ip4_spec.l4_4_bytes) + 2,
+	  offsetof(struct ethtool_rx_flow_spec, m_u.usr_ip4_spec.l4_4_bytes) + 2 },
+	{ "action", OPT_U64, NFC_FLAG_RING,
+	  offsetof(struct ethtool_rx_flow_spec, ring_cookie), -1 },
+	{ "loc", OPT_U32, NFC_FLAG_LOC,
+	  offsetof(struct ethtool_rx_flow_spec, location), -1 },
+	{ "vlan-etype", OPT_BE16, NTUPLE_FLAG_VETH,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.vlan_etype),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.vlan_etype) },
+	{ "vlan", OPT_BE16, NTUPLE_FLAG_VLAN,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.vlan_tci),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.vlan_tci) },
+	{ "user-def", OPT_BE64, NTUPLE_FLAG_UDEF,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.data),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.data) },
+};
+
+static struct rule_opts rule_nfc_ether[] = {
+	{ "src", OPT_MAC, NFC_FLAG_SADDR,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.ether_spec.h_dest),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.ether_spec.h_dest) },
+	{ "dst", OPT_MAC, NFC_FLAG_DADDR,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.ether_spec.h_source),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.ether_spec.h_source) },
+	{ "proto", OPT_BE16, NFC_FLAG_PROTO,
+	  offsetof(struct ethtool_rx_flow_spec, h_u.ether_spec.h_proto),
+	  offsetof(struct ethtool_rx_flow_spec, m_u.ether_spec.h_proto) },
+	{ "action", OPT_U64, NFC_FLAG_RING,
+	  offsetof(struct ethtool_rx_flow_spec, ring_cookie), -1 },
+	{ "loc", OPT_U32, NFC_FLAG_LOC,
+	  offsetof(struct ethtool_rx_flow_spec, location), -1 },
+	{ "vlan-etype", OPT_BE16, NTUPLE_FLAG_VETH,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.vlan_etype),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.vlan_etype) },
+	{ "vlan", OPT_BE16, NTUPLE_FLAG_VLAN,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.vlan_tci),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.vlan_tci) },
+	{ "user-def", OPT_BE64, NTUPLE_FLAG_UDEF,
+	  offsetof(struct ethtool_rx_flow_spec, h_ext.data),
+	  offsetof(struct ethtool_rx_flow_spec, m_ext.data) },
+};
+
+static int rxclass_get_long(char *str, long long *val, int size)
+{
+	long long max = ~0ULL >> (65 - size);
+	char *endp;
+
+	errno = 0;
+
+	*val = strtoll(str, &endp, 0);
+
+	if (*endp || errno || (*val > max) || (*val < ~max))
+		return -1;
+
+	return 0;
+}
+
+static int rxclass_get_ulong(char *str, unsigned long long *val, int size)
+{
+	long long max = ~0ULL >> (64 - size);
+	char *endp;
+
+	errno = 0;
+
+	*val = strtoull(str, &endp, 0);
+
+	if (*endp || errno || (*val > max))
+		return -1;
+
+	return 0;
+}
+
+static int rxclass_get_ipv4(char *str, __be32 *val)
+{
+	if (!strchr(str, '.')) {
+		unsigned long long v;
+		int err;
+
+		err = rxclass_get_ulong(str, &v, 32);
+		if (err)
+			return -1;
+
+		*val = htonl((u32)v);
+
+		return 0;
+	}
+
+	if (!inet_pton(AF_INET, str, val))
+		return -1;
+
+	return 0;
+}
+
+static int rxclass_get_ether(char *str, unsigned char *val)
+{
+	unsigned int buf[ETH_ALEN];
+	int count;
+
+	if (!strchr(str, ':'))
+		return -1;
+
+	count = sscanf(str, "%2x:%2x:%2x:%2x:%2x:%2x",
+		       &buf[0], &buf[1], &buf[2],
+		       &buf[3], &buf[4], &buf[5]);
+
+	if (count != ETH_ALEN)
+		return -1;
+
+	do {
+		count--;
+		val[count] = buf[count];
+	} while (count);
+
+	return 0;
+}
+
+static int rxclass_get_val(char *str, unsigned char *p, u32 *flags,
+			   const struct rule_opts *opt)
+{
+	unsigned long long mask = ~0ULL;
+	int err = 0;
+
+	if (*flags & opt->flag)
+		return -1;
+
+	*flags |= opt->flag;
+
+	switch (opt->type) {
+	case OPT_S32: {
+		long long val;
+		err = rxclass_get_long(str, &val, 32);
+		if (err)
+			return -1;
+		*(int *)&p[opt->offset] = (int)val;
+		if (opt->moffset >= 0)
+			*(int *)&p[opt->moffset] = (int)mask;
+		break;
+	}
+	case OPT_U8: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 8);
+		if (err)
+			return -1;
+		*(u8 *)&p[opt->offset] = (u8)val;
+		if (opt->moffset >= 0)
+			*(u8 *)&p[opt->moffset] = (u8)mask;
+		break;
+	}
+	case OPT_U16: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 16);
+		if (err)
+			return -1;
+		*(u16 *)&p[opt->offset] = (u16)val;
+		if (opt->moffset >= 0)
+			*(u16 *)&p[opt->moffset] = (u16)mask;
+		break;
+	}
+	case OPT_U32: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 32);
+		if (err)
+			return -1;
+		*(u32 *)&p[opt->offset] = (u32)val;
+		if (opt->moffset >= 0)
+			*(u32 *)&p[opt->moffset] = (u32)mask;
+		break;
+	}
+	case OPT_U64: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 64);
+		if (err)
+			return -1;
+		*(u64 *)&p[opt->offset] = (u64)val;
+		if (opt->moffset >= 0)
+			*(u64 *)&p[opt->moffset] = (u64)mask;
+		break;
+	}
+	case OPT_BE16: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 16);
+		if (err)
+			return -1;
+		*(__be16 *)&p[opt->offset] = htons((u16)val);
+		if (opt->moffset >= 0)
+			*(__be16 *)&p[opt->moffset] = (__be16)mask;
+		break;
+	}
+	case OPT_BE32: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 32);
+		if (err)
+			return -1;
+		*(__be32 *)&p[opt->offset] = htonl((u32)val);
+		if (opt->moffset >= 0)
+			*(__be32 *)&p[opt->moffset] = (__be32)mask;
+		break;
+	}
+	case OPT_BE64: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 64);
+		if (err)
+			return -1;
+		*(__be64 *)&p[opt->offset] = htonll((u64)val);
+		if (opt->moffset >= 0)
+			*(__be64 *)&p[opt->moffset] = (__be64)mask;
+		break;
+	}
+	case OPT_IP4: {
+		__be32 val;
+		err = rxclass_get_ipv4(str, &val);
+		if (err)
+			return -1;
+		*(__be32 *)&p[opt->offset] = val;
+		if (opt->moffset >= 0)
+			*(__be32 *)&p[opt->moffset] = (__be32)mask;
+		break;
+	}
+	case OPT_MAC: {
+		unsigned char val[ETH_ALEN];
+		err = rxclass_get_ether(str, val);
+		if (err)
+			return -1;
+		memcpy(&p[opt->offset], val, ETH_ALEN);
+		if (opt->moffset >= 0)
+			memcpy(&p[opt->moffset], &mask, ETH_ALEN);
+		break;
+	}
+	case OPT_NONE:
+	default:
+		return -1;
+	}
+
+	return 0;
+}
+
+static int rxclass_get_mask(char *str, unsigned char *p,
+			    const struct rule_opts *opt)
+{
+	int err = 0;
+
+	if (opt->moffset < 0)
+		return -1;
+
+	switch (opt->type) {
+	case OPT_S32: {
+		long long val;
+		err = rxclass_get_long(str, &val, 32);
+		if (err)
+			return -1;
+		*(int *)&p[opt->moffset] = ~(int)val;
+		break;
+	}
+	case OPT_U8: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 8);
+		if (err)
+			return -1;
+		*(u8 *)&p[opt->moffset] = ~(u8)val;
+		break;
+	}
+	case OPT_U16: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 16);
+		if (err)
+			return -1;
+		*(u16 *)&p[opt->moffset] = ~(u16)val;
+		break;
+	}
+	case OPT_U32: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 32);
+		if (err)
+			return -1;
+		*(u32 *)&p[opt->moffset] = ~(u32)val;
+		break;
+	}
+	case OPT_U64: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 64);
+		if (err)
+			return -1;
+		*(u64 *)&p[opt->moffset] = ~(u64)val;
+		break;
+	}
+	case OPT_BE16: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 16);
+		if (err)
+			return -1;
+		*(__be16 *)&p[opt->moffset] = ~htons((u16)val);
+		break;
+	}
+	case OPT_BE32: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 32);
+		if (err)
+			return -1;
+		*(__be32 *)&p[opt->moffset] = ~htonl((u32)val);
+		break;
+	}
+	case OPT_BE64: {
+		unsigned long long val;
+		err = rxclass_get_ulong(str, &val, 64);
+		if (err)
+			return -1;
+		*(__be64 *)&p[opt->moffset] = ~htonll((u64)val);
+		break;
+	}
+	case OPT_IP4: {
+		__be32 val;
+		err = rxclass_get_ipv4(str, &val);
+		if (err)
+			return -1;
+		*(__be32 *)&p[opt->moffset] = ~val;
+		break;
+	}
+	case OPT_MAC: {
+		unsigned char val[ETH_ALEN];
+		int i;
+		err = rxclass_get_ether(str, val);
+		if (err)
+			return -1;
+
+		for (i = 0; i < ETH_ALEN; i++)
+			val[i] = ~val[i];
+
+		memcpy(&p[opt->moffset], val, ETH_ALEN);
+		break;
+	}
+	case OPT_NONE:
+	default:
+		return -1;
+	}
+
+	return 0;
+}
+
+int rxclass_parse_ruleopts(char **argp, int argc,
+			   struct ethtool_rx_flow_spec *fsp)
+{
+	const struct rule_opts *options;
+	unsigned char *p = (unsigned char *)fsp;
+	int i = 0, n_opts, err;
+	u32 flags = 0;
+	int flow_type;
+
+	if (*argp == NULL || **argp == '\0' || argc < 1)
+		goto syntax_err;
+
+	if (!strcmp(argp[0], "tcp4"))
+		flow_type = TCP_V4_FLOW;
+	else if (!strcmp(argp[0], "udp4"))
+		flow_type = UDP_V4_FLOW;
+	else if (!strcmp(argp[0], "sctp4"))
+		flow_type = SCTP_V4_FLOW;
+	else if (!strcmp(argp[0], "ah4"))
+		flow_type = AH_V4_FLOW;
+	else if (!strcmp(argp[0], "esp4"))
+		flow_type = ESP_V4_FLOW;
+	else if (!strcmp(argp[0], "ip4"))
+		flow_type = IP_USER_FLOW;
+	else if (!strcmp(argp[0], "ether"))
+		flow_type = ETHER_FLOW;
+	else
+		goto syntax_err;
+
+	switch (flow_type) {
+	case TCP_V4_FLOW:
+	case UDP_V4_FLOW:
+	case SCTP_V4_FLOW:
+		options = rule_nfc_tcp_ip4;
+		n_opts = ARRAY_SIZE(rule_nfc_tcp_ip4);
+		break;
+	case AH_V4_FLOW:
+	case ESP_V4_FLOW:
+		options = rule_nfc_esp_ip4;
+		n_opts = ARRAY_SIZE(rule_nfc_esp_ip4);
+		break;
+	case IP_USER_FLOW:
+		options = rule_nfc_usr_ip4;
+		n_opts = ARRAY_SIZE(rule_nfc_usr_ip4);
+		break;
+	case ETHER_FLOW:
+		options = rule_nfc_ether;
+		n_opts = ARRAY_SIZE(rule_nfc_ether);
+		break;
+	default:
+		fprintf(stdout, "Add rule, invalid rule type[%s]\n", argp[0]);
+		return -1;
+	}
+
+	memset(p, 0, sizeof(*fsp));
+	fsp->flow_type = flow_type;
+	fsp->location = RX_CLS_LOC_UNSPEC;
+
+	for (i = 1; i < argc;) {
+		const struct rule_opts *opt;
+		int idx;
+		for (opt = options, idx = 0; idx < n_opts; idx++, opt++) {
+			char mask_name[16];
+
+			if (strcmp(argp[i], opt->name))
+				continue;
+
+			i++;
+			if (i >= argc)
+				break;
+
+			err = rxclass_get_val(argp[i], p, &flags, opt);
+			if (err) {
+				fprintf(stderr, "Invalid %s value[%s]\n",
+					opt->name, argp[i]);
+				return -1;
+			}
+
+			i++;
+			if (i >= argc)
+				break;
+
+			sprintf(mask_name, "%s-mask", opt->name);
+			if (strcmp(argp[i], "m") && strcmp(argp[i], mask_name))
+				break;
+
+			i++;
+			if (i >= argc)
+				goto syntax_err;
+
+			err = rxclass_get_mask(argp[i], p, opt);
+			if (err) {
+				fprintf(stderr, "Invalid %s mask[%s]\n",
+					opt->name, argp[i]);
+				return -1;
+			}
+
+			i++;
+
+			break;
+		}
+		if (idx == n_opts) {
+			fprintf(stdout, "Add rule, unreconized option[%s]\n",
+				argp[i]);
+			return -1;
+		}
+	}
+
+	if (flags & (NTUPLE_FLAG_VLAN | NTUPLE_FLAG_UDEF | NTUPLE_FLAG_VETH))
+		fsp->flow_type |= FLOW_EXT;
+
+	return 0;
+
+syntax_err:
+	fprintf(stdout, "Add rule, invalid syntax\n");
+	return -1;
+}

Re: [ethtool PATCH 5/6] v4 Add RX packet classification interface

[Ben Hutchings]

On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
[...]
> diff --git a/ethtool.c b/ethtool.c
> index 15af86a..421fe20 100644
> --- a/ethtool.c
> +++ b/ethtool.c
[...]
> @@ -926,16 +862,45 @@ static void parse_cmdline(int argc, char **argp)
>  				i = argc;
>  				break;
>  			}
> -			if (mode == MODE_SNTUPLE) {
> +			if (mode == MODE_SCLSRULE) {
>  				if (!strcmp(argp[i], "flow-type")) {
>  					i += 1;
>  					if (i >= argc) {
>  						exit_bad_args();
>  						break;
>  					}
> -					parse_rxntupleopts(argc, argp, i);
> -					i = argc;
> -					break;
> +					if (rxclass_parse_ruleopts(&argp[i],
> +							argc - i,
> +							&rx_rule_fs) < 0) {
> +						exit_bad_args();
> +					} else {
> +						i = argc;
> +						rx_class_rule_added = 1;
> +					}
> +				} else if (!strcmp(argp[i], "class-rule-del")) {

A shorter keyword like 'delete' would do, since the -U option is only
used for flow classification.

> +					i += 1;
> +					if (i >= argc) {
> +						exit_bad_args();
> +						break;
> +					}
> +					rx_class_rule_del =
> +						get_uint_range(argp[i], 0,
> +							       INT_MAX);
> +				} else {
> +					exit_bad_args();
> +				}
> +				break;
> +			}
> +			if (mode == MODE_GCLSRULE) {
> +				if (!strcmp(argp[i], "class-rule")) {

This keyword seems redundant.

[...]
> @@ -3163,21 +3068,130 @@ static int do_permaddr(int fd, struct ifreq *ifr)
>  	return err;
>  }
>  
> +static int flow_spec_to_ntuple(struct ethtool_rx_flow_spec *fsp,
> +			       struct ethtool_rx_ntuple_flow_spec *ntuple)
> +{
> +	int i;

Should be size_t, since it's compared with a sizeof() expression.

> +	/* verify location is not specified */
> +	if (fsp->location != RX_CLS_LOC_UNSPEC)
> +		return -1;
> +
> +	/* verify ring cookie can transfer to action */
> +	if (fsp->ring_cookie > INT_MAX &&
> +	    ~fsp->ring_cookie > 1)
> +		return -1;

The second part of this condition would be more clearly expressed as:

	fsp->ring_cookie < (u64)(-2)

> +	/* verify only one field is setting data field */
> +	if ((fsp->flow_type & FLOW_EXT) &&
> +	    (fsp->m_ext.data[0] || fsp->m_ext.data[1]) &&
> +	    fsp->m_ext.vlan_etype)
> +		return -1;
> +
> +	/* initialize entire ntuple to all 0xFF */
> +	memset(ntuple, ~0, sizeof(*ntuple));

The comment needs to explain *why* the value is ~0 rather than 0.  I
assume the idea is to set the masks to ~0 if they are not initialised
below.

> +	/* set non-filter values */
> +	ntuple->flow_type = fsp->flow_type;
> +	ntuple->action = fsp->ring_cookie;
> +
> +	/* copy header portion over */
> +	memcpy(&ntuple->h_u, &fsp->h_u, sizeof(fsp->h_u));

This deserves a comment that the two h_u fields are different unions,
but are defined identically except for padding at the end.

> +	/* copy mask portion over and invert */
> +	memcpy(&ntuple->m_u, &fsp->m_u, sizeof(fsp->m_u));
> +	for (i = 0; i < sizeof(fsp->m_u); i++)
> +		ntuple->m_u.hdata[i] ^= 0xFF;
> +
> +	/* copy extended fields */
> +	if (fsp->flow_type & FLOW_EXT) {
> +		ntuple->vlan_tag =
> +			ntohs(fsp->h_ext.vlan_tci);
> +		ntuple->vlan_tag_mask =
> +			~ntohs(fsp->m_ext.vlan_tci);
> +		if (fsp->m_ext.vlan_etype) {
> +			ntuple->data =
> +				ntohl(fsp->h_ext.vlan_etype);
> +			ntuple->data_mask =
> +				~(u64)ntohl(fsp->m_ext.vlan_etype);
> +		} else {
> +			ntuple->data =
> +				(u64)ntohl(fsp->h_ext.data[0]);
> +			ntuple->data |=
> +				(u64)ntohl(fsp->h_ext.data[1]) << 32;
> +			ntuple->data_mask =
> +				(u64)ntohl(~fsp->m_ext.data[0]);
> +			ntuple->data_mask |=
> +				(u64)ntohl(~fsp->m_ext.data[1]) << 32;
> +		}
> +	}

I think it would be clearer to add:

	else {
		ntuple->vlan_tag_mask = ~(u16)0;
		ntuple->data_mask = ~(u64)0;
	}

rather than use memset() above.

> +	return 0;
> +}
> +
>  static int do_srxntuple(int fd, struct ifreq *ifr)
>  {
> +	struct ethtool_rx_ntuple ntuplecmd;
> +	struct ethtool_value eval;
>  	int err;
>  
> -	if (sntuple_changed) {
> -		struct ethtool_rx_ntuple ntuplecmd;
> +	/* verify if Ntuple is supported on the HW */

This comment is inaccurate.

> +	err = flow_spec_to_ntuple(&rx_rule_fs, &ntuplecmd.fs);
> +	if (err)
> +		return -1;
> +
> +	/*
> +	 * Check to see if the flag is set for N-tuple, this allows
> +	 * us to avoid the possible EINVAL response for the N-tuple
> +	 * flag not being set on the device
> +	 */
> +	eval.cmd = ETHTOOL_GFLAGS;
> +	ifr->ifr_data = (caddr_t)&eval;
> +	err = ioctl(fd, SIOCETHTOOL, ifr);
> +	if (err || !(eval.data & ETH_FLAG_NTUPLE))
> +		return -1;
>  
> -		ntuplecmd.cmd = ETHTOOL_SRXNTUPLE;
> -		memcpy(&ntuplecmd.fs, &ntuple_fs,
> -		       sizeof(struct ethtool_rx_ntuple_flow_spec));
> +	/* send rule via N-tuple */
> +	ntuplecmd.cmd = ETHTOOL_SRXNTUPLE;
> +	ifr->ifr_data = (caddr_t)&ntuplecmd;
> +	err = ioctl(fd, SIOCETHTOOL, ifr);
>  
> -		ifr->ifr_data = (caddr_t)&ntuplecmd;
> -		err = ioctl(fd, SIOCETHTOOL, ifr);
> -		if (err < 0)
> -			perror("Cannot add new RX n-tuple filter");
> +	/*
> +	 * Display error only if reponse is something other than op not
> +	 * supported.  It is possible that the interface uses the network
> +	 * flow classifier interface instead of N-tuple. 
> +	 */ 
> +	if (err && errno != EOPNOTSUPP)
> +		perror("Cannot add new rule via N-tuple");
> +
> +	return err;
> +}
> +
> +static int do_srxclsrule(int fd, struct ifreq *ifr)
> +{
> +	int err;
> +
> +	if (rx_class_rule_added) {
> +		/* attempt to add rule via N-tuple specifier */
> +		err = do_srxntuple(fd, ifr);
> +		if (!err)
> +			return 0;
> +
> +		/* attempt to add rule via network flow classifier */
> +		err = rxclass_rule_ins(fd, ifr, &rx_rule_fs);
> +		if (err < 0) {
> +			fprintf(stderr, "Cannot insert"
> +				" classification rule\n");
> +			return 1;
> +		}

Is this the right order to try them?  I'm not sure.

> +	} else if (rx_class_rule_del >= 0) {
> +		err = rxclass_rule_del(fd, ifr, rx_class_rule_del);
> +
> +		if (err < 0) {
> +			fprintf(stderr, "Cannot delete"
> +				" classification rule\n");
> +			return 1;
> +		}
>  	} else {
>  		exit_bad_args();
>  	}
[...]
> diff --git a/rxclass.c b/rxclass.c
> new file mode 100644
> index 0000000..5ad0639
> --- /dev/null
> +++ b/rxclass.c
> @@ -0,0 +1,1106 @@
> +/*
> + * Copyright (C) 2008 Sun Microsystems, Inc. All rights reserved.
> + */
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stddef.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <linux/sockios.h>
> +#include <arpa/inet.h>
> +#include "ethtool-util.h"
> +#include "ethtool-bitops.h"
> +
> +/*
> + * This is a rule manager implementation for ordering rx flow
> + * classification rules in a longest prefix first match order.
> + * The assumption is that this rule manager is the only one adding rules to
> + * the device's hardware classifier.
> + */
> +
> +struct rmgr_ctrl {
> +	/* slot contains a bitmap indicating which filters are valid */
> +	unsigned long		*slot;
> +	__u32			n_rules;
> +	__u32			size;
> +};
> +
> +static struct rmgr_ctrl rmgr;
> +static int rmgr_init_done = 0;
> +
> +static void invert_flow_mask(struct ethtool_rx_flow_spec *fsp)
> +{
> +	int i;

Should be size_t, since it's compared with a sizeof() expression.

> +	for (i = 0; i < sizeof(fsp->m_u); i++)
> +		fsp->m_u.hdata[i] ^= 0xFF;
> +}
> +
> +static void rmgr_print_nfc_spec_ext(struct ethtool_rx_flow_spec *fsp)
> +{
> +	u64 data, datam;
> +	__u16 etype, etypem, tci, tcim;
> +
> +	if (!(fsp->flow_type & FLOW_EXT))
> +		return;
> +
> +	etype = ntohs(fsp->h_ext.vlan_etype);
> +	etypem = ntohs(~fsp->m_ext.vlan_etype);
> +	tci = ntohs(fsp->h_ext.vlan_tci);
> +	tcim = ntohs(~fsp->m_ext.vlan_tci);
> +	data = (u64)ntohl(fsp->h_ext.data[0]) << 32;
> +	data = (u64)ntohl(fsp->h_ext.data[1]);
> +	datam = (u64)ntohl(~fsp->m_ext.data[0]) << 32;
> +	datam |= (u64)ntohl(~fsp->m_ext.data[1]);
> +
> +	fprintf(stdout,
> +		"\tVLAN EtherType: 0x%x mask: 0x%x\n"

Lower-case 'ethertype', please.

> +		"\tVLAN: 0x%x mask: 0x%x\n"
> +		"\tUser-defined: 0x%Lx mask: 0x%Lx\n",

'L' is not documented as an integer modifier for printf().  Use 'll'
instead.

> +		etype, etypem, tci, tcim, data, datam);
> +}
> +
> +static void rmgr_print_nfc_rule(struct ethtool_rx_flow_spec *fsp)
> +{
> +	unsigned char	*smac, *smacm, *dmac, *dmacm;
> +	__u32		sip, dip, sipm, dipm, flow_type;
> +	__u16		proto, protom;
> +
> +	if (fsp->location != RX_CLS_LOC_UNSPEC)
> +		fprintf(stdout,	"Filter: %d\n", fsp->location);
> +	else
> +		fprintf(stdout,	"Filter: Unspecified\n");
> +
> +	flow_type = fsp->flow_type & ~FLOW_EXT;
> +
> +	invert_flow_mask(fsp);
> +
> +	switch (flow_type) {
> +	case TCP_V4_FLOW:
> +	case UDP_V4_FLOW:
> +	case SCTP_V4_FLOW:
> +	case AH_V4_FLOW:
> +	case ESP_V4_FLOW:
> +	case IP_USER_FLOW:
> +		sip = ntohl(fsp->h_u.tcp_ip4_spec.ip4src);
> +		dip = ntohl(fsp->h_u.tcp_ip4_spec.ip4dst);
> +		sipm = ntohl(fsp->m_u.tcp_ip4_spec.ip4src);
> +		dipm = ntohl(fsp->m_u.tcp_ip4_spec.ip4dst);

I think this will work for AH_V4, ESP_V4 and IP_USER due to similar
structure layout, but it's kind of dodgy.  Please handle each structure
type separately.

[...]
> +static int rmgr_del(__u32 loc)
> +{
> +	/* verify rule exists before attempting to delete */
> +	int err = rmgr_find(loc);
> +	if (err)
> +		return err;
> +
> +	/* clear bit for the rule */
> +	clear_bit(loc, rmgr.slot);
> +
> +	return 0;
> +}
> +
> +static int rmgr_add(struct ethtool_rx_flow_spec *fsp)
> +{
> +	__u32 loc = fsp->location;
> +
> +	/* location provided, insert rule and update regions to match rule */
> +	if (loc != RX_CLS_LOC_UNSPEC)
> +		return rmgr_ins(loc);
> +
> +	/* start at the end of the list since it is lowest priority */
> +	loc = rmgr.size - 1;
> +
> +	/* only part of last word is set so fill in remaining bits and test */
> +	if (!~(rmgr.slot[loc / BITS_PER_LONG] |
> +	       (~1UL << (loc % BITS_PER_LONG))))
> +		loc -= 1 + (loc % BITS_PER_LONG);

I think this is meant to avoid the need to search for bits in a partial
word but it's a very non-obvious way to do that.

> +	/* find an open slot */
> +	while (loc != RX_CLS_LOC_UNSPEC && !~rmgr.slot[loc / BITS_PER_LONG])
> +		loc -= BITS_PER_LONG;

Just because RX_CLS_LOC_UNSPEC happens to be equal to -1 doesn't mean
it's a good idea to use the name here!

> +	/* find and use available location in slot */
> +	while (loc != RX_CLS_LOC_UNSPEC && test_bit(loc, rmgr.slot))
> +		loc--;
> +
> +       /* location found, insert rule */
> +       if (loc != RX_CLS_LOC_UNSPEC) {
> +               fsp->location = loc;
> +               return rmgr_ins(loc);
> +       }

I think it would be clearer to use a separate word index:

	__u32 loc;
	int i;

	/* location provided, insert rule and update regions to match rule */
	if (fs->location != RX_CLS_LOC_UNSPEC)
		return rmgr_ins(fs->location);

	/* start at the end of the list since it is lowest priority */
	for (i = rmgr.size / BITS_PER_LONG - 1; i >= 0; i--) {
		if (~rmgr.slot[i]) {
			loc = i * BITS_PER_LONG - 1;
			while (test_bit(loc, rmgr.slot))
				loc--;

			/* location found, insert rule */
			fsp->location = loc;
			return rmgr_ins(loc);
		}
	}

[...]
> +int rxclass_rule_getall(int fd, struct ifreq *ifr)
> +{
> +	struct ethtool_rxnfc nfccmd;
> +	int err, i, j;
> +
> +	/* init table of available rules */
> +	err = rmgr_init(fd, ifr);
> +	if (err < 0)
> +		return err;
> +
> +	fprintf(stdout, "Total %d rules\n\n", rmgr.n_rules);
> +
> +	/* fetch and display all available rules */
> +	for (i = 0; i < rmgr.size; i += BITS_PER_LONG) {
> +		if (!~rmgr.slot[i / BITS_PER_LONG])
> +			continue;

The '~' is wrong.  We want to skip words where all bits are clear, not
where all bits are set.

[...]
> +int rxclass_rule_get(int fd, struct ifreq *ifr, __u32 loc)
> +{
> +	struct ethtool_rxnfc nfccmd;
> +	int err;
> +
> +	/* init table of available rules */
> +	err = rmgr_init(fd, ifr);
> +	if (err < 0)
> +		return err;
> +
> +	/* verify rule exists before attempting to display */
> +	err = rmgr_find(loc);
> +	if (err < 0)
> +		return err;

Is this really necessary?  Shouldn't we let the driver check that for
us, and save the cost of initialising the rule manager?

[...]
> +int rxclass_rule_ins(int fd, struct ifreq *ifr,
> +		     struct ethtool_rx_flow_spec *fsp)
> +{
> +	struct ethtool_rxnfc nfccmd;
> +	int err;
> +
> +	/* init table of available rules */
> +	err = rmgr_init(fd, ifr);
> +	if (err < 0)
> +		return err;
> +
> +	/* verify rule location */
> +	err = rmgr_add(fsp);
> +	if (err < 0)
> +		return err;
> +
> +	/* notify netdev of new rule */
> +	nfccmd.cmd = ETHTOOL_SRXCLSRLINS;
> +	nfccmd.fs = *fsp;
> +	ifr->ifr_data = (caddr_t)&nfccmd;
> +	err = ioctl(fd, SIOCETHTOOL, ifr);
> +	if (err < 0) {
> +		perror("rmgr: Cannot insert RX class rule");
> +		return -1;
> +	}
> +	rmgr.n_rules++;

If we're about to destroy the rule manager immediately afterwards, why
bother maintaining n_rules?

> +	printf("Added rule with ID %d\n", fsp->location);
> +
> +	rmgr_cleanup();
> +
> +	return 0;
> +}
> +
> +int rxclass_rule_del(int fd, struct ifreq *ifr, __u32 loc)
> +{
> +	struct ethtool_rxnfc nfccmd;
> +	int err;
> +
> +	/* init table of available rules */
> +	err = rmgr_init(fd, ifr);
> +	if (err < 0)
> +		return err;
> +
> +	/* verify rule exists */
> +	err = rmgr_del(loc);
> +	if (err < 0)
> +		return err;

Again, I think that can be left to the driver.

[...]
> +typedef enum {
> +	OPT_NONE = 0,
> +	OPT_S32,
> +	OPT_U8,
> +	OPT_U16,
> +	OPT_U32,
> +	OPT_U64,
> +	OPT_BE16,
> +	OPT_BE32,
> +	OPT_BE64,
> +	OPT_IP4,
> +	OPT_MAC,
> +} rule_opt_type_t;
> +
> +#define NFC_FLAG_RING		0x001
> +#define NFC_FLAG_LOC		0x002
> +#define NFC_FLAG_SADDR		0x004
> +#define NFC_FLAG_DADDR		0x008
> +#define NFC_FLAG_SPORT		0x010
> +#define NFC_FLAG_DPORT		0x020
> +#define NFC_FLAG_SPI		0x030
> +#define NFC_FLAG_TOS		0x040
> +#define NFC_FLAG_PROTO		0x080
> +#define NTUPLE_FLAG_VLAN	0x100
> +#define NTUPLE_FLAG_UDEF	0x200
> +#define NTUPLE_FLAG_VETH	0x400
> +
> +struct rule_opts {
> +	const char	*name;
> +	rule_opt_type_t	type;
> +	u32		flag;
> +	int		offset;
> +	int		moffset;
> +};

I think that this ought to be merged with the argument parsing in
ethtool.c, but I won't insist that you do that immedaitely.  However:

[...]
> +static int rxclass_get_ipv4(char *str, __be32 *val)
> +{
> +	if (!strchr(str, '.')) {
> +		unsigned long long v;
> +		int err;
> +
> +		err = rxclass_get_ulong(str, &v, 32);
> +		if (err)
> +			return -1;
> +
> +		*val = htonl((u32)v);
> +
> +		return 0;
> +	}
> +
> +	if (!inet_pton(AF_INET, str, val))
> +		return -1;

There's no need to make a special case for integers.  inet_aton() should
handle all the historically supported IPv4 literal formats.

[...]
> +int rxclass_parse_ruleopts(char **argp, int argc,
> +			   struct ethtool_rx_flow_spec *fsp)
> +{
> +	const struct rule_opts *options;
> +	unsigned char *p = (unsigned char *)fsp;
> +	int i = 0, n_opts, err;
> +	u32 flags = 0;
> +	int flow_type;
> +
> +	if (*argp == NULL || **argp == '\0' || argc < 1)
> +		goto syntax_err;

argc < 1 is sufficient.

> +	if (!strcmp(argp[0], "tcp4"))
> +		flow_type = TCP_V4_FLOW;
> +	else if (!strcmp(argp[0], "udp4"))
> +		flow_type = UDP_V4_FLOW;
> +	else if (!strcmp(argp[0], "sctp4"))
> +		flow_type = SCTP_V4_FLOW;
> +	else if (!strcmp(argp[0], "ah4"))
> +		flow_type = AH_V4_FLOW;
> +	else if (!strcmp(argp[0], "esp4"))
> +		flow_type = ESP_V4_FLOW;
> +	else if (!strcmp(argp[0], "ip4"))
> +		flow_type = IP_USER_FLOW;
> +	else if (!strcmp(argp[0], "ether"))
> +		flow_type = ETHER_FLOW;
> +	else
> +		goto syntax_err;
> +
> +	switch (flow_type) {
> +	case TCP_V4_FLOW:
> +	case UDP_V4_FLOW:
> +	case SCTP_V4_FLOW:
> +		options = rule_nfc_tcp_ip4;
> +		n_opts = ARRAY_SIZE(rule_nfc_tcp_ip4);
> +		break;
> +	case AH_V4_FLOW:
> +	case ESP_V4_FLOW:
> +		options = rule_nfc_esp_ip4;
> +		n_opts = ARRAY_SIZE(rule_nfc_esp_ip4);
> +		break;
> +	case IP_USER_FLOW:
> +		options = rule_nfc_usr_ip4;
> +		n_opts = ARRAY_SIZE(rule_nfc_usr_ip4);
> +		break;
> +	case ETHER_FLOW:
> +		options = rule_nfc_ether;
> +		n_opts = ARRAY_SIZE(rule_nfc_ether);
> +		break;
> +	default:
> +		fprintf(stdout, "Add rule, invalid rule type[%s]\n", argp[0]);

stderr, not stdout

[...]
> +		if (idx == n_opts) {
> +			fprintf(stdout, "Add rule, unreconized option[%s]\n",

Typo: 'unreconized' should be 'unrecognized'.

> +				argp[i]);
> +			return -1;
> +		}
> +	}
> +
> +	if (flags & (NTUPLE_FLAG_VLAN | NTUPLE_FLAG_UDEF | NTUPLE_FLAG_VETH))
> +		fsp->flow_type |= FLOW_EXT;
> +
> +	return 0;
> +
> +syntax_err:
> +	fprintf(stdout, "Add rule, invalid syntax\n");

stderr

> +	return -1;
> +}
> 

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: [ethtool PATCH 5/6] v4 Add RX packet classification interface

[Ben Hutchings]

On Wed, 2011-04-27 at 19:12 +0100, Ben Hutchings wrote:
> On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
[...]
> > +	fprintf(stdout,
> > +		"\tVLAN EtherType: 0x%x mask: 0x%x\n"
> 
> Lower-case 'ethertype', please.
[...]

Sorry, your capitalisation here is correct and the existing uses in
lower-case are wrong.

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: [ethtool PATCH 5/6] v4 Add RX packet classification interface

[Alexander Duyck]

On 4/27/2011 11:12 AM, Ben Hutchings wrote:
> On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
[...]
>> +     /* verify only one field is setting data field */
>> +     if ((fsp->flow_type&  FLOW_EXT)&&
>> +         (fsp->m_ext.data[0] || fsp->m_ext.data[1])&&
>> +         fsp->m_ext.vlan_etype)
>> +             return -1;
>> +
>> +     /* initialize entire ntuple to all 0xFF */
>> +     memset(ntuple, ~0, sizeof(*ntuple));
>
> The comment needs to explain *why* the value is ~0 rather than 0.  I
> assume the idea is to set the masks to ~0 if they are not initialised
> below.

This has to do with future compatibility and general setup.  By setting 
all of the values to ~0 the rule is essentially set to mask everything 
and has a default action of drop.

>> +     /* set non-filter values */
>> +     ntuple->flow_type = fsp->flow_type;
>> +     ntuple->action = fsp->ring_cookie;
>> +
>> +     /* copy header portion over */
>> +     memcpy(&ntuple->h_u,&fsp->h_u, sizeof(fsp->h_u));
>
> This deserves a comment that the two h_u fields are different unions,
> but are defined identically except for padding at the end.

I've added a comment similar to that to the code.

>> +     /* copy mask portion over and invert */
>> +     memcpy(&ntuple->m_u,&fsp->m_u, sizeof(fsp->m_u));
>> +     for (i = 0; i<  sizeof(fsp->m_u); i++)
>> +             ntuple->m_u.hdata[i] ^= 0xFF;
>> +
>> +     /* copy extended fields */
>> +     if (fsp->flow_type&  FLOW_EXT) {
>> +             ntuple->vlan_tag =
>> +                     ntohs(fsp->h_ext.vlan_tci);
>> +             ntuple->vlan_tag_mask =
>> +                     ~ntohs(fsp->m_ext.vlan_tci);
>> +             if (fsp->m_ext.vlan_etype) {
>> +                     ntuple->data =
>> +                             ntohl(fsp->h_ext.vlan_etype);
>> +                     ntuple->data_mask =
>> +                             ~(u64)ntohl(fsp->m_ext.vlan_etype);
>> +             } else {
>> +                     ntuple->data =
>> +                             (u64)ntohl(fsp->h_ext.data[0]);
>> +                     ntuple->data |=
>> +                             (u64)ntohl(fsp->h_ext.data[1])<<  32;
>> +                     ntuple->data_mask =
>> +                             (u64)ntohl(~fsp->m_ext.data[0]);
>> +                     ntuple->data_mask |=
>> +                             (u64)ntohl(~fsp->m_ext.data[1])<<  32;
>> +             }
>> +     }
>
> I think it would be clearer to add:
>
>          else {
>                  ntuple->vlan_tag_mask = ~(u16)0;
>                  ntuple->data_mask = ~(u64)0;
>          }
>
> rather than use memset() above.

I thought about doing that, but the advantage of the memset is that it 
covers all of the fields.  So in the unlikely event that someone were to 
add fields in the future to the h_u/m_u sections of the driver this way 
we can guarantee all of the fields that we didn't set are masked.

>> +     return 0;
>> +}
>> +
>>   static int do_srxntuple(int fd, struct ifreq *ifr)
>>   {
>> +     struct ethtool_rx_ntuple ntuplecmd;
>> +     struct ethtool_value eval;
>>        int err;
>>
>> -     if (sntuple_changed) {
>> -             struct ethtool_rx_ntuple ntuplecmd;
>> +     /* verify if Ntuple is supported on the HW */
>
> This comment is inaccurate.

Yeah, it belonged a few lines lower  I think it was left over from some 
earlier work that was there and when I reordered things to do the 
conversion to ntuple first the comment didn't get moved.

>> +     err = flow_spec_to_ntuple(&rx_rule_fs,&ntuplecmd.fs);
>> +     if (err)
>> +             return -1;
>> +
>> +     /*
>> +      * Check to see if the flag is set for N-tuple, this allows
>> +      * us to avoid the possible EINVAL response for the N-tuple
>> +      * flag not being set on the device
>> +      */
>> +     eval.cmd = ETHTOOL_GFLAGS;
>> +     ifr->ifr_data = (caddr_t)&eval;
>> +     err = ioctl(fd, SIOCETHTOOL, ifr);
>> +     if (err || !(eval.data&  ETH_FLAG_NTUPLE))
>> +             return -1;
>>
>> -             ntuplecmd.cmd = ETHTOOL_SRXNTUPLE;
>> -             memcpy(&ntuplecmd.fs,&ntuple_fs,
>> -                    sizeof(struct ethtool_rx_ntuple_flow_spec));
>> +     /* send rule via N-tuple */
>> +     ntuplecmd.cmd = ETHTOOL_SRXNTUPLE;
>> +     ifr->ifr_data = (caddr_t)&ntuplecmd;
>> +     err = ioctl(fd, SIOCETHTOOL, ifr);
>>
>> -             ifr->ifr_data = (caddr_t)&ntuplecmd;
>> -             err = ioctl(fd, SIOCETHTOOL, ifr);
>> -             if (err<  0)
>> -                     perror("Cannot add new RX n-tuple filter");
>> +     /*
>> +      * Display error only if reponse is something other than op not
>> +      * supported.  It is possible that the interface uses the network
>> +      * flow classifier interface instead of N-tuple.
>> +      */
>> +     if (err&&  errno != EOPNOTSUPP)
>> +             perror("Cannot add new rule via N-tuple");
>> +
>> +     return err;
>> +}
>> +
>> +static int do_srxclsrule(int fd, struct ifreq *ifr)
>> +{
>> +     int err;
>> +
>> +     if (rx_class_rule_added) {
>> +             /* attempt to add rule via N-tuple specifier */
>> +             err = do_srxntuple(fd, ifr);
>> +             if (!err)
>> +                     return 0;
>> +
>> +             /* attempt to add rule via network flow classifier */
>> +             err = rxclass_rule_ins(fd, ifr,&rx_rule_fs);
>> +             if (err<  0) {
>> +                     fprintf(stderr, "Cannot insert"
>> +                             " classification rule\n");
>> +                     return 1;
>> +             }
>
> Is this the right order to try them?  I'm not sure.

The reason why I chose to do it this way was because it is actually 
fewer steps to try doing an ntuple than it is to do a network flow 
classifier rule.  To fail ntuple I only really need to check the flag, 
whereas for network flow classifier I end up having to go through the 
path that does init for any rule add which is significantly more overhead.

All of the other changes you suggested for this patch I think I have 
implemented for the next version.  I'm working on finishing up the 
updates now.  But I think it will take me a day or to in order to 
sufficient testing so I can be confident all the bugs are worked out 
after the changes.  I'll probably be able to email out the update 
patches on Friday.

Thanks,

Alex

[ethtool PATCH 6/6] Update documentation for -u/-U operations

[Alexander Duyck]

This patch updates the documentation for the -u/-U operations to include
the recent changes made to support addition/deletion/display of network
flow classifier rules.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

 ethtool.8.in |  185 +++++++++++++++++++++++++++++-----------------------------
 ethtool.c    |   32 ++++++----
 2 files changed, 111 insertions(+), 106 deletions(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index 12a1d1d..8908351 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -42,10 +42,20 @@
 [\\fB\\$1\\fP\ \\fIN\\fP]
 ..
 .\"
+.\"	.BM - same as above but has a mask field for format "[value N [value-mask N]]"
+.\"
+.de BM
+[\\fB\\$1\\fP\ \\fIN\\fP\ [\\fB\\$1\-mask\\fP\ \\fIN\\fP]]
+..
+.\"
 .\"	\(*MA - mac address
 .\"
 .ds MA \fIxx\fP\fB:\fP\fIyy\fP\fB:\fP\fIzz\fP\fB:\fP\fIaa\fP\fB:\fP\fIbb\fP\fB:\fP\fIcc\fP
 .\"
+.\"	\(*PA - IP address
+.\"
+.ds PA \fIx\fP\fB.\fP\fIx\fP\fB.\fP\fIx\fP\fB.\fP\fIx\fP
+.\"
 .\"	\(*WO - wol flags
 .\"
 .ds WO \fBp\fP|\fBu\fP|\fBm\fP|\fBb\fP|\fBa\fP|\fBg\fP|\fBs\fP|\fBd\fP...
@@ -57,6 +67,12 @@
 .\"	\(*HO - hash options
 .\"
 .ds HO \fBm\fP|\fBv\fP|\fBt\fP|\fBs\fP|\fBd\fP|\fBf\fP|\fBn\fP|\fBr\fP...
+.\"
+.\"	\(*NC - Network Classifier type values
+.\"
+.ds NC \fBether\fP|\fBip4\fP|\fBtcp4\fP|\fBudp4\fP|\fBsctp4\fP|\fBah4\fP|\fBesp4\fP
+
+.\"
 .\" Start URL.
 .de UR
 .  ds m1 \\$1\"
@@ -236,9 +252,9 @@ ethtool \- query or control network driver and hardware settings
 .HP
 .B ethtool \-N
 .I ethX
-.RB [ rx\-flow\-hash \ \*(FL
-.RB \ \*(HO]
+.RB [ rx-flow-hash \ \*(FL \  \*(HO]
 .HP
+
 .B ethtool \-x|\-\-show\-rxfh\-indir
 .I ethX
 .HP
@@ -257,54 +273,28 @@ ethtool \- query or control network driver and hardware settings
 .HP
 .B ethtool \-u|\-\-show\-ntuple
 .I ethX
-.TP
+.BN class-rule
+.HP
+
 .BI ethtool\ \-U|\-\-config\-ntuple \ ethX
-.RB {
-.A3 flow\-type tcp4 udp4 sctp4
-.RB [ src\-ip
-.IR addr
-.RB [ src\-ip\-mask
-.IR mask ]]
-.RB [ dst\-ip
-.IR addr
-.RB [ dst\-ip\-mask
-.IR mask ]]
-.RB [ src\-port
-.IR port
-.RB [ src\-port\-mask
-.IR mask ]]
-.RB [ dst\-port
-.IR port
-.RB [ dst\-port\-mask
-.IR mask ]]
-.br
-.RB | \ flow\-type\ ether
-.RB [ src
-.IR mac\-addr
-.RB [ src\-mask
-.IR mask ]]
-.RB [ dst
-.IR mac\-addr
-.RB [ dst\-mask
-.IR mask ]]
-.RB [ proto
-.IR N
-.RB [ proto\-mask
-.IR mask ]]\ }
-.br
-.RB [ vlan
-.IR VLAN\-tag
-.RB [ vlan\-mask
-.IR mask ]]
-.RB [ user\-def
-.IR data
-.RB [ user\-def\-mask
-.IR mask ]]
-.RI action \ N
-.
-.\" Adjust lines (i.e. full justification) and hyphenate.
-.ad
-.hy
+.BN class-rule-del
+.RB [\  flow-type \ \*(NC
+.RB [ src \ \*(MA\ [ src-mask \ \*(MA]]
+.RB [ dst \ \*(MA\ [ dst-mask \ \*(MA]]
+.BM proto
+.RB [ src-ip \ \*(PA\ [ src-ip-mask \ \*(PA]]
+.RB [ dst-ip \ \*(PA\ [ dst-ip-mask \ \*(PA]]
+.BM tos
+.BM l4proto
+.BM src-port
+.BM dst-port
+.BM spi
+.BM vlan-etype
+.BM vlan
+.BM user-def
+.BN action
+.BN loc
+.RB ]
 
 .SH DESCRIPTION
 .BI ethtool
@@ -630,12 +620,18 @@ Default region is 0 which denotes all regions in the flash.
 .TP
 .B \-u \-\-show\-ntuple
 Get Rx ntuple filters and actions, then display them to the user.
+.TP
+.BI class-rule \ N
+Retrieves the RX classification rule with the given ID.
 .PD
 .RE
 .TP
 .B \-U \-\-config\-ntuple
 Configure Rx ntuple filters and actions
 .TP
+.BI class-rule-del \ N
+Deletes the RX classification rule with the given ID.
+.TP
 .B flow\-type tcp4|udp4|sctp4|ether
 .TS
 nokeep;
@@ -643,64 +639,61 @@ lB	l.
 tcp4	TCP over IPv4
 udp4	UDP over IPv4
 sctp4	SCTP over IPv4
+ah4	IPSEC AH over IPv4
+esp4	IPSEC ESP over IPv4
+ip4	Raw IPv4
 ether	Ethernet
 .TE
 .TP
-.BI src\-ip \ addr
-Includes the source IP address, specified using dotted-quad notation
-or as a single 32-bit number.
-.TP
-.BI src\-ip\-mask \ mask
-Specify a mask for the source IP address.
-.TP
-.BI dst\-ip \ addr
-Includes the destination IP address.
-.TP
-.BI dst\-ip\-mask \ mask
-Specify a mask for the destination IP address.
-.TP
-.BI src\-port \ port
-Includes the source port.
-.TP
-.BI src\-port\-mask \ mask
-Specify a mask for the source port.
+.BR src \ \*(MA\ [ src-mask \ \*(MA]
+Includes the source MAC address, specified as 6 bytes in hexadecimal
+separated by colons, along with an optional mask.
 .TP
-.BI dst\-port \ port
-Includes the destination port.
+.BR dst \ \*(MA\ [ src-mask \ \*(MA]
+Includes the destination MAC address, specified as 6 bytes in hexadecimal
+separated by colons, along with an optional mask.
 .TP
-.BI dst\-port\-mask \ mask
-Specify a mask for the destination port.
+.BI proto \ N \\fR\ [\\fPproto-mask \ N \\fR]\\fP
+Includes the Ethernet protocol number (ethertype) and an optional mask.
 .TP
-.BI src \ mac\-addr
-Includes the source MAC address, specified as 6 bytes in hexadecimal
-separated by colons.
+.BR src-ip \ \*(PA\ [ src-ip-mask \ \*(PA]
+Specify the source IP address of the incoming packet to
+match along with an optional mask.
 .TP
-.BI src\-mask \ mask
-Specify a mask for the source MAC address.
+.BR dst-ip \ \*(PA\ [ dst-ip-mask \ \*(PA]
+Specify the destination IP address of the incoming packet to
+match along with an optional mask.
 .TP
-.BI dst \ mac\-addr
-Includes the destination MAC address.
+.BI tos \ N \\fR\ [\\fPtos-mask \ N \\fR]\\fP
+Specify the value of the Type of Service field in the incoming packet to
+match along with an optional mask.
 .TP
-.BI dst\-mask \ mask
-Specify a mask for the destination MAC address.
+.BI l4proto \ N \\fR\ [\\fPl4proto-mask \ N \\fR]\\fP
+Includes the layer 4 protocol number and optional mask.
 .TP
-.BI proto \ N
-Includes the Ethernet protocol number (ethertype).
+.BI src-port \ N \\fR\ [\\fPsrc-port-mask \ N \\fR]\\fP
+Specify the value of the source port field (applicable to
+TCP/UDP packets)in the incoming packet to match along with an
+optional mask.
 .TP
-.BI proto\-mask \ mask
-Specify a mask for the Ethernet protocol number.
+.BI dst-port \ N \\fR\ [\\fPdst-port-mask \ N \\fR]\\fP
+Specify the value of the destination port field (applicable to
+TCP/UDP packets)in the incoming packet to match along with an
+optional mask.
 .TP
-.BI vlan \ VLAN\-tag
-Includes the VLAN tag.
+.BI spi \ N \\fR\ [\\fPspi-mask \ N \\fR]\\fP
+Specify the value of the security parameter index field (applicable to
+AH/ESP packets)in the incoming packet to match along with an
+optional mask.
 .TP
-.BI vlan\-mask \ mask
-Specify a mask for the VLAN tag.
+.BI vlan-etype \ N \\fR\ [\\fPvlan-etype-mask \ N \\fR]\\fP
+Includes the VLAN tag Ethertype and an optional mask.
 .TP
-.BI user\-def \ data
-Includes 64-bits of user-specific data.
+.BI vlan \ N \\fR\ [\\fPvlan-mask \ N \\fR]\\fP
+Includes the VLAN tag and an optional mask.
 .TP
-.BI user\-def\-mask \ mask
-Specify a mask for the user-specific data.
+.BI user-def \ N \\fR\ [\\fPuser-def-mask \ N \\fR]\\fP
+Includes 64-bits of user-specific data and an optional mask.
 .TP
 .BI action \ N
 Specifies the Rx queue to send packets to, or some other action.
@@ -711,6 +704,11 @@ lB	l.
 -1	Drop the matched flow
 0 or higher	Rx queue to route the flow
 .TE
+.TP
+.BI loc \ N
+Specify the location/ID to insert the rule. This will overwrite
+any rule present in that location and will not go through any
+of the rule ordering process.
 .SH BUGS
 Not supported (in part or whole) on all network drivers.
 .SH AUTHOR
@@ -724,7 +722,8 @@ Jakub Jelinek,
 Andre Majorel,
 Eli Kupermann,
 Scott Feldman,
-Andi Kleen.
+Andi Kleen,
+Alexander Duyck.
 .SH AVAILABILITY
 .B ethtool
 is available from
diff --git a/ethtool.c b/ethtool.c
index 421fe20..e65979d 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -243,20 +243,26 @@ static struct option {
 		"		equal N | weight W0 W1 ...\n" },
     { "-U", "--config-ntuple", MODE_SCLSRULE, "Configure Rx ntuple filters "
 		"and actions",
-		"		{ flow-type tcp4|udp4|sctp4\n"
-		"		  [ src-ip ADDR [src-ip-mask MASK] ]\n"
-		"		  [ dst-ip ADDR [dst-ip-mask MASK] ]\n"
-		"		  [ src-port PORT [src-port-mask MASK] ]\n"
-		"		  [ dst-port PORT [dst-port-mask MASK] ]\n"
-		"		| flow-type ether\n"
-		"		  [ src MAC-ADDR [src-mask MASK] ]\n"
-		"		  [ dst MAC-ADDR [dst-mask MASK] ]\n"
-		"		  [ proto N [proto-mask MASK] ] }\n"
-		"		[ vlan VLAN-TAG [vlan-mask MASK] ]\n"
-		"		[ user-def DATA [user-def-mask MASK] ]\n"
-		"		action N\n" },
+		"		[ class-rule-del %d ] |\n"
+		"		[ flow-type ether|ip4|tcp4|udp4|sctp4|ah4|esp4\n"
+		"			[ src %x:%x:%x:%x:%x:%x [src-mask %x:%x:%x:%x:%x:%x] ]\n"
+		"			[ dst %x:%x:%x:%x:%x:%x [dst-mask %x:%x:%x:%x:%x:%x] ]\n"
+		"			[ proto %d [proto-mask MASK] ]\n"
+		"			[ src-ip %d.%d.%d.%d [src-ip-mask %d.%d.%d.%d] ]\n"
+		"			[ dst-ip %d.%d.%d.%d [dst-ip-mask %d.%d.%d.%d] ]\n"
+		"			[ tos %d [tos-mask %x] ]\n"
+		"			[ l4proto %d [l4proto-mask MASK] ]\n"
+		"			[ src-port %d [src-port-mask %x] ]\n"
+		"			[ dst-port %d [dst-port-mask %x] ]\n"
+		"			[ spi %d [spi-mask %x] ]\n"
+		"			[ vlan-etype %x [vlan-etype-mask %x] ]\n"
+		"			[ vlan %x [vlan-mask %x] ]\n"
+		"			[ user-def %x [user-def-mask %x] ]\n"
+		"			[ action %d ]\n"
+		"			[ loc %d]]\n" },
     { "-u", "--show-ntuple", MODE_GCLSRULE,
-		"Get Rx ntuple filters and actions\n" },
+		"Get Rx ntuple filters and actions",
+		"		[ class-rule %d ]\n"},
     { "-P", "--show-permaddr", MODE_PERMADDR,
 		"Show permanent hardware address" },
     { "-h", "--help", MODE_HELP, "Show this help" },

Re: [ethtool PATCH 6/6] Update documentation for -u/-U operations

[Ben Hutchings]

On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
> This patch updates the documentation for the -u/-U operations to include
> the recent changes made to support addition/deletion/display of network
> flow classifier rules.

This should be included in the same patch.

> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> 
>  ethtool.8.in |  185 +++++++++++++++++++++++++++++-----------------------------
>  ethtool.c    |   32 ++++++----
>  2 files changed, 111 insertions(+), 106 deletions(-)
> 
> diff --git a/ethtool.8.in b/ethtool.8.in
> index 12a1d1d..8908351 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -42,10 +42,20 @@
>  [\\fB\\$1\\fP\ \\fIN\\fP]
>  ..
>  .\"
> +.\"	.BM - same as above but has a mask field for format "[value N [value-mask N]]"
> +.\"
> +.de BM
> +[\\fB\\$1\\fP\ \\fIN\\fP\ [\\fB\\$1\-mask\\fP\ \\fIN\\fP]]

You've changed the code to accept 'm' as an alternative to
<field> '-mask', so this should be changed accordingly.

[...]
> @@ -236,9 +252,9 @@ ethtool \- query or control network driver and hardware settings
>  .HP
>  .B ethtool \-N
>  .I ethX
> -.RB [ rx\-flow\-hash \ \*(FL
> -.RB \ \*(HO]
> +.RB [ rx-flow-hash \ \*(FL \  \*(HO]
>  .HP
> +

This looks like an unintentional reversion of part of commit
db6c0cee6cd956767e1c39109fe81104cc4694cb.

>  .B ethtool \-x|\-\-show\-rxfh\-indir
>  .I ethX
>  .HP
> @@ -257,54 +273,28 @@ ethtool \- query or control network driver and hardware settings
>  .HP
>  .B ethtool \-u|\-\-show\-ntuple
>  .I ethX
> -.TP
> +.BN class-rule
> +.HP
> +
>  .BI ethtool\ \-U|\-\-config\-ntuple \ ethX
> -.RB {
> -.A3 flow\-type tcp4 udp4 sctp4
> -.RB [ src\-ip
> -.IR addr
> -.RB [ src\-ip\-mask
> -.IR mask ]]
> -.RB [ dst\-ip
> -.IR addr
> -.RB [ dst\-ip\-mask
> -.IR mask ]]
> -.RB [ src\-port
> -.IR port
> -.RB [ src\-port\-mask
> -.IR mask ]]
> -.RB [ dst\-port
> -.IR port
> -.RB [ dst\-port\-mask
> -.IR mask ]]
> -.br
> -.RB | \ flow\-type\ ether
> -.RB [ src
> -.IR mac\-addr
> -.RB [ src\-mask
> -.IR mask ]]
> -.RB [ dst
> -.IR mac\-addr
> -.RB [ dst\-mask
> -.IR mask ]]
> -.RB [ proto
> -.IR N
> -.RB [ proto\-mask
> -.IR mask ]]\ }
> -.br
> -.RB [ vlan
> -.IR VLAN\-tag
> -.RB [ vlan\-mask
> -.IR mask ]]
> -.RB [ user\-def
> -.IR data
> -.RB [ user\-def\-mask
> -.IR mask ]]
> -.RI action \ N
> -.
> -.\" Adjust lines (i.e. full justification) and hyphenate.
> -.ad
> -.hy

As do the last 3 deleted lines here.

> +.BN class-rule-del
> +.RB [\  flow-type \ \*(NC
> +.RB [ src \ \*(MA\ [ src-mask \ \*(MA]]
> +.RB [ dst \ \*(MA\ [ dst-mask \ \*(MA]]
> +.BM proto
> +.RB [ src-ip \ \*(PA\ [ src-ip-mask \ \*(PA]]
> +.RB [ dst-ip \ \*(PA\ [ dst-ip-mask \ \*(PA]]
> +.BM tos
> +.BM l4proto
> +.BM src-port
> +.BM dst-port
> +.BM spi
> +.BM vlan-etype
> +.BM vlan
> +.BM user-def
> +.BN action
> +.BN loc
> +.RB ]

But these options aren't all applicable to all flow-types.
 
[...]
> diff --git a/ethtool.c b/ethtool.c
> index 421fe20..e65979d 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -243,20 +243,26 @@ static struct option {
>  		"		equal N | weight W0 W1 ...\n" },
>      { "-U", "--config-ntuple", MODE_SCLSRULE, "Configure Rx ntuple filters "
>  		"and actions",
> -		"		{ flow-type tcp4|udp4|sctp4\n"
> -		"		  [ src-ip ADDR [src-ip-mask MASK] ]\n"
> -		"		  [ dst-ip ADDR [dst-ip-mask MASK] ]\n"
> -		"		  [ src-port PORT [src-port-mask MASK] ]\n"
> -		"		  [ dst-port PORT [dst-port-mask MASK] ]\n"
> -		"		| flow-type ether\n"
> -		"		  [ src MAC-ADDR [src-mask MASK] ]\n"
> -		"		  [ dst MAC-ADDR [dst-mask MASK] ]\n"
> -		"		  [ proto N [proto-mask MASK] ] }\n"
> -		"		[ vlan VLAN-TAG [vlan-mask MASK] ]\n"
> -		"		[ user-def DATA [user-def-mask MASK] ]\n"
> -		"		action N\n" },
> +		"		[ class-rule-del %d ] |\n"
> +		"		[ flow-type ether|ip4|tcp4|udp4|sctp4|ah4|esp4\n"
> +		"			[ src %x:%x:%x:%x:%x:%x [src-mask %x:%x:%x:%x:%x:%x] ]\n"
> +		"			[ dst %x:%x:%x:%x:%x:%x [dst-mask %x:%x:%x:%x:%x:%x] ]\n"
> +		"			[ proto %d [proto-mask MASK] ]\n"
> +		"			[ src-ip %d.%d.%d.%d [src-ip-mask %d.%d.%d.%d] ]\n"
> +		"			[ dst-ip %d.%d.%d.%d [dst-ip-mask %d.%d.%d.%d] ]\n"
> +		"			[ tos %d [tos-mask %x] ]\n"
> +		"			[ l4proto %d [l4proto-mask MASK] ]\n"
> +		"			[ src-port %d [src-port-mask %x] ]\n"
> +		"			[ dst-port %d [dst-port-mask %x] ]\n"
> +		"			[ spi %d [spi-mask %x] ]\n"
> +		"			[ vlan-etype %x [vlan-etype-mask %x] ]\n"
> +		"			[ vlan %x [vlan-mask %x] ]\n"
> +		"			[ user-def %x [user-def-mask %x] ]\n"
> +		"			[ action %d ]\n"
> +		"			[ loc %d]]\n" },
[...]

Again, it's not clear which options apply to which flow-types, and the
'm' shortcut is not documented.

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: [ethtool PATCH 6/6] Update documentation for -u/-U operations

[Alexander Duyck]

On 4/27/2011 11:23 AM, Ben Hutchings wrote:
> On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
>> This patch updates the documentation for the -u/-U operations to include
>> the recent changes made to support addition/deletion/display of network
>> flow classifier rules.
>
> This should be included in the same patch.

I'll combine the two patches for the next release if that is what is 
preferred.  I was just trying to keep the overall size down by splitting 
them since this is documentation only.

>> Signed-off-by: Alexander Duyck<alexander.h.duyck@intel.com>
>> ---
>>
>>   ethtool.8.in |  185 +++++++++++++++++++++++++++++-----------------------------
>>   ethtool.c    |   32 ++++++----
>>   2 files changed, 111 insertions(+), 106 deletions(-)
>>
>> diff --git a/ethtool.8.in b/ethtool.8.in
>> index 12a1d1d..8908351 100644
>> --- a/ethtool.8.in
>> +++ b/ethtool.8.in
>> @@ -42,10 +42,20 @@
>>   [\\fB\\$1\\fP\ \\fIN\\fP]
>>   ..
>>   .\"
>> +.\"	.BM - same as above but has a mask field for format "[value N [value-mask N]]"
>> +.\"
>> +.de BM
>> +[\\fB\\$1\\fP\ \\fIN\\fP\ [\\fB\\$1\-mask\\fP\ \\fIN\\fP]]
>
> You've changed the code to accept 'm' as an alternative to
> <field>  '-mask', so this should be changed accordingly.

What would be the preferred way of stating that?  For now I just 
replaced the \\$1\-mask with m.  However I am assuming that probably 
isn't the best approach either.  Should I state somewhere that m can be 
replaced with "field name"-mask?

> [...]
>> @@ -236,9 +252,9 @@ ethtool \- query or control network driver and hardware settings
>>   .HP
>>   .B ethtool \-N
>>   .I ethX
>> -.RB [ rx\-flow\-hash \ \*(FL
>> -.RB \ \*(HO]
>> +.RB [ rx-flow-hash \ \*(FL \  \*(HO]
>>   .HP
>> +
>
> This looks like an unintentional reversion of part of commit
> db6c0cee6cd956767e1c39109fe81104cc4694cb.

Yeah, my bad.  I will have it updated for the next patch.  I only really 
meant to combine this into one line, I didn't intend to drop the "\-" 
formatting fixes you added.

>>   .B ethtool \-x|\-\-show\-rxfh\-indir
>>   .I ethX
>>   .HP
>> @@ -257,54 +273,28 @@ ethtool \- query or control network driver and hardware settings
>>   .HP
>>   .B ethtool \-u|\-\-show\-ntuple
>>   .I ethX
>> -.TP
>> +.BN class-rule
>> +.HP
>> +
>>   .BI ethtool\ \-U|\-\-config\-ntuple \ ethX
>> -.RB {
>> -.A3 flow\-type tcp4 udp4 sctp4
>> -.RB [ src\-ip
>> -.IR addr
>> -.RB [ src\-ip\-mask
>> -.IR mask ]]
>> -.RB [ dst\-ip
>> -.IR addr
>> -.RB [ dst\-ip\-mask
>> -.IR mask ]]
>> -.RB [ src\-port
>> -.IR port
>> -.RB [ src\-port\-mask
>> -.IR mask ]]
>> -.RB [ dst\-port
>> -.IR port
>> -.RB [ dst\-port\-mask
>> -.IR mask ]]
>> -.br
>> -.RB | \ flow\-type\ ether
>> -.RB [ src
>> -.IR mac\-addr
>> -.RB [ src\-mask
>> -.IR mask ]]
>> -.RB [ dst
>> -.IR mac\-addr
>> -.RB [ dst\-mask
>> -.IR mask ]]
>> -.RB [ proto
>> -.IR N
>> -.RB [ proto\-mask
>> -.IR mask ]]\ }
>> -.br
>> -.RB [ vlan
>> -.IR VLAN\-tag
>> -.RB [ vlan\-mask
>> -.IR mask ]]
>> -.RB [ user\-def
>> -.IR data
>> -.RB [ user\-def\-mask
>> -.IR mask ]]
>> -.RI action \ N
>> -.
>> -.\" Adjust lines (i.e. full justification) and hyphenate.
>> -.ad
>> -.hy
>
> As do the last 3 deleted lines here.

I put them back so they should be left in place for the next version.

>> +.BN class-rule-del
>> +.RB [\  flow-type \ \*(NC
>> +.RB [ src \ \*(MA\ [ src-mask \ \*(MA]]
>> +.RB [ dst \ \*(MA\ [ dst-mask \ \*(MA]]
>> +.BM proto
>> +.RB [ src-ip \ \*(PA\ [ src-ip-mask \ \*(PA]]
>> +.RB [ dst-ip \ \*(PA\ [ dst-ip-mask \ \*(PA]]
>> +.BM tos
>> +.BM l4proto
>> +.BM src-port
>> +.BM dst-port
>> +.BM spi
>> +.BM vlan-etype
>> +.BM vlan
>> +.BM user-def
>> +.BN action
>> +.BN loc
>> +.RB ]
>
> But these options aren't all applicable to all flow-types.

This is the part that gets messy and I am not sure what the best 
approach is.  I have more comments on that below.  For now what I am 
planning to implement to address this is that in the "DESCRIPTION" 
section below I add a statement to each specifier that has restrictions 
by stating "Valid for flow-types X, Y, and Z."

> [...]
>> diff --git a/ethtool.c b/ethtool.c
>> index 421fe20..e65979d 100644
>> --- a/ethtool.c
>> +++ b/ethtool.c
>> @@ -243,20 +243,26 @@ static struct option {
>>   		"		equal N | weight W0 W1 ...\n" },
>>       { "-U", "--config-ntuple", MODE_SCLSRULE, "Configure Rx ntuple filters "
>>   		"and actions",
>> -		"		{ flow-type tcp4|udp4|sctp4\n"
>> -		"		  [ src-ip ADDR [src-ip-mask MASK] ]\n"
>> -		"		  [ dst-ip ADDR [dst-ip-mask MASK] ]\n"
>> -		"		  [ src-port PORT [src-port-mask MASK] ]\n"
>> -		"		  [ dst-port PORT [dst-port-mask MASK] ]\n"
>> -		"		| flow-type ether\n"
>> -		"		  [ src MAC-ADDR [src-mask MASK] ]\n"
>> -		"		  [ dst MAC-ADDR [dst-mask MASK] ]\n"
>> -		"		  [ proto N [proto-mask MASK] ] }\n"
>> -		"		[ vlan VLAN-TAG [vlan-mask MASK] ]\n"
>> -		"		[ user-def DATA [user-def-mask MASK] ]\n"
>> -		"		action N\n" },
>> +		"		[ class-rule-del %d ] |\n"
>> +		"		[ flow-type ether|ip4|tcp4|udp4|sctp4|ah4|esp4\n"
>> +		"			[ src %x:%x:%x:%x:%x:%x [src-mask %x:%x:%x:%x:%x:%x] ]\n"
>> +		"			[ dst %x:%x:%x:%x:%x:%x [dst-mask %x:%x:%x:%x:%x:%x] ]\n"
>> +		"			[ proto %d [proto-mask MASK] ]\n"
>> +		"			[ src-ip %d.%d.%d.%d [src-ip-mask %d.%d.%d.%d] ]\n"
>> +		"			[ dst-ip %d.%d.%d.%d [dst-ip-mask %d.%d.%d.%d] ]\n"
>> +		"			[ tos %d [tos-mask %x] ]\n"
>> +		"			[ l4proto %d [l4proto-mask MASK] ]\n"
>> +		"			[ src-port %d [src-port-mask %x] ]\n"
>> +		"			[ dst-port %d [dst-port-mask %x] ]\n"
>> +		"			[ spi %d [spi-mask %x] ]\n"
>> +		"			[ vlan-etype %x [vlan-etype-mask %x] ]\n"
>> +		"			[ vlan %x [vlan-mask %x] ]\n"
>> +		"			[ user-def %x [user-def-mask %x] ]\n"
>> +		"			[ action %d ]\n"
>> +		"			[ loc %d]]\n" },
> [...]
>
> Again, it's not clear which options apply to which flow-types, and the
> 'm' shortcut is not documented.

The 'm' part I agree with 100%, however the flow types are going to 
become kinda hairy using that approach.  You basically end up with 
something like this:

	flow-type ether
		[ src %x:%x:%x:%x:%x:%x [src-mask %x:%x:%x:%x:%x:%x]]
		[ dst %x:%x:%x:%x:%x:%x [dst-mask %x:%x:%x:%x:%x:%x]]
		[ proto %d [proto-mask %x]]
		[ vlan-etype %x [vlan-etype-mask %x]]
		[ vlan %x [vlan-mask %x]]
		[ user-def %x [user-def-mask %x]]
		[ action %d ]
		[ loc %d]
	flow-type ip4
		[ src-ip %d.%d.%d.%d [src-ip-mask %d.%d.%d.%d]]
		[ dst-ip %d.%d.%d.%d [dst-ip-mask %d.%d.%d.%d]]
		[ tos %d [tos-mask %x]]
		[ l4proto %d [l4proto-mask %x]]
		[ src-port %d [src-port-mask %x]]
		[ dst-port %d [dst-port-mask %x]]
		[ spi %d [spi-mask %x]]
		[ vlan-etype %x [vlan-etype-mask %x]]
		[ vlan %x [vlan-mask %x]]
		[ user-def %x [user-def-mask %x]]
		[ action %d ]
		[ loc %d]
	flow-type tcp4|udp4|sctp4
		[ src-ip %d.%d.%d.%d [src-ip-mask %d.%d.%d.%d]]
		[ dst-ip %d.%d.%d.%d [dst-ip-mask %d.%d.%d.%d]]
		[ tos %d [tos-mask %x]]
		[ src-port %d [src-port-mask %x]]
		[ dst-port %d [dst-port-mask %x]]
		[ vlan-etype %x [vlan-etype-mask %x]]
		[ vlan %x [vlan-mask %x]]
		[ user-def %x [user-def-mask %x]]
		[ action %d ]
		[ loc %d]
	flow-type ah4|esp4
		[ src-ip %d.%d.%d.%d [src-ip-mask %d.%d.%d.%d]]
		[ dst-ip %d.%d.%d.%d [dst-ip-mask %d.%d.%d.%d]]
		[ tos %d [tos-mask %x]]
		[ spi %d [spi-mask %x]]
		[ vlan-etype %x [vlan-etype-mask %x]]
		[ vlan %x [vlan-mask %x]]
		[ user-def %x [user-def-mask %x]]
		[ action %d ]
		[ loc %d]

As you can see it will be a bit oversized to go through and specify 
which flow-type options support what fields.  If that is what you want I 
can implement it that way but for now I would prefer calling out the 
flow-type limitations of the fields in the "DESCRIPTION" portion of the 
man page.

>
> Ben.
>

Thanks,

Alex

Re: [ethtool PATCH 6/6] Update documentation for -u/-U operations

[Ben Hutchings]

On Thu, 2011-04-28 at 13:40 -0700, Alexander Duyck wrote:
> On 4/27/2011 11:23 AM, Ben Hutchings wrote:
> > On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
[...]
> >> --- a/ethtool.8.in
> >> +++ b/ethtool.8.in
> >> @@ -42,10 +42,20 @@
> >>   [\\fB\\$1\\fP\ \\fIN\\fP]
> >>   ..
> >>   .\"
> >> +.\"	.BM - same as above but has a mask field for format "[value N [value-mask N]]"
> >> +.\"
> >> +.de BM
> >> +[\\fB\\$1\\fP\ \\fIN\\fP\ [\\fB\\$1\-mask\\fP\ \\fIN\\fP]]
> >
> > You've changed the code to accept 'm' as an alternative to
> > <field>  '-mask', so this should be changed accordingly.
> 
> What would be the preferred way of stating that?  For now I just 
> replaced the \\$1\-mask with m.  However I am assuming that probably 
> isn't the best approach either.  Should I state somewhere that m can be 
> replaced with "field name"-mask?

I think that's reasonable.

[...]
> >> +.BN class-rule-del
> >> +.RB [\  flow-type \ \*(NC
> >> +.RB [ src \ \*(MA\ [ src-mask \ \*(MA]]
> >> +.RB [ dst \ \*(MA\ [ dst-mask \ \*(MA]]
> >> +.BM proto
> >> +.RB [ src-ip \ \*(PA\ [ src-ip-mask \ \*(PA]]
> >> +.RB [ dst-ip \ \*(PA\ [ dst-ip-mask \ \*(PA]]
> >> +.BM tos
> >> +.BM l4proto
> >> +.BM src-port
> >> +.BM dst-port
> >> +.BM spi
> >> +.BM vlan-etype
> >> +.BM vlan
> >> +.BM user-def
> >> +.BN action
> >> +.BN loc
> >> +.RB ]
> >
> > But these options aren't all applicable to all flow-types.
> 
> This is the part that gets messy and I am not sure what the best 
> approach is.  I have more comments on that below.  For now what I am 
> planning to implement to address this is that in the "DESCRIPTION" 
> section below I add a statement to each specifier that has restrictions 
> by stating "Valid for flow-types X, Y, and Z."

OK.

> > [...]
> >> diff --git a/ethtool.c b/ethtool.c
> >> index 421fe20..e65979d 100644
> >> --- a/ethtool.c
> >> +++ b/ethtool.c
> >> @@ -243,20 +243,26 @@ static struct option {
> >>   		"		equal N | weight W0 W1 ...\n" },
> >>       { "-U", "--config-ntuple", MODE_SCLSRULE, "Configure Rx ntuple filters "
> >>   		"and actions",
> >> -		"		{ flow-type tcp4|udp4|sctp4\n"
> >> -		"		  [ src-ip ADDR [src-ip-mask MASK] ]\n"
> >> -		"		  [ dst-ip ADDR [dst-ip-mask MASK] ]\n"
> >> -		"		  [ src-port PORT [src-port-mask MASK] ]\n"
> >> -		"		  [ dst-port PORT [dst-port-mask MASK] ]\n"
> >> -		"		| flow-type ether\n"
> >> -		"		  [ src MAC-ADDR [src-mask MASK] ]\n"
> >> -		"		  [ dst MAC-ADDR [dst-mask MASK] ]\n"
> >> -		"		  [ proto N [proto-mask MASK] ] }\n"
> >> -		"		[ vlan VLAN-TAG [vlan-mask MASK] ]\n"
> >> -		"		[ user-def DATA [user-def-mask MASK] ]\n"
> >> -		"		action N\n" },
> >> +		"		[ class-rule-del %d ] |\n"
> >> +		"		[ flow-type ether|ip4|tcp4|udp4|sctp4|ah4|esp4\n"
> >> +		"			[ src %x:%x:%x:%x:%x:%x [src-mask %x:%x:%x:%x:%x:%x] ]\n"
> >> +		"			[ dst %x:%x:%x:%x:%x:%x [dst-mask %x:%x:%x:%x:%x:%x] ]\n"
> >> +		"			[ proto %d [proto-mask MASK] ]\n"
> >> +		"			[ src-ip %d.%d.%d.%d [src-ip-mask %d.%d.%d.%d] ]\n"
> >> +		"			[ dst-ip %d.%d.%d.%d [dst-ip-mask %d.%d.%d.%d] ]\n"
> >> +		"			[ tos %d [tos-mask %x] ]\n"
> >> +		"			[ l4proto %d [l4proto-mask MASK] ]\n"
> >> +		"			[ src-port %d [src-port-mask %x] ]\n"
> >> +		"			[ dst-port %d [dst-port-mask %x] ]\n"
> >> +		"			[ spi %d [spi-mask %x] ]\n"
> >> +		"			[ vlan-etype %x [vlan-etype-mask %x] ]\n"
> >> +		"			[ vlan %x [vlan-mask %x] ]\n"
> >> +		"			[ user-def %x [user-def-mask %x] ]\n"
> >> +		"			[ action %d ]\n"
> >> +		"			[ loc %d]]\n" },
> > [...]
> >
> > Again, it's not clear which options apply to which flow-types, and the
> > 'm' shortcut is not documented.
> 
> The 'm' part I agree with 100%, however the flow types are going to 
> become kinda hairy using that approach.  You basically end up with 
> something like this:
[...]

Yes, I see the problem.

> As you can see it will be a bit oversized to go through and specify 
> which flow-type options support what fields.  If that is what you want I 
> can implement it that way but for now I would prefer calling out the 
> flow-type limitations of the fields in the "DESCRIPTION" portion of the 
> man page.

In fact, even with this patch, the help for -U is pretty oversized.  It
might be better to replace the list of field names with '...' here and
only list them in full in the man page.

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: [ethtool PATCH 0/6] Network flow classifier

[Ben Hutchings]

On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
> The following patches are meant to add support for the network flow classifier
> interface built into the ethtool_rxnfc.
> 
> Once this has been accepted I plan to submit the kernel portion to Jeff
> Kirsher as it is almost all ixgbe code changes with the exception of the
> removal of support for ethtool_get_rx_ntuple from the kernel.
[...]

This is the start of a 4-way weekend in the UK, so don't be surprised if
I take a while to review these changes.

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: [ethtool PATCH 0/6] Network flow classifier

[Alexander Duyck]

On 4/21/2011 1:51 PM, Ben Hutchings wrote:
> On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
>> The following patches are meant to add support for the network flow classifier
>> interface built into the ethtool_rxnfc.
>>
>> Once this has been accepted I plan to submit the kernel portion to Jeff
>> Kirsher as it is almost all ixgbe code changes with the exception of the
>> removal of support for ethtool_get_rx_ntuple from the kernel.
> [...]
>
> This is the start of a 4-way weekend in the UK, so don't be surprised if
> I take a while to review these changes.
>
> Ben.
>

I'm fine with that.  I wasn't in any big hurry.  I was mostly just 
getting them out there for review so that I can probably see about 
polishing them up and submitting the final version for them next week.

Thanks,

Alex