Add support for RX network flow classifier rules

This change adds a macro for displaying optional values that take a numeric value to the manpage.

From: Santwona Behera <[email protected]>

This patch was originally introduced as:
[PATCH 1/3] [ethtool] Add rx pkt classification interface

Signed-off-by Santwona Behera <[email protected]>

http://patchwork.ozlabs.org/patch/23223/

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.

I also added an ethtool bitops interface to allow for simple bit set and test activities since the rule manager can most efficiently store the list of active rules via a bitmap.

ethtool-bitops.h new file +17 -0

+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])) != 0UL;
+}
+
+#endif

ethtool-util.h +0 -0

index f053028..e9300e2 100644

ethtool-util.h +8 -0

+int rxclass_parse_ruleopts(char **optstr, int opt_cnt,
+			   struct ethtool_rx_flow_spec *fsp, __u8 *loc_valid);
+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, __u8 loc_valid);
+int rxclass_rule_del(int fd, struct ifreq *ifr, __u32 loc);
+
 #endif

ethtool.8.in +8 -0

index 133825b..c183a3d 100644
@@ -40,21 +40,36 @@
 [\\fB\\$1\\fP\ \\fIN\\fP]
..
 .\"
+.\"	.BM - same as above but has a mask field for format "[value N [m N]]"
+.\"
+.de BM
+[\\fB\\$1\\fP\ \\fIN\\fP\ [\\fBm\\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
+.\"

.\"
.\" \(*HO - hash options
.\"
.ds HO \fBm\fP|\fBv\fP|\fBt\fP|\fBs\fP|\fBd\fP|\fBf\fP|\fBn\fP|\fBr\fP...
+.\"
+.\" \(*L4 - L4 proto options
+.\"
+.ds L4 \fBtcp\fP|\fBudp\fP|\fBsctp\fP|\fBah\fP|\fBesp\fP|\fIN\fP +.\"
.\" Start URL.
.de UR
. ds m1 \\$1\"

ethtool.c +2 -0

index 1afdfe4..b624980 100644
@@ -6,6 +6,7 @@
  * Kernel 2.4 update Copyright 2001 Jeff Garzik <[email protected]>
  * Wake-on-LAN,natsemi,misc support by Tim Hockin <[email protected]>
  * Portions Copyright 2002 Intel
+ * Portions Copyright (C) Sun Microsystems 2008
  * do_test support by Eli Kupermann <[email protected]>
  * ETHTOOL_PHYS_ID support by Chris Leech <[email protected]>
  * e1000 support by Scott Feldman <[email protected]>

@@ -14,6 +15,7 @@
  * amd8111e support by Reeja John <[email protected]>
  * long arguments by Andi Kleen.
  * SMSC LAN911x support by Steve Glendinning <[email protected]>
+ * Rx Network Flow Control configuration support <[email protected]>
  * Various features by Ben Hutchings <[email protected]>;
  *	Copyright 2009, 2010 Solarflare Communications
  *

ethtool.c +8 -0

@@ -408,6 +425,14 @@ static int msglvl_changed;
 static u32 msglvl_wanted = 0;
 static u32 msglvl_mask = 0;
 
+static int rx_rings_get = 0;
+static int rx_class_rule_get = -1;
+static int rx_class_rule_getall = 0;
+static int rx_class_rule_del = -1;
+static int rx_class_rule_added = 0;
+static struct ethtool_rx_flow_spec rx_rule_fs;
+static u8 rxclass_loc_valid = 0;
+
 static enum {
 	ONLINE=0,
 	OFFLINE,

ethtool.c +4 -0

+				} else if (!strcmp(argp[i],
+						   "rx-class-rule-all")) {
+					i += 1;
+					rx_class_rule_getall = 1;

ethtool.c +1 -1

@@ -978,8 +1024,37 @@ static void parse_cmdline(int argc, char **argp)
 						show_usage(1);
 					else
 						rx_fhash_changed = 1;
-				} else
+				} else if (!strcmp(argp[i],

ethtool.c +19 -0

+				} else if (!strcmp(argp[i],
+						   "rx-class-rule-add")) {
+					i += 1;
+					if (i >= argc) {
+						show_usage(1);
+						break;
+					}
+					if (rxclass_parse_ruleopts(&argp[i],
+								   argc - i,
+								   &rx_rule_fs,
+								   &rxclass_loc_valid) < 0) {
+						show_usage(1);
+					} else {
+						i = argc;
+						rx_class_rule_added = 1;
+					}
+				} else {
 					show_usage(1);
+				}
+
 				break;
 			}
 			if (mode == MODE_SRXFHINDIR) {

ethtool.c +4 -0

+	case AH_V4_FLOW:
 		fprintf(stdout, "IPSEC AH over IPV4 flows");
 		break;
+	case ESP_V4_FLOW:
+		fprintf(stdout, "IPSEC ESP over IPV4 flows");
+		break;
 	case TCP_V6_FLOW:
 		fprintf(stdout, "TCP over IPV6 flows");
 		break;

ethtool.c +11 -3

+	case AH_V6_FLOW:
 		fprintf(stdout, "IPSEC AH over IPV6 flows");
 		break;
+	case ESP_V6_FLOW:
+		fprintf(stdout, "IPSEC ESP over IPV6 flows");
+		break;
 	default:
 		break;
 	}

@@ -2911,14 +2992,12 @@ static int do_gstats(int fd, struct ifreq *ifr)
 	return 0;
 }
 
-
 static int do_srxclass(int fd, struct ifreq *ifr)
 {
 	int err;
+	struct ethtool_rxnfc nfccmd;
 
 	if (rx_fhash_changed) {
-		struct ethtool_rxnfc nfccmd;
-
 		nfccmd.cmd = ETHTOOL_SRXFH;
 		nfccmd.flow_type = rx_fhash_set;
 		nfccmd.data = rx_fhash_val;

@@ -2930,6 +3009,20 @@ static int do_srxclass(int fd, struct ifreq *ifr)
 
 	}
 
+	if (rx_class_rule_added) {
+		err = rxclass_rule_ins(fd, ifr, &rx_rule_fs,
+				       rxclass_loc_valid);
+		if (err < 0)
+			fprintf(stderr, "Cannot insert RX classification rule\n");
+	}

ethtool.c +17 -0

@@ -2950,6 +3043,31 @@ static int do_grxclass(int fd, struct ifreq *ifr)
 			dump_rxfhash(rx_fhash_get, nfccmd.data);
 	}
 
+	if (rx_rings_get) {
+		struct ethtool_rxnfc nfccmd;
+
+		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);
+	}
+
+	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");

rxclass.c new file +25 -0

new file mode 100644
index 0000000..fd01a32
@@ -0,0 +1,809 @@
+/*
+ * 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 <strings.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;

rxclass.c new file +18 -0

+static void rmgr_print_ipv4_rule(struct ethtool_rx_flow_spec *fsp)
+{
+	char		chan[16];
+	char		l4_proto[16];
+	__u32		sip, dip, sipm, dipm;
+
+	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);
+
+	if (fsp->ring_cookie != RX_CLS_FLOW_DISC)
+		sprintf(chan, "Rx Ring [%d]", (int)fsp->ring_cookie);
+	else
+		sprintf(chan, "Discard");
+
+	switch (fsp->flow_type) {
+	case TCP_V4_FLOW:

rxclass.c new file +76 -0

+int rxclass_parse_ruleopts(char **optstr, int opt_cnt,
+			   struct ethtool_rx_flow_spec *fsp,
+			   u_int8_t *loc_valid)
+{
+	int i = 0;
+
+	u_int8_t discard, ring_set;
+	u_int32_t ipsa, ipsm, ipda, ipdm, spi, spim;
+	u_int16_t sp, spm, dp, dpm;
+	u_int8_t ip_ver, proto, tos, tm;
+	struct in_addr in_addr;
+
+	if (*optstr == NULL || **optstr == '\0' || opt_cnt < 2) {
+		fprintf(stdout, "Add rule, invalid syntax\n");
+		return -1;
+	}
+
+	bzero(fsp, sizeof(struct ethtool_rx_flow_spec));
+	ipsa = ipda = ipsm = ipdm = spi = spim = 0x0;
+	sp = dp = spm = dpm = 0x0;
+	ip_ver = proto = tos = tm = 0x0;
+	discard = ring_set = 0;
+
+	if (!strcmp(optstr[i], "ip4")) {
+		ip_ver = ETH_RX_NFC_IP4;
+	} else if (!strcmp(optstr[i], "ip6")) {
+		fprintf(stdout, "IPv6 not yet implemented\n");
+		return -1;
+	} else {
+		fprintf(stdout, "Add rule, invalid syntax for IP version\n");
+		return -1;
+	}
+
+	i++;
+
+	switch (ip_ver) {
+	case ETH_RX_NFC_IP4:
+		if (!strcmp(optstr[i], "tcp"))
+			fsp->flow_type = TCP_V4_FLOW;
+		else if (!strcmp(optstr[i], "udp"))
+			fsp->flow_type = UDP_V4_FLOW;
+		else if (!strcmp(optstr[i], "sctp"))
+			fsp->flow_type = SCTP_V4_FLOW;
+		else if (!strcmp(optstr[i], "ah"))
+			fsp->flow_type = AH_V4_FLOW;
+		else if (!strcmp(optstr[i], "esp"))
+			fsp->flow_type = ESP_V4_FLOW;
+		break;
+	default:
+		fprintf(stdout, "Add rule, Invalid IP version %d\n", ip_ver);
+			return -1;
+	}
+
+	if (fsp->flow_type == 0) {
+		proto = (u_int8_t)strtoul(optstr[i], (char **)NULL, 0);
+		if (proto != 0) {
+			fprintf(stdout, "Add rule, user defined proto %d\n",
+				proto);
+			fsp->flow_type = IP_USER_FLOW;
+			fsp->h_u.usr_ip4_spec.proto = proto;
+			fsp->h_u.usr_ip4_spec.ip_ver = ip_ver;
+		} else {
+			fprintf(stdout, "Add rule, invalid IP proto %s\n",
+				optstr[i]);
+			return -1;
+		}
+	}
+
+	for (i = 2; i < opt_cnt;) {
+		if (!strcmp(optstr[i], "tos")) {
+			tos = (u_int8_t)strtoul(optstr[i+1], (char **)NULL,
+						 0);
+			tm = 0xff;
+			fsp->h_u.tcp_ip4_spec.tos = tos;
+
+			i += 2;

Quoted 23 lines · Alexander Duyck

From: Santwona Behera<[email protected]>

This patch was originally introduced as:
[PATCH 1/3] [ethtool] Add rx pkt classification interface

Signed-off-by Santwona Behera<[email protected]>

http://patchwork.ozlabs.org/patch/23223/

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.

I also added an ethtool bitops interface to allow for simple bit set and test activities since the rule manager can most efficiently store the list of active rules via a bitmap.

[...]

ethtool-bitops.h line 4

Quoted 10 lines

ethtool-bitops.h new file +4 -0

new file mode 100644
index 0000000..7101056
@@ -0,0 +1,25 @@
+#ifndef ETHTOOL_BITOPS_H__
+#define ETHTOOL_BITOPS_H__
+
+#define BITS_PER_LONG                __WORDSIZE

This seems to be a glibc-internal macro and I don't think we should use it.

ethtool-bitops.h line 7

Quoted 3 lines

ethtool-bitops.h new file +3 -0

+#define BITS_PER_BYTE                8
+#define DIV_ROUND_UP(n, d)   (((n) + (d) - 1) / (d))
+#define BITS_TO_LONGS(nr)    DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))

In fact I notice you don't use it here...

Quoted 23 lines

ethtool-bitops.h new file +17 -0

+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])) != 0UL;
+}
+
+#endif

ethtool-util.h +0 -0

index f053028..e9300e2 100644
@@ -103,4 +103,17 @@ int sfc_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
  int st_mac100_dump_regs(struct ethtool_drvinfo *info,

ethtool-util.h int sfc_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);

Quoted 6 lines

ethtool-util.h +4 -0

                       struct ethtool_regs *regs);
  int st_gmac_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
+
+/* Rx flow classification */
+#include<sys/ioctl.h>
+#include<net/if.h>

Put #includes at the top of the file, please.

Quoted 33 lines

ethtool-util.h +8 -0

+int rxclass_parse_ruleopts(char **optstr, int opt_cnt,
+                        struct ethtool_rx_flow_spec *fsp, __u8 *loc_valid);
+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, __u8 loc_valid);
+int rxclass_rule_del(int fd, struct ifreq *ifr, __u32 loc);
+
  #endif

ethtool.8.in +10 -0

index 133825b..c183a3d 100644
@@ -40,21 +40,36 @@
  [\\fB\\$1\\fP\ \\fIN\\fP]
..
  .\"
+.\"  .BM - same as above but has a mask field for format "[value N [m N]]"
+.\"
+.de BM
+[\\fB\\$1\\fP\ \\fIN\\fP\ [\\fBm\\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
  .\"

Quoted 6 lines

.ds WO \fBp\fP|\fBu\fP|\fBm\fP|\fBb\fP|\fBa\fP|\fBg\fP|\fBs\fP|\fBd\fP... .\"
.\" \(*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

This adds two '|' characters between 'ah4' and 'esp4'.

Quoted 14 lines

.\"
.\" \(*HO - hash options
.\"
.ds HO \fBm\fP|\fBv\fP|\fBt\fP|\fBs\fP|\fBd\fP|\fBf\fP|\fBn\fP|\fBr\fP...
+.\"
+.\" \(*L4 - L4 proto options
+.\"
+.ds L4 \fBtcp\fP|\fBudp\fP|\fBsctp\fP|\fBah\fP|\fBesp\fP|\fIN\fP +.\"
.\" Start URL.
.de UR
. ds m1 \\$1\"

ethtool.8.in +0 -0

@@ -224,11 +239,27 @@ ethtool \- query or control network driver and hardware settings
  .B ethtool \-n

ethtool.8.in ethtool \- query or control network driver and hardware settings · line 245

Quoted 6 lines

ethtool.8.in +4 -0

  .I ethX
  .RB [ rx-flow-hash \ \*(FL]
+.RB [ rx-rings ]
+.RB [ rx-class-rule-all ]
+.RB [ rx-class-rule
+.IR N ]

Should use '.BN'.

[...]

Quoted 20 lines

ethtool.c +2 -0

index 1afdfe4..b624980 100644
@@ -6,6 +6,7 @@
   * Kernel 2.4 update Copyright 2001 Jeff Garzik<[email protected]>
   * Wake-on-LAN,natsemi,misc support by Tim Hockin<[email protected]>
   * Portions Copyright 2002 Intel
+ * Portions Copyright (C) Sun Microsystems 2008
   * do_test support by Eli Kupermann<[email protected]>
   * ETHTOOL_PHYS_ID support by Chris Leech<[email protected]>
   * e1000 support by Scott Feldman<[email protected]>

@@ -14,6 +15,7 @@
   * amd8111e support by Reeja John<[email protected]>
   * long arguments by Andi Kleen.
   * SMSC LAN911x support by Steve Glendinning<[email protected]>
+ * Rx Network Flow Control configuration support<[email protected]>
   * Various features by Ben Hutchings<[email protected]>;
   *   Copyright 2009, 2010 Solarflare Communications
   *

ethtool.c line 37

Quoted 6 lines

ethtool.c +1 -1

@@ -32,7 +34,7 @@
  #include<sys/ioctl.h>
  #include<sys/stat.h>
  #include<stdio.h>
-#include<string.h>
+#include<strings.h>

No,<string.h> is standard.

[...]

Quoted 15 lines

ethtool.c +8 -0

@@ -408,6 +425,14 @@ static int msglvl_changed;
  static u32 msglvl_wanted = 0;
  static u32 msglvl_mask = 0;
 
+static int rx_rings_get = 0;
+static int rx_class_rule_get = -1;
+static int rx_class_rule_getall = 0;
+static int rx_class_rule_del = -1;
+static int rx_class_rule_added = 0;
+static struct ethtool_rx_flow_spec rx_rule_fs;
+static u8 rxclass_loc_valid = 0;
+
  static enum {
       ONLINE=0,
       OFFLINE,

[...]

ethtool.c static void parse_cmdline(int argc, char **argp) · line 979

Quoted 7 lines

ethtool.c +3 -0

@@ -945,6 +974,23 @@ static void parse_cmdline(int argc, char **argp)
                                               rxflow_str_to_type(argp[i]);
                                       if (!rx_fhash_get)
                                               show_usage(1);
+                             } else if (!strcmp(argp[i], "rx-rings")) {
+                                     i += 1;
+                                     rx_rings_get = 1;

I'm not convinced of the value of a separate rx-rings option/keyword. However it's probably worth displaying the number of rings/queues when showing other flow hashing and steering/filtering information (the -x option does this).

Quoted 4 lines

ethtool.c +4 -0

+                             } else if (!strcmp(argp[i],
+                                                "rx-class-rule-all")) {
+                                     i += 1;
+                                     rx_class_rule_getall = 1;

ethtool.c static void parse_cmdline(int argc, char **argp) · line 993

Quoted 10 lines

ethtool.c +10 -0

+                             } else if (!strcmp(argp[i], "rx-class-rule")) {
+                                     i += 1;
+                                     if (i>= argc) {
+                                             show_usage(1);
+                                             break;
+                                     }
+                                     rx_class_rule_get =
+                                             strtol(argp[i], NULL, 0);
+                                     if (rx_class_rule_get<  0)
+                                             show_usage(1);

Use get_uint_range(argp[i], 0, INT_MAX).

ethtool.c static void parse_cmdline(int argc, char **argp) · line 996

Quoted 3 lines

ethtool.c +0 -0

                               } else
                                       show_usage(1);
                               break;

I don't think the same options (-n, -N) should be used both for flow hashing and n-tuple flow steering/filtering. This command-line interface and the structure used in the ethtool API just seem to reflect the implementation in the niu driver.

(In fact I would much prefer it if the -u and -U options could be used for both the rxnfc and rxntuple interfaces. But I haven't thought about how the differences in functionality would be exposed to or hidden from the user.)

Quoted 10 lines

ethtool.c +5 -1

@@ -978,8 +1024,37 @@ static void parse_cmdline(int argc, char **argp)
                                               show_usage(1);
                                       else
                                               rx_fhash_changed = 1;
-                             } else
+                             } else if (!strcmp(argp[i],
+                                                "rx-class-rule-del")) {
+                                     i += 1;
+                                     if (i>= argc) {
+                                             show_usage(1);

ethtool.c static void parse_cmdline(int argc, char **argp) · line 1037

Quoted 6 lines

ethtool.c +6 -0

+                                             break;
+                                     }
+                                     rx_class_rule_del =
+                                             strtol(argp[i], NULL, 0);
+                                     if (rx_class_rule_del<  0)
+                                             show_usage(1);

Use get_uint_range(argp[i], 0, INT_MAX).

Quoted 22 lines

ethtool.c +19 -0

+                             } else if (!strcmp(argp[i],
+                                                "rx-class-rule-add")) {
+                                     i += 1;
+                                     if (i>= argc) {
+                                             show_usage(1);
+                                             break;
+                                     }
+                                     if (rxclass_parse_ruleopts(&argp[i],
+                                                                argc - i,
+&rx_rule_fs,
+&rxclass_loc_valid)<  0) {
+                                             show_usage(1);
+                                     } else {
+                                             i = argc;
+                                             rx_class_rule_added = 1;
+                                     }
+                             } else {
                                       show_usage(1);
+                             }
+
                               break;
                       }

ethtool.c static int dump_rxfhash(int fhash, u64 val) · line 1920

Quoted 6 lines

ethtool.c +0 -1

                       if (mode == MODE_SRXFHINDIR) {

@@ -1917,9 +1992,12 @@ static int dump_rxfhash(int fhash, u64 val)
       case SCTP_V4_FLOW:
               fprintf(stdout, "SCTP over IPV4 flows");
               break;
-     case AH_ESP_V4_FLOW:

I believe this is still a valid type for flow hashing.

Quoted 8 lines

ethtool.c +4 -0

+     case AH_V4_FLOW:
               fprintf(stdout, "IPSEC AH over IPV4 flows");
               break;
+     case ESP_V4_FLOW:
+             fprintf(stdout, "IPSEC ESP over IPV4 flows");
+             break;
       case TCP_V6_FLOW:
               fprintf(stdout, "TCP over IPV6 flows");

ethtool.c static int dump_rxfhash(int fhash, u64 val) · line 1932

Quoted 6 lines

ethtool.c +0 -1

               break;

@@ -1929,9 +2007,12 @@ static int dump_rxfhash(int fhash, u64 val)
       case SCTP_V6_FLOW:
               fprintf(stdout, "SCTP over IPV6 flows");
               break;
-     case AH_ESP_V6_FLOW:

Same as for AH_ESP_V4_FLOW.

Quoted 40 lines

ethtool.c +16 -3

+     case AH_V6_FLOW:
               fprintf(stdout, "IPSEC AH over IPV6 flows");
               break;
+     case ESP_V6_FLOW:
+             fprintf(stdout, "IPSEC ESP over IPV6 flows");
+             break;
       default:
               break;
       }

@@ -2911,14 +2992,12 @@ static int do_gstats(int fd, struct ifreq *ifr)
       return 0;
  }
 
-
  static int do_srxclass(int fd, struct ifreq *ifr)
  {
       int err;
+     struct ethtool_rxnfc nfccmd;
 
       if (rx_fhash_changed) {
-             struct ethtool_rxnfc nfccmd;
-
               nfccmd.cmd = ETHTOOL_SRXFH;
               nfccmd.flow_type = rx_fhash_set;
               nfccmd.data = rx_fhash_val;

@@ -2930,6 +3009,20 @@ static int do_srxclass(int fd, struct ifreq *ifr)
 
       }
 
+     if (rx_class_rule_added) {
+             err = rxclass_rule_ins(fd, ifr,&rx_rule_fs,
+                                    rxclass_loc_valid);
+             if (err<  0)
+                     fprintf(stderr, "Cannot insert RX classification rule\n");
+     }
+
+     if (rx_class_rule_del>= 0) {
+             err = rxclass_rule_del(fd, ifr, rx_class_rule_del);
+
+             if (err<  0)

ethtool.c static int do_srxclass(int fd, struct ifreq *ifr) · line 3027

Quoted 5 lines

ethtool.c +3 -0

+                     fprintf(stderr, "Cannot delete RX classification rule\n");
+     }
+
       return 0;
  }

This needs to return 1 on error (I know that's an existing bug, but don't compound it).

Quoted 25 lines

ethtool.c +21 -0

@@ -2950,6 +3043,31 @@ static int do_grxclass(int fd, struct ifreq *ifr)
                       dump_rxfhash(rx_fhash_get, nfccmd.data);
       }
 
+     if (rx_rings_get) {
+             struct ethtool_rxnfc nfccmd;
+
+             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);
+     }
+
+     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");
+     }
+
+     if (rx_class_rule_getall) {
+             err = rxclass_rule_getall(fd, ifr);

ethtool.c static int do_grxclass(int fd, struct ifreq *ifr) · line 3072

Quoted 6 lines

ethtool.c +4 -0

+             if (err<  0)
+                     fprintf(stderr, "RX classification rule retrieval failed\n");
+     }
+
       return 0;
  }

Ditto for this.

Quoted 36 lines

rxclass.c new file +30 -0

new file mode 100644
index 0000000..fd01a32
@@ -0,0 +1,809 @@
+/*
+ * 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<strings.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;

rxclass.c line 35

Quoted 5 lines

rxclass.c new file +5 -0

+static int rmgr_init_done = 0;
+
+#ifndef SIOCETHTOOL
+#define SIOCETHTOOL     0x8946
+#endif

This definition ought to be moved to ethtool-util.h rather than duplicated.

Quoted 23 lines

rxclass.c new file +23 -0

+static void rmgr_print_ipv4_rule(struct ethtool_rx_flow_spec *fsp)
+{
+     char            chan[16];
+     char            l4_proto[16];
+     __u32           sip, dip, sipm, dipm;
+
+     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);
+
+     if (fsp->ring_cookie != RX_CLS_FLOW_DISC)
+             sprintf(chan, "Rx Ring [%d]", (int)fsp->ring_cookie);
+     else
+             sprintf(chan, "Discard");
+
+     switch (fsp->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:

rxclass.c line 64

Quoted 5 lines

rxclass.c new file +5 -0

+             fprintf(stdout,
+                     "      IPv4 Rule:  ID[%d] Target[%s]\n"
+                     "      IP src addr[%d.%d.%d.%d] mask[%d.%d.%d.%d]\n"
+                     "      IP dst addr[%d.%d.%d.%d] mask[%d.%d.%d.%d]\n"
+                     "      IP TOS[0x%x] mask[0x%x]\n",

To be consistent with other ethtool output, this should use colons rather than square brackets to separate field names and values.

[...]

Quoted 86 lines

rxclass.c new file +86 -0

+int rxclass_parse_ruleopts(char **optstr, int opt_cnt,
+                        struct ethtool_rx_flow_spec *fsp,
+                        u_int8_t *loc_valid)
+{
+     int i = 0;
+
+     u_int8_t discard, ring_set;
+     u_int32_t ipsa, ipsm, ipda, ipdm, spi, spim;
+     u_int16_t sp, spm, dp, dpm;
+     u_int8_t ip_ver, proto, tos, tm;
+     struct in_addr in_addr;
+
+     if (*optstr == NULL || **optstr == '\0' || opt_cnt<  2) {
+             fprintf(stdout, "Add rule, invalid syntax\n");
+             return -1;
+     }
+
+     bzero(fsp, sizeof(struct ethtool_rx_flow_spec));
+     ipsa = ipda = ipsm = ipdm = spi = spim = 0x0;
+     sp = dp = spm = dpm = 0x0;
+     ip_ver = proto = tos = tm = 0x0;
+     discard = ring_set = 0;
+
+     if (!strcmp(optstr[i], "ip4")) {
+             ip_ver = ETH_RX_NFC_IP4;
+     } else if (!strcmp(optstr[i], "ip6")) {
+             fprintf(stdout, "IPv6 not yet implemented\n");
+             return -1;
+     } else {
+             fprintf(stdout, "Add rule, invalid syntax for IP version\n");
+             return -1;
+     }
+
+     i++;
+
+     switch (ip_ver) {
+     case ETH_RX_NFC_IP4:
+             if (!strcmp(optstr[i], "tcp"))
+                     fsp->flow_type = TCP_V4_FLOW;
+             else if (!strcmp(optstr[i], "udp"))
+                     fsp->flow_type = UDP_V4_FLOW;
+             else if (!strcmp(optstr[i], "sctp"))
+                     fsp->flow_type = SCTP_V4_FLOW;
+             else if (!strcmp(optstr[i], "ah"))
+                     fsp->flow_type = AH_V4_FLOW;
+             else if (!strcmp(optstr[i], "esp"))
+                     fsp->flow_type = ESP_V4_FLOW;
+             break;
+     default:
+             fprintf(stdout, "Add rule, Invalid IP version %d\n", ip_ver);
+                     return -1;
+     }
+
+     if (fsp->flow_type == 0) {
+             proto = (u_int8_t)strtoul(optstr[i], (char **)NULL, 0);
+             if (proto != 0) {
+                     fprintf(stdout, "Add rule, user defined proto %d\n",
+                             proto);
+                     fsp->flow_type = IP_USER_FLOW;
+                     fsp->h_u.usr_ip4_spec.proto = proto;
+                     fsp->h_u.usr_ip4_spec.ip_ver = ip_ver;
+             } else {
+                     fprintf(stdout, "Add rule, invalid IP proto %s\n",
+                             optstr[i]);
+                     return -1;
+             }
+     }
+
+     for (i = 2; i<  opt_cnt;) {
+             if (!strcmp(optstr[i], "tos")) {
+                     tos = (u_int8_t)strtoul(optstr[i+1], (char **)NULL,
+                                              0);
+                     tm = 0xff;
+                     fsp->h_u.tcp_ip4_spec.tos = tos;
+
+                     i += 2;
+                     if (opt_cnt>  (i+1)) {
+                             if (!strcmp(optstr[i], "m")) {
+                                     tm = (u_int8_t)strtoul(optstr[i+1],
+                                                            (char **)NULL,
+                                                            16);
+                                     i += 2;
+                             }
+                     }
+                     fsp->m_u.tcp_ip4_spec.tos = tm;
+             } else if (!strcmp(optstr[i], "sip")) {

[...]

These keyword names must be made consistent with those used for the -U (--config-ntuple) option.

Also, they can be parsed much more concisely using the new option types I defined a while back for struct cmdline_info.

Ben.

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

Quoted 16 lines

Quoted 15 lines

ethtool.c +8 -0

@@ -408,6 +425,14 @@ static int msglvl_changed;
  static u32 msglvl_wanted = 0;
  static u32 msglvl_mask = 0;
 
+static int rx_rings_get = 0;
+static int rx_class_rule_get = -1;
+static int rx_class_rule_getall = 0;
+static int rx_class_rule_del = -1;
+static int rx_class_rule_added = 0;
+static struct ethtool_rx_flow_spec rx_rule_fs;
+static u8 rxclass_loc_valid = 0;
+
  static enum {
       ONLINE=0,
       OFFLINE,

[...]

Quoted 12 lines

ethtool.c static void parse_cmdline(int argc, char **argp) · line 979

Quoted 7 lines

ethtool.c +3 -0

@@ -945,6 +974,23 @@ static void parse_cmdline(int argc, char **argp)
                                               rxflow_str_to_type(argp[i]);
                                       if (!rx_fhash_get)
                                               show_usage(1);
+                             } else if (!strcmp(argp[i], "rx-rings")) {
+                                     i += 1;
+                                     rx_rings_get = 1;

I'm not convinced of the value of a separate rx-rings option/keyword. However it's probably worth displaying the number of rings/queues when showing other flow hashing and steering/filtering information (the -x option does this).

My thought was that it would be useful for determining the number of rings prior to adding a rule. Especially if we have any kind of scripts running on top of ethtool so that we can avoid rules that will fail due to ring values being greater than the actual number of rings. I might try looking into adding it to the display options for the filters.

Quoted 9 lines

ethtool.c static void parse_cmdline(int argc, char **argp) · line 996

Quoted 3 lines

ethtool.c +0 -0

                               } else
                                       show_usage(1);
                               break;

I don't think the same options (-n, -N) should be used both for flow hashing and n-tuple flow steering/filtering. This command-line interface and the structure used in the ethtool API just seem to reflect the implementation in the niu driver.

Quoted 4 lines

(In fact I would much prefer it if the -u and -U options could be used for both the rxnfc and rxntuple interfaces. But I haven't thought about how the differences in functionality would be exposed to or hidden from the user.)

I was kind of thinking about merging the two interfaces too, but I was looking at it more from the perspective of moving away from ntuple more towards this newer interface. My main motivation being that the filter display option is so badly broken for ntuple that it would be easier to make ntuple a subset of the flow classifier instead of the other way around.

What would you think of using the "flow-type" keyword to indicate legacy ntuple support, and then adding something like "class-rule-add", and "class-rule-del" to add support for the network flow classifier calls?

Quoted 20 lines

Quoted 18 lines

rxclass.c new file +18 -0

+     for (i = 2; i<  opt_cnt;) {
+             if (!strcmp(optstr[i], "tos")) {
+                     tos = (u_int8_t)strtoul(optstr[i+1], (char **)NULL,
+                                              0);
+                     tm = 0xff;
+                     fsp->h_u.tcp_ip4_spec.tos = tos;
+
+                     i += 2;
+                     if (opt_cnt>  (i+1)) {
+                             if (!strcmp(optstr[i], "m")) {
+                                     tm = (u_int8_t)strtoul(optstr[i+1],
+                                                            (char **)NULL,
+                                                            16);
+                                     i += 2;
+                             }
+                     }
+                     fsp->m_u.tcp_ip4_spec.tos = tm;
+             } else if (!strcmp(optstr[i], "sip")) {

[...]

Quoted 3 lines

These keyword names must be made consistent with those used for the -U (--config-ntuple) option.

I will update the names to be consistent with the ntuple options, however I would prefer to keep the option of short-cutting the mask via the "m" value. It will not be hard to make it support both since the pattern would be to test for either "m" or "%s-mask".

On Tue, 2011-02-22 at 12:52 -0800, Alexander Duyck wrote:

Quoted 20 lines

Quoted 9 lines

Quoted 3 lines

                                } else
                                        show_usage(1);
                                break;

I don't think the same options (-n, -N) should be used both for flow hashing and n-tuple flow steering/filtering. This command-line interface and the structure used in the ethtool API just seem to reflect the implementation in the niu driver.

Quoted 4 lines

(In fact I would much prefer it if the -u and -U options could be used for both the rxnfc and rxntuple interfaces. But I haven't thought about how the differences in functionality would be exposed to or hidden from the user.)

I was kind of thinking about merging the two interfaces too, but I was looking at it more from the perspective of moving away from ntuple more towards this newer interface. My main motivation being that the filter display option is so badly broken for ntuple that it would be easier to make ntuple a subset of the flow classifier instead of the other way around.

Quoted 3 lines

What would you think of using the "flow-type" keyword to indicate legacy ntuple support, and then adding something like "class-rule-add", and "class-rule-del" to add support for the network flow classifier calls?

I really don't want to introduce different syntax for functionality that is common between the two command sets. The user should not have to know that driver A implements interface I and driver B implements interface J, except that since version 2.6.y driver A implements interface J too.

Surely it is possible to try one interface, then the other, when the requested filter can be implemented either way?

Quoted 1 line · Ben Hutchings

On Tue, 2011-02-22 at 12:52 -0800, Alexander Duyck wrote:

[...]

Quoted 30 lines

Quoted 20 lines

Quoted 9 lines

Quoted 3 lines

                                } else
                                        show_usage(1);
                                break;

I don't think the same options (-n, -N) should be used both for flow hashing and n-tuple flow steering/filtering. This command-line interface and the structure used in the ethtool API just seem to reflect the implementation in the niu driver.

Quoted 4 lines

(In fact I would much prefer it if the -u and -U options could be used for both the rxnfc and rxntuple interfaces. But I haven't thought about how the differences in functionality would be exposed to or hidden from the user.)

I was kind of thinking about merging the two interfaces too, but I was looking at it more from the perspective of moving away from ntuple more towards this newer interface. My main motivation being that the filter display option is so badly broken for ntuple that it would be easier to make ntuple a subset of the flow classifier instead of the other way around.

Quoted 3 lines

What would you think of using the "flow-type" keyword to indicate legacy ntuple support, and then adding something like "class-rule-add", and "class-rule-del" to add support for the network flow classifier calls?

I really don't want to introduce different syntax for functionality that is common between the two command sets. The user should not have to know that driver A implements interface I and driver B implements interface J, except that since version 2.6.y driver A implements interface J too.

Quoted 2 lines

Surely it is possible to try one interface, then the other, when the requested filter can be implemented either way?

The problem is that the interfaces are different in the way they implement their masks. N-tuple defines the mask as 0s mean inclusion, 1s, mean exclusion.

The network flow classifier filters are the exact opposite. As such we really need to know which type of filter we are dealing with before we start setting up values.

In addition there are
options such as location which exist in network flow classifier rules, but not in ntuple rules.

Quoted 38 lines · Alexander Duyck

Quoted 1 line · Ben Hutchings

On Tue, 2011-02-22 at 12:52 -0800, Alexander Duyck wrote:

[...]

Quoted 32 lines

Quoted 20 lines

Quoted 9 lines

Quoted 3 lines

                                 } else
                                         show_usage(1);
                                 break;

I don't think the same options (-n, -N) should be used both for flow hashing and n-tuple flow steering/filtering. This command-line interface and the structure used in the ethtool API just seem to reflect the implementation in the niu driver.

Quoted 4 lines

(In fact I would much prefer it if the -u and -U options could be used for both the rxnfc and rxntuple interfaces. But I haven't thought about how the differences in functionality would be exposed to or hidden from the user.)

I was kind of thinking about merging the two interfaces too, but I was looking at it more from the perspective of moving away from ntuple more towards this newer interface. My main motivation being that the filter display option is so badly broken for ntuple that it would be easier to make ntuple a subset of the flow classifier instead of the other way around.

Quoted 3 lines

What would you think of using the "flow-type" keyword to indicate legacy ntuple support, and then adding something like "class-rule-add", and "class-rule-del" to add support for the network flow classifier calls?

I really don't want to introduce different syntax for functionality that is common between the two command sets. The user should not have to know that driver A implements interface I and driver B implements interface J, except that since version 2.6.y driver A implements interface J too.

Surely it is possible to try one interface, then the other, when the requested filter can be implemented either way?

Quoted 3 lines

The problem is that the interfaces are different in the way they implement their masks. N-tuple defines the mask as 0s mean inclusion, 1s, mean exclusion.

You have got to be kidding me!

If this is the case, then the current kernel-doc for ethtool_rx_flow_spec::m_u is incorrect.

Quoted 3 lines

The network flow classifier filters are the exact opposite. As such we really need to know which type of filter we are dealing with before we start setting up values.

This is nonsense; it's not hard to flip the bits later.

Quoted 3 lines

In addition there are
options such as location which exist in network flow classifier rules, but not in ntuple rules.

Sure, and when those are specified then there can be no fallback.

Ben.

Quoted 39 lines · Ben Hutchings

Quoted 38 lines · Alexander Duyck

Quoted 1 line · Ben Hutchings

On Tue, 2011-02-22 at 12:52 -0800, Alexander Duyck wrote:

[...]

Quoted 32 lines

Quoted 20 lines

Quoted 9 lines

Quoted 3 lines

                                 } else
                                         show_usage(1);
                                 break;

I don't think the same options (-n, -N) should be used both for flow hashing and n-tuple flow steering/filtering. This command-line interface and the structure used in the ethtool API just seem to reflect the implementation in the niu driver.

Quoted 4 lines

(In fact I would much prefer it if the -u and -U options could be used for both the rxnfc and rxntuple interfaces. But I haven't thought about how the differences in functionality would be exposed to or hidden from the user.)

I was kind of thinking about merging the two interfaces too, but I was looking at it more from the perspective of moving away from ntuple more towards this newer interface. My main motivation being that the filter display option is so badly broken for ntuple that it would be easier to make ntuple a subset of the flow classifier instead of the other way around.

Quoted 3 lines

What would you think of using the "flow-type" keyword to indicate legacy ntuple support, and then adding something like "class-rule-add", and "class-rule-del" to add support for the network flow classifier calls?

I really don't want to introduce different syntax for functionality that is common between the two command sets. The user should not have to know that driver A implements interface I and driver B implements interface J, except that since version 2.6.y driver A implements interface J too.

Surely it is possible to try one interface, then the other, when the requested filter can be implemented either way?

Quoted 8 lines

Quoted 3 lines

The problem is that the interfaces are different in the way they implement their masks. N-tuple defines the mask as 0s mean inclusion, 1s, mean exclusion.

You have got to be kidding me!

If this is the case, then the current kernel-doc for ethtool_rx_flow_spec::m_u is incorrect.

That would be the case. The m_u for ethtool_rx_flow_spec is 0 for bits to be ignored. It is one of the things I really liked about that since in combination with the way the original patch generated the masks it would mean no goofy bit setting workarounds.

I think the documentation was added after the ethtool_rx_flow_spec and ethtool_rx_ntuple_flow_spec were and it looks like whoever added it probably assumed it worked the same way as ntuple. I can probably submit an updated patch to correct the kernel-doc for that.

Actually now that I am thinking about it I could probably just ignore location for rules that end up being processed via ntuple.

The only time where location really matters is if you are attempting to overwrite an existing rule and I am not sure how that would be handled in ntuple anyway since right now adding additional rules via ntuple for ixgbe just results in duplicate rules being defined.

The idea I have for this now is to just record everything using the ethtool_rx_flow_spec. With it extended to support VLAN and 64 bytes of data we should be able to just store everything in the one structure, try recording it to the hardware via the nfc interface, if that fails then copy the values except for location into a ntuple structure, and attempt to write it via the ntuple interface.

Quoted 1 line · Alexander Duyck

The only time where location really matters is if you are attempting to overwrite an existing rule and I am not sure how that would be handled in ntuple anyway since right now adding additional rules via ntuple for ixgbe just results in duplicate rules being defined.

As I understand it, the location also determines the *priority* for the rule.

Which is why I wrote that "@fs.@location specifies the index to use and must not be ignored."

To support hardware where the filter table is hash-based rather than a TCAM, we would need some kind of flag or special value of location that means 'wherever'.

Quoted 6 lines · Ben Hutchings

Quoted 4 lines · Alexander Duyck

The only time where location really matters is if you are attempting to overwrite an existing rule and I am not sure how that would be handled in ntuple anyway since right now adding additional rules via ntuple for ixgbe just results in duplicate rules being defined.

Quoted 2 lines

As I understand it, the location also determines the *priority* for the rule.

This is true, at least for TCAMs. But it's relevant only when multiple filters would match a packet. People often use non-overlapping filters, for these adding the filter at any available slot is OK.

Quoted 6 lines

Which is why I wrote that "@fs.@location specifies the index to use and must not be ignored."

To support hardware where the filter table is hash-based rather than a TCAM, we would need some kind of flag or special value of location that means 'wherever'.

I'd find the 'wherever' option useful for TCAMs too. Maybe even have a few of those, like 'first available', 'any', and 'last available'. The last one is quite useful for catch-all rules without requiring one to know the TCAM size.

Quoted 7 lines · Dimitris Michailidis

Quoted 6 lines · Ben Hutchings

Quoted 4 lines · Alexander Duyck

The only time where location really matters is if you are attempting to overwrite an existing rule and I am not sure how that would be handled in ntuple anyway since right now adding additional rules via ntuple for ixgbe just results in duplicate rules being defined.

Quoted 6 lines

Quoted 2 lines

As I understand it, the location also determines the *priority* for the rule.

This is true, at least for TCAMs. But it's relevant only when multiple filters would match a packet. People often use non-overlapping filters, for these adding the filter at any available slot is OK.

Right. But ethtool would have to determine that the filter was non- overlapping, before ignoring the location. Also it cannot allow deletion by location if it ever ignores the location on insertion. We should make the location optional at both the command-line and API level, but never ignore it.

Quoted 7 lines

Quoted 6 lines

Which is why I wrote that "@fs.@location specifies the index to use and must not be ignored."

To support hardware where the filter table is hash-based rather than a TCAM, we would need some kind of flag or special value of location that means 'wherever'.

Quoted 3 lines

I'd find the 'wherever' option useful for TCAMs too. Maybe even have a few of those, like 'first available', 'any', and 'last available'. The last one is quite useful for catch-all rules without requiring one to know the TCAM size.

Agreed.

Ben.

Quoted 15 lines · Ben Hutchings

Quoted 7 lines · Dimitris Michailidis

Quoted 6 lines · Ben Hutchings

Quoted 4 lines · Alexander Duyck

The only time where location really matters is if you are attempting to overwrite an existing rule and I am not sure how that would be handled in ntuple anyway since right now adding additional rules via ntuple for ixgbe just results in duplicate rules being defined.

Quoted 6 lines

Quoted 2 lines

As I understand it, the location also determines the *priority* for the rule.

This is true, at least for TCAMs. But it's relevant only when multiple filters would match a packet. People often use non-overlapping filters, for these adding the filter at any available slot is OK.

Quoted 6 lines

Right. But ethtool would have to determine that the filter was non- overlapping, before ignoring the location. Also it cannot allow deletion by location if it ever ignores the location on insertion. We should make the location optional at both the command-line and API level, but never ignore it.

I wasn't implying that we ignore it for rules inserted via the nfc interface. Only for those inserted via the ntuple interface.

My reasoning for that was because it had occurred to me that what my patch series had done is allow for ntuples to be displayed via the get_rx_nfc interface. As such you would end up with a location being implied when displaying the rules since it would give you a list of n entities.

If you attempted to restore the rules you would probably end up with the location information for filters 0..(n-1), and that could be dropped since it would just be extra information.

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Which is why I wrote that "@fs.@location specifies the index to use and must not be ignored."

To support hardware where the filter table is hash-based rather than a TCAM, we would need some kind of flag or special value of location that means 'wherever'.

I'd find the 'wherever' option useful for TCAMs too. Maybe even have a few of those, like 'first available', 'any', and 'last available'. The last one is quite useful for catch-all rules without requiring one to know the TCAM size.

Quoted 3 lines

Agreed.

Ben.

The first and last options make a lot of sense to me. The one I'm not sure about would be the "any" option. It seems like it would be redundant with the "first available" option or is there something I'm missing?

Also the code I have currently for the user space is just starting at 0 and filling in the rules on a first available basis for location not specified. Is this going to work for most cases or should I look at changing it to something like a "last available" approach for the nfc based filters?

Quoted 15 lines · Ben Hutchings

Quoted 7 lines · Dimitris Michailidis

Quoted 6 lines · Ben Hutchings

Quoted 4 lines · Alexander Duyck

The only time where location really matters is if you are attempting to overwrite an existing rule and I am not sure how that would be handled in ntuple anyway since right now adding additional rules via ntuple for ixgbe just results in duplicate rules being defined.

Quoted 6 lines

Quoted 2 lines

As I understand it, the location also determines the *priority* for the rule.

This is true, at least for TCAMs. But it's relevant only when multiple filters would match a packet. People often use non-overlapping filters, for these adding the filter at any available slot is OK.

Quoted 6 lines

Right. But ethtool would have to determine that the filter was non- overlapping, before ignoring the location. Also it cannot allow deletion by location if it ever ignores the location on insertion. We should make the location optional at both the command-line and API level, but never ignore it.

I wasn't implying that we ignore it for rules inserted via the nfc interface. Only for those inserted via the ntuple interface.

My reasoning for that was because it had occurred to me that what my patch series had done is allow for ntuples to be displayed via the get_rx_nfc interface. As such you would end up with a location being implied when displaying the rules since it would give you a list of n entities.

If you attempted to restore the rules you would probably end up with the location information for filters 0..(n-1), and that could be dropped since it would just be extra information.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Which is why I wrote that "@fs.@location specifies the index to use and must not be ignored."

To support hardware where the filter table is hash-based rather than a TCAM, we would need some kind of flag or special value of location that means 'wherever'.

Quoted 7 lines

Quoted 3 lines

I'd find the 'wherever' option useful for TCAMs too. Maybe even have a few of those, like 'first available', 'any', and 'last available'. The last one is quite useful for catch-all rules without requiring one to know the TCAM size.

Agreed.

Ben.

The first and last options make a lot of sense to me. The one I'm not sure about would be the "any" option. It seems like it would be redundant with the "first available" option or is there something I'm missing?

Also the code I have currently for the user space is just starting at 0 and filling in the rules on a first available basis for location not specified. Is this going to work for most cases or should I look at changing it to something like a "last available" approach for the nfc based filters?

Quoted 13 lines · Alexander Duyck

Quoted 12 lines · Ben Hutchings

Quoted 11 lines · Dimitris Michailidis

Quoted 7 lines · Ben Hutchings

Quoted 4 lines · Alexander Duyck

The only time where location really matters is if you are attempting to overwrite an existing rule and I am not sure how that would be handled in ntuple anyway since right now adding additional rules via ntuple for ixgbe just results in duplicate rules being defined.

As I understand it, the location also determines the *priority* for the rule.

This is true, at least for TCAMs. But it's relevant only when multiple filters would match a packet. People often use non-overlapping filters, for these adding the filter at any available slot is OK.

Quoted 7 lines

Quoted 6 lines

Right. But ethtool would have to determine that the filter was non- overlapping, before ignoring the location. Also it cannot allow deletion by location if it ever ignores the location on insertion. We should make the location optional at both the command-line and API level, but never ignore it.

I wasn't implying that we ignore it for rules inserted via the nfc interface. Only for those inserted via the ntuple interface.

We should never fall back to the ntuple interface if a location is specified!

Quoted 1 line

My reasoning for that was because it had occurred to me that what my patch series had done is allow for ntuples to be displayed via the get_rx_nfc interface. As such you would end up with a location being implied when displaying the rules since it would give you a list of n entities.

We need to sort that out then.

Quoted 16 lines

If you attempted to restore the rules you would probably end up with the location information for filters 0..(n-1), and that could be dropped since it would just be extra information.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Which is why I wrote that "@fs.@location specifies the index to use and must not be ignored."

To support hardware where the filter table is hash-based rather than a TCAM, we would need some kind of flag or special value of location that means 'wherever'.

Quoted 6 lines

Quoted 3 lines

I'd find the 'wherever' option useful for TCAMs too. Maybe even have a few of those, like 'first available', 'any', and 'last available'. The last one is quite useful for catch-all rules without requiring one to know the TCAM size.

Agreed.

Ben.

The first and last options make a lot of sense to me. The one I'm not sure about would be the "any" option. It seems like it would be redundant with the "first available" option or is there something I'm missing?

Quoted 1 line

Also the code I have currently for the user space is just starting at 0 and filling in the rules on a first available basis for location not specified. Is this going to work for most cases or should I look at changing it to something like a "last available" approach for the nfc based filters?

My *guess* (and this is just a guess) is that users are more likely to want to specify explicit priorities for the high-priority rules and not for the low-priority rules. So if the location is not explicitly set then we should choose the last available (lowest-priority) location in a TCAM, possibly excluding the very last location so that 'last' will still work.