[dpdk-dev] [PATCH v2 0/2] TCP flow classification using 4-tuple and flags

[dpdk-dev] [PATCH v2 0/2] TCP flow classification using 4-tuple and flags

[Sowmini Varadhan]

V2 updates: checkpatch typo fixes, bernard.iremonger review fixes

The problem space of TCP flow tracking and classification
based on TCP state requires the ability to classify TCP
flows on more packet properties than just the 4-tuple,
e.g., TCP flags. This patch-set provides the set of
changes needed in the examples/flow_classify.c needed to
achieve this.

Patch 1 extends examples/flow_classify.c to allow constraints
on tcp flags. Patch 2 extends the ACL handling in
librte_flow_classify to include keys on the properties in
addition to the tcp 4-tuple.

Sowmini Varadhan (2):
  examples/flow_classify: hooks for filters on tcp flags
  examples/flow_classify: add an ACL table for tcp

 examples/flow_classify/flow_classify.c      | 118 +++++++++++++++++---
 examples/flow_classify/ipv4_rules_file.txt  |  22 ++--
 lib/flow_classify/rte_flow_classify.c       |  87 +++++++++++++++
 lib/flow_classify/rte_flow_classify.h       |  19 ++++
 lib/flow_classify/rte_flow_classify_parse.c |   8 +-
 5 files changed, 228 insertions(+), 26 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH v2 1/2] examples/flow_classify: hooks for filters on tcp flags

[Sowmini Varadhan]

v2 fixes:  typo fixes, get_tcp_flags returns -EINVAL

The rte_eth_ntuple_filter allows tcp_flags which can check for things
like
    #define RTE_TCP_CWR_FLAG 0x80 /**< Congestion Window Reduced */
    #define RTE_TCP_ECE_FLAG 0x40 /**< ECN-Echo */
    #define RTE_TCP_URG_FLAG 0x20 /**< Urgent Pointer field significant */
    #define RTE_TCP_ACK_FLAG 0x10 /**< Acknowledgment field significant */
    #define RTE_TCP_PSH_FLAG 0x08 /**< Push Function */
    #define RTE_TCP_RST_FLAG 0x04 /**< Reset the connection */
    #define RTE_TCP_SYN_FLAG 0x02 /**< Synchronize sequence numbers */
    #define RTE_TCP_FIN_FLAG 0x01 /**< No more data from sender */
but there are no existing examples that demonstrate how to use this
feature.

This patch extends the existing classification support to allow
an optional flags in the input file. The flags string can be any
concatenation of characters from {C, E, U, A, P, R, S, F} and
"*" indicates "don't care". These flags are set in the ntuple_filter and
are used to construct the tcp_spec and tcp_mask sent to the driver

The rte_acl_field_def is updated to use the (u8) tcp flag as lookup key.
Note that, as per
  https://doc.dpdk.org/guides/prog_guide/packet_classif_access_ctrl.html
this field MUST be allocated for 4 bytes, thus it has sizeof(uint32_t).

However, also note the XXX in this commit: additional updates are
needed to the rte_flow_classify_table_entry_add() so that it does
not ignore any key fields other than the 5-tuple.

Signed-off-by: Sowmini Varadhan <sovaradh@linux.microsoft.com>
---
 examples/flow_classify/flow_classify.c     | 87 ++++++++++++++++++++--
 examples/flow_classify/ipv4_rules_file.txt | 22 +++---
 2 files changed, 91 insertions(+), 18 deletions(-)

diff --git a/examples/flow_classify/flow_classify.c b/examples/flow_classify/flow_classify.c
index db71f5aa04..ff4c8bc2f5 100644
--- a/examples/flow_classify/flow_classify.c
+++ b/examples/flow_classify/flow_classify.c
@@ -51,6 +51,7 @@ enum {
 	CB_FLD_DST_PORT_MASK,
 	CB_FLD_PROTO,
 	CB_FLD_PRIORITY,
+	CB_FLD_TCP_FLAGS,
 	CB_FLD_NUM,
 };
 
@@ -86,6 +87,7 @@ enum {
 	DST_FIELD_IPV4,
 	SRCP_FIELD_IPV4,
 	DSTP_FIELD_IPV4,
+	TCP_FLAGS_FIELD,
 	NUM_FIELDS_IPV4
 };
 
@@ -93,7 +95,8 @@ enum {
 	PROTO_INPUT_IPV4,
 	SRC_INPUT_IPV4,
 	DST_INPUT_IPV4,
-	SRCP_DESTP_INPUT_IPV4
+	SRCP_DESTP_INPUT_IPV4,
+	TCP_FLAGS_INDEX,
 };
 
 static struct rte_acl_field_def ipv4_defs[NUM_FIELDS_IPV4] = {
@@ -150,6 +153,17 @@ static struct rte_acl_field_def ipv4_defs[NUM_FIELDS_IPV4] = {
 			sizeof(struct rte_ipv4_hdr) +
 			offsetof(struct rte_tcp_hdr, dst_port),
 	},
+	/* next field must be 4 bytes, even though flags is only 1 byte */
+	{
+		/* rte_flags */
+		.type = RTE_ACL_FIELD_TYPE_BITMASK,
+		.size = sizeof(uint32_t),
+		.field_index = TCP_FLAGS_FIELD,
+		.input_index = TCP_FLAGS_INDEX,
+		.offset = sizeof(struct rte_ether_hdr) +
+			sizeof(struct rte_ipv4_hdr) +
+			offsetof(struct rte_tcp_hdr, tcp_flags),
+	},
 };
 /* >8 End of creation of ACL table. */
 
@@ -285,12 +299,14 @@ lcore_main(struct flow_classifier *cls_app)
 	int ret;
 	int i = 0;
 
-	ret = rte_flow_classify_table_entry_delete(cls_app->cls,
-			rules[7]);
-	if (ret)
-		printf("table_entry_delete failed [7] %d\n\n", ret);
-	else
-		printf("table_entry_delete succeeded [7]\n\n");
+	if (rules[7]) {
+		ret = rte_flow_classify_table_entry_delete(cls_app->cls,
+				rules[7]);
+		if (ret)
+			printf("table_entry_delete failed [7] %d\n\n", ret);
+		else
+			printf("table_entry_delete succeeded [7]\n\n");
+	}
 
 	/*
 	 * Check that the port is on the same NUMA node as the polling thread
@@ -410,6 +426,53 @@ parse_ipv4_net(char *in, uint32_t *addr, uint32_t *mask_len)
 	return 0;
 }
 
+static int
+get_tcp_flags(char *in, struct rte_eth_ntuple_filter *ntuple_filter)
+{
+	int len = strlen(in);
+	int i;
+	uint8_t flags = 0;
+
+	if (strcmp(in, "*") == 0) {
+		ntuple_filter->tcp_flags = 0;
+		return 0;
+	}
+
+	for (i = 0; i < len; i++) {
+		switch (in[i]) {
+		case 'S':
+			flags |= RTE_TCP_SYN_FLAG;
+			break;
+		case 'F':
+			flags |= RTE_TCP_FIN_FLAG;
+			break;
+		case 'R':
+			flags |= RTE_TCP_RST_FLAG;
+			break;
+		case 'P':
+			flags |= RTE_TCP_PSH_FLAG;
+			break;
+		case 'A':
+			flags |= RTE_TCP_ACK_FLAG;
+			break;
+		case 'U':
+			flags |= RTE_TCP_URG_FLAG;
+			break;
+		case 'E':
+			flags |= RTE_TCP_ECE_FLAG;
+			break;
+		case 'C':
+			flags |= RTE_TCP_CWR_FLAG;
+			break;
+		default:
+			fprintf(stderr, "unknown flag %c\n", in[i]);
+			return -EINVAL;
+		}
+	}
+	ntuple_filter->tcp_flags = flags;
+	return 0;
+}
+
 static int
 parse_ipv4_5tuple_rule(char *str, struct rte_eth_ntuple_filter *ntuple_filter)
 {
@@ -481,6 +544,8 @@ parse_ipv4_5tuple_rule(char *str, struct rte_eth_ntuple_filter *ntuple_filter)
 	ntuple_filter->priority = (uint16_t)temp;
 	if (ntuple_filter->priority > FLOW_CLASSIFY_MAX_PRIORITY)
 		ret = -EINVAL;
+	if (get_tcp_flags(in[CB_FLD_TCP_FLAGS], ntuple_filter))
+		return -EINVAL;
 
 	return ret;
 }
@@ -597,10 +662,13 @@ add_classify_rule(struct rte_eth_ntuple_filter *ntuple_filter,
 		memset(&tcp_spec, 0, sizeof(tcp_spec));
 		tcp_spec.hdr.src_port = ntuple_filter->src_port;
 		tcp_spec.hdr.dst_port = ntuple_filter->dst_port;
+		tcp_spec.hdr.tcp_flags = ntuple_filter->tcp_flags;
 
 		memset(&tcp_mask, 0, sizeof(tcp_mask));
 		tcp_mask.hdr.src_port = ntuple_filter->src_port_mask;
 		tcp_mask.hdr.dst_port = ntuple_filter->dst_port_mask;
+		if (tcp_spec.hdr.tcp_flags != 0)
+			tcp_mask.hdr.tcp_flags = 0xff;
 
 		tcp_item.type = RTE_FLOW_ITEM_TYPE_TCP;
 		tcp_item.spec = &tcp_spec;
@@ -655,6 +723,11 @@ add_classify_rule(struct rte_eth_ntuple_filter *ntuple_filter,
 		return ret;
 	}
 
+	/* XXX but this only adds table_type of
+	 * RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE
+	 * i.e., it only ever does allocate_acl_ipv4_5tuple_rule()
+	 * so the tcp_flags is ignored!
+	 */
 	rule = rte_flow_classify_table_entry_add(
 			cls_app->cls, &attr, pattern_ipv4_5tuple,
 			actions, &key_found, &error);
diff --git a/examples/flow_classify/ipv4_rules_file.txt b/examples/flow_classify/ipv4_rules_file.txt
index dfa0631fcc..415573732a 100644
--- a/examples/flow_classify/ipv4_rules_file.txt
+++ b/examples/flow_classify/ipv4_rules_file.txt
@@ -1,14 +1,14 @@
 #file format:
-#src_ip/masklen dst_ip/masklen src_port : mask dst_port : mask proto/mask priority
+#src_ip/masklen dst_ip/masklen src_port : mask dst_port : mask proto/mask priority tcp_flags
 #
-2.2.2.3/24 2.2.2.7/24 32 : 0xffff 33 : 0xffff 17/0xff 0
-9.9.9.3/24 9.9.9.7/24 32 : 0xffff 33 : 0xffff 17/0xff 1
-9.9.9.3/24 9.9.9.7/24 32 : 0xffff 33 : 0xffff 6/0xff 2
-9.9.8.3/24 9.9.8.7/24 32 : 0xffff 33 : 0xffff 6/0xff 3
-6.7.8.9/24 2.3.4.5/24 32 : 0x0000 33 : 0x0000 132/0xff 4
-6.7.8.9/32 192.168.0.36/32 10 : 0xffff 11 : 0xffff 6/0xfe 5
-6.7.8.9/24 192.168.0.36/24 10 : 0xffff 11 : 0xffff 6/0xfe 6
-6.7.8.9/16 192.168.0.36/16 10 : 0xffff 11 : 0xffff 6/0xfe 7
-6.7.8.9/8 192.168.0.36/8 10 : 0xffff 11 : 0xffff 6/0xfe 8
+2.2.2.3/24 2.2.2.7/24 32 : 0xffff 33 : 0xffff 17/0xff 0 *
+9.9.9.3/24 9.9.9.7/24 32 : 0xffff 33 : 0xffff 17/0xff 1 *
+9.9.9.3/24 9.9.9.7/24 32 : 0xffff 33 : 0xffff 6/0xff 2 *
+9.9.8.3/24 9.9.8.7/24 32 : 0xffff 33 : 0xffff 6/0xff 3 *
+6.7.8.9/24 2.3.4.5/24 32 : 0x0000 33 : 0x0000 132/0xff 4 *
+6.7.8.9/32 192.168.0.36/32 10 : 0xffff 11 : 0xffff 6/0xfe 5 *
+6.7.8.9/24 192.168.0.36/24 10 : 0xffff 11 : 0xffff 6/0xfe 6 *
+6.7.8.9/16 192.168.0.36/16 10 : 0xffff 11 : 0xffff 6/0xfe 7 *
+6.7.8.9/8 192.168.0.36/8 10 : 0xffff 11 : 0xffff 6/0xfe 8 *
 #error rules
-#9.8.7.6/8 192.168.0.36/8 10 : 0xffff 11 : 0xffff 6/0xfe 9
\ No newline at end of file
+#9.8.7.6/8 192.168.0.36/8 10 : 0xffff 11 : 0xffff 6/0xfe 9 *
-- 
2.17.1

[dpdk-dev] [PATCH v2 2/2] examples/flow_classify: add an ACL table for tcp

[Sowmini Varadhan]

v2 updates: typo fixes for checkpatch,  bernard.iremonger comments

The struct rte_flow_classifier can have up to RTE_FLOW_CLASSIFY_TABLE_MAX
(32) classifier tables, but the existing flow_classify examples only adds
a single table for the L4 5-tuple.

When dealing with tcp flows, we frequently want to add ACLs and filters
to filter based on the state of the TCP connection, e.g., by looking at
the tcp flags field.

So we enhance flow_classify to add an additional acl table for
tcp 5-tuples. If the input-file-parser detects a filter for a tcp flow with
a non-wildcard argument for tcp_flags, the IP4_TCP_5TUPLE table is used
by flow_classify.

Signed-off-by: Sowmini Varadhan <sovaradh@linux.microsoft.com>
---
 examples/flow_classify/flow_classify.c      | 41 +++++++---
 lib/flow_classify/rte_flow_classify.c       | 87 +++++++++++++++++++++
 lib/flow_classify/rte_flow_classify.h       | 19 +++++
 lib/flow_classify/rte_flow_classify_parse.c |  8 +-
 4 files changed, 142 insertions(+), 13 deletions(-)

diff --git a/examples/flow_classify/flow_classify.c b/examples/flow_classify/flow_classify.c
index ff4c8bc2f5..8f6aacae03 100644
--- a/examples/flow_classify/flow_classify.c
+++ b/examples/flow_classify/flow_classify.c
@@ -723,11 +723,6 @@ add_classify_rule(struct rte_eth_ntuple_filter *ntuple_filter,
 		return ret;
 	}
 
-	/* XXX but this only adds table_type of
-	 * RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE
-	 * i.e., it only ever does allocate_acl_ipv4_5tuple_rule()
-	 * so the tcp_flags is ignored!
-	 */
 	rule = rte_flow_classify_table_entry_add(
 			cls_app->cls, &attr, pattern_ipv4_5tuple,
 			actions, &key_found, &error);
@@ -856,7 +851,8 @@ main(int argc, char *argv[])
 	int ret;
 	int socket_id;
 	struct rte_table_acl_params table_acl_params;
-	struct rte_flow_classify_table_params cls_table_params;
+	struct rte_table_acl_params table_acl_tcp_params;
+	struct rte_flow_classify_table_params cls_table_params[2];
 	struct flow_classifier *cls_app;
 	struct rte_flow_classifier_params cls_params;
 	uint32_t size;
@@ -923,21 +919,42 @@ main(int argc, char *argv[])
 	memcpy(table_acl_params.field_format, ipv4_defs, sizeof(ipv4_defs));
 
 	/* initialise table create params */
-	cls_table_params.ops = &rte_table_acl_ops;
-	cls_table_params.arg_create = &table_acl_params;
-	cls_table_params.type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
+	cls_table_params[0].ops = &rte_table_acl_ops;
+	cls_table_params[0].arg_create = &table_acl_params;
+	cls_table_params[0].type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
+
+	/* initialise ACL table params */
+	table_acl_tcp_params.name = "table_acl_ipv4_tcp_5tuple";
+	table_acl_tcp_params.n_rules = FLOW_CLASSIFY_MAX_RULE_NUM;
+	table_acl_tcp_params.n_rule_fields = RTE_DIM(ipv4_defs);
+	memcpy(table_acl_tcp_params.field_format, ipv4_defs, sizeof(ipv4_defs));
+
+	/* initialise table create params */
+	cls_table_params[1].ops = &rte_table_acl_ops;
+	cls_table_params[1].arg_create = &table_acl_tcp_params;
+	cls_table_params[1].type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE;
 
-	ret = rte_flow_classify_table_create(cls_app->cls, &cls_table_params);
+	ret = rte_flow_classify_table_create(cls_app->cls,
+					     &cls_table_params[0]);
 	if (ret) {
 		rte_flow_classifier_free(cls_app->cls);
 		rte_free(cls_app);
 		rte_exit(EXIT_FAILURE, "Failed to create classifier table\n");
 	}
+	ret = rte_flow_classify_table_create(cls_app->cls,
+					     &cls_table_params[1]);
+	if (ret) {
+		rte_flow_classifier_free(cls_app->cls);
+		rte_free(cls_app);
+		rte_exit(EXIT_FAILURE,
+			 "Failed to create classifier table\n");
+	}
+
 	/* >8 End of initialization of table create params. */
 
 	/* read file of IPv4 5 tuple rules and initialize parameters
-	 * for rte_flow_classify_validate and rte_flow_classify_table_entry_add
-	 * API's.
+	 * for rte_flow_classify_validate and
+	 * rte_flow_classify_table_entry_add  API's.
 	 */
 
 	/* Read file of IPv4 tuple rules. 8< */
diff --git a/lib/flow_classify/rte_flow_classify.c b/lib/flow_classify/rte_flow_classify.c
index d3ba2ed227..e960c3b140 100644
--- a/lib/flow_classify/rte_flow_classify.c
+++ b/lib/flow_classify/rte_flow_classify.c
@@ -60,6 +60,7 @@ enum {
 	DST_FIELD_IPV4,
 	SRCP_FIELD_IPV4,
 	DSTP_FIELD_IPV4,
+	TCP_FLAGS_FIELD,
 	NUM_FIELDS_IPV4
 };
 
@@ -72,6 +73,7 @@ struct classify_rules {
 	enum rte_flow_classify_rule_type type;
 	union {
 		struct rte_flow_classify_ipv4_5tuple ipv4_5tuple;
+		struct rte_flow_classify_ipv4_tcp_5tuple ipv4_tcp_5tuple;
 	} u;
 };
 
@@ -477,6 +479,84 @@ allocate_acl_ipv4_5tuple_rule(struct rte_flow_classifier *cls)
 	return rule;
 }
 
+static struct rte_flow_classify_rule *
+allocate_acl_ipv4_tcp_5tuple_rule(struct rte_flow_classifier *cls)
+{
+	struct rte_flow_classify_rule *rule;
+	int log_level;
+
+	rule = malloc(sizeof(struct rte_flow_classify_rule));
+	if (!rule)
+		return rule;
+
+	memset(rule, 0, sizeof(struct rte_flow_classify_rule));
+	rule->id = unique_id++;
+	rule->rules.type = RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_TCP_5TUPLE;
+
+	/* key add values */
+	rule->u.key.key_add.priority = cls->ntuple_filter.priority;
+	rule->u.key.key_add.field_value[PROTO_FIELD_IPV4].mask_range.u8 =
+			cls->ntuple_filter.proto_mask;
+	rule->u.key.key_add.field_value[PROTO_FIELD_IPV4].value.u8 =
+			cls->ntuple_filter.proto;
+	rule->rules.u.ipv4_tcp_5tuple.proto = cls->ntuple_filter.proto;
+	rule->rules.u.ipv4_tcp_5tuple.proto_mask =
+			cls->ntuple_filter.proto_mask;
+
+	rule->u.key.key_add.field_value[SRC_FIELD_IPV4].mask_range.u32 =
+			cls->ntuple_filter.src_ip_mask;
+	rule->u.key.key_add.field_value[SRC_FIELD_IPV4].value.u32 =
+			cls->ntuple_filter.src_ip;
+	rule->rules.u.ipv4_tcp_5tuple.src_ip_mask =
+			cls->ntuple_filter.src_ip_mask;
+	rule->rules.u.ipv4_tcp_5tuple.src_ip = cls->ntuple_filter.src_ip;
+
+	rule->u.key.key_add.field_value[DST_FIELD_IPV4].mask_range.u32 =
+			cls->ntuple_filter.dst_ip_mask;
+	rule->u.key.key_add.field_value[DST_FIELD_IPV4].value.u32 =
+			cls->ntuple_filter.dst_ip;
+	rule->rules.u.ipv4_tcp_5tuple.dst_ip_mask =
+			cls->ntuple_filter.dst_ip_mask;
+	rule->rules.u.ipv4_tcp_5tuple.dst_ip = cls->ntuple_filter.dst_ip;
+
+	rule->u.key.key_add.field_value[SRCP_FIELD_IPV4].mask_range.u16 =
+			cls->ntuple_filter.src_port_mask;
+	rule->u.key.key_add.field_value[SRCP_FIELD_IPV4].value.u16 =
+			cls->ntuple_filter.src_port;
+	rule->rules.u.ipv4_tcp_5tuple.src_port_mask =
+			cls->ntuple_filter.src_port_mask;
+	rule->rules.u.ipv4_tcp_5tuple.src_port = cls->ntuple_filter.src_port;
+
+	rule->u.key.key_add.field_value[DSTP_FIELD_IPV4].mask_range.u16 =
+			cls->ntuple_filter.dst_port_mask;
+	rule->u.key.key_add.field_value[DSTP_FIELD_IPV4].value.u16 =
+			cls->ntuple_filter.dst_port;
+	rule->rules.u.ipv4_tcp_5tuple.dst_port_mask =
+			cls->ntuple_filter.dst_port_mask;
+	rule->rules.u.ipv4_tcp_5tuple.dst_port = cls->ntuple_filter.dst_port;
+
+	rule->u.key.key_add.field_value[TCP_FLAGS_FIELD].mask_range.u32 =
+			rte_be_to_cpu_32(0xff);
+	rule->u.key.key_add.field_value[TCP_FLAGS_FIELD].value.u32 =
+			rte_be_to_cpu_32(cls->ntuple_filter.tcp_flags);
+	rule->rules.u.ipv4_tcp_5tuple.tcp_flags = cls->ntuple_filter.tcp_flags;
+
+	log_level = rte_log_get_level(librte_flow_classify_logtype);
+
+	if (log_level == RTE_LOG_DEBUG)
+		print_acl_ipv4_key_add(&rule->u.key.key_add);
+
+	/* key delete values */
+	memcpy(&rule->u.key.key_del.field_value[PROTO_FIELD_IPV4],
+	       &rule->u.key.key_add.field_value[PROTO_FIELD_IPV4],
+	       NUM_FIELDS_IPV4 * sizeof(struct rte_acl_field));
+
+	if (log_level == RTE_LOG_DEBUG)
+		print_acl_ipv4_key_delete(&rule->u.key.key_del);
+
+	return rule;
+}
+
 struct rte_flow_classify_rule *
 rte_flow_classify_table_entry_add(struct rte_flow_classifier *cls,
 		const struct rte_flow_attr *attr,
@@ -514,6 +594,13 @@ rte_flow_classify_table_entry_add(struct rte_flow_classifier *cls,
 		rule->tbl_type = table_type;
 		cls->table_mask |= table_type;
 		break;
+	case RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE:
+		rule = allocate_acl_ipv4_tcp_5tuple_rule(cls);
+		if (!rule)
+			return NULL;
+		rule->tbl_type = table_type;
+		cls->table_mask |= table_type;
+		break;
 	default:
 		return NULL;
 	}
diff --git a/lib/flow_classify/rte_flow_classify.h b/lib/flow_classify/rte_flow_classify.h
index 82ea92b6a6..65585d9f92 100644
--- a/lib/flow_classify/rte_flow_classify.h
+++ b/lib/flow_classify/rte_flow_classify.h
@@ -80,6 +80,8 @@ enum rte_flow_classify_rule_type {
 	RTE_FLOW_CLASSIFY_RULE_TYPE_NONE,
 	/** IPv4 5tuple type */
 	RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_5TUPLE,
+	/** IPv4 TCP 5tuple type */
+	RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_TCP_5TUPLE,
 };
 
 /** Flow classify table type */
@@ -92,6 +94,8 @@ enum rte_flow_classify_table_type {
 	RTE_FLOW_CLASSIFY_TABLE_ACL_VLAN_IP4_5TUPLE = 1 << 2,
 	/** ACL QinQ IP4 5TUPLE */
 	RTE_FLOW_CLASSIFY_TABLE_ACL_QINQ_IP4_5TUPLE = 1 << 3,
+	/** ACL IP4 5TUPLE with tcp_flags */
+	RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE = 1 << 4,
 
 };
 
@@ -131,6 +135,21 @@ struct rte_flow_classify_ipv4_5tuple {
 	uint8_t proto_mask;      /**< Mask of L4 protocol. */
 };
 
+/** IPv4 5-tuple data with tcp flags*/
+struct rte_flow_classify_ipv4_tcp_5tuple {
+	uint32_t dst_ip;         /**< Destination IP address in big endian. */
+	uint32_t dst_ip_mask;    /**< Mask of destination IP address. */
+	uint32_t src_ip;         /**< Source IP address in big endian. */
+	uint32_t src_ip_mask;    /**< Mask of destination IP address. */
+	uint16_t dst_port;       /**< Destination port in big endian. */
+	uint16_t dst_port_mask;  /**< Mask of destination port. */
+	uint16_t src_port;       /**< Source Port in big endian. */
+	uint16_t src_port_mask;  /**< Mask of source port. */
+	uint8_t proto;           /**< L4 protocol. */
+	uint8_t proto_mask;      /**< Mask of L4 protocol. */
+	uint8_t tcp_flags;       /**< Tcp only */
+};
+
 /**
  * Flow stats
  *
diff --git a/lib/flow_classify/rte_flow_classify_parse.c b/lib/flow_classify/rte_flow_classify_parse.c
index 465330291f..fe4ee05b6f 100644
--- a/lib/flow_classify/rte_flow_classify_parse.c
+++ b/lib/flow_classify/rte_flow_classify_parse.c
@@ -216,6 +216,7 @@ classify_parse_ntuple_filter(const struct rte_flow_attr *attr,
 	const struct rte_flow_action_count *count;
 	const struct rte_flow_action_mark *mark_spec;
 	uint32_t index;
+	bool have_tcp = false;
 
 	/* parse pattern */
 	index = 0;
@@ -375,6 +376,8 @@ classify_parse_ntuple_filter(const struct rte_flow_attr *attr,
 		filter->dst_port  = tcp_spec->hdr.dst_port;
 		filter->src_port  = tcp_spec->hdr.src_port;
 		filter->tcp_flags = tcp_spec->hdr.tcp_flags;
+		if (filter->tcp_flags != 0)
+			have_tcp = true;
 	} else if (item->type == RTE_FLOW_ITEM_TYPE_UDP) {
 		udp_mask = item->mask;
 
@@ -434,7 +437,10 @@ classify_parse_ntuple_filter(const struct rte_flow_attr *attr,
 		return -EINVAL;
 	}
 
-	table_type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
+	if (have_tcp)
+		table_type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE;
+	else
+		table_type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
 
 	/* parse attr */
 	/* must be input direction */
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2 2/2] examples/flow_classify: add an ACL table for tcp

[Iremonger, Bernard]

Hi Sowmini,

> -----Original Message-----
> From: Sowmini Varadhan <sovaradh@linux.microsoft.com>
> Sent: Wednesday, August 18, 2021 4:01 PM
> To: sowmini05@gmail.com; Iremonger, Bernard
> <bernard.iremonger@intel.com>; dev@dpdk.org;
> sovaradh@linux.microsoft.com
> Cc: thomas@monjalon.net
> Subject: [PATCH v2 2/2] examples/flow_classify: add an ACL table for tcp
> 
> v2 updates: typo fixes for checkpatch,  bernard.iremonger comments

The above line should not be added to the commit message.
~/dpdk/devtools/check-git-log.sh -1
Wrong tag:
        v2 fixes:  typo fixes, get_tcp_flags returns -EINVAL

The check-git-log.sh  script is in your dpdk checkout, best to run this script before sending patches

> 
> The struct rte_flow_classifier can have up to
> RTE_FLOW_CLASSIFY_TABLE_MAX
> (32) classifier tables, but the existing flow_classify examples only adds a
> single table for the L4 5-tuple.
> 
> When dealing with tcp flows, we frequently want to add ACLs and filters to
> filter based on the state of the TCP connection, e.g., by looking at the tcp
> flags field.
> 
> So we enhance flow_classify to add an additional acl table for tcp 5-tuples. If
> the input-file-parser detects a filter for a tcp flow with a non-wildcard
> argument for tcp_flags, the IP4_TCP_5TUPLE table is used by flow_classify.
> 
> Signed-off-by: Sowmini Varadhan <sovaradh@linux.microsoft.com>
> ---

If you want comment on the patch updates (optional), it can be added here after the --- line

>  examples/flow_classify/flow_classify.c      | 41 +++++++---
>  lib/flow_classify/rte_flow_classify.c       | 87 +++++++++++++++++++++
>  lib/flow_classify/rte_flow_classify.h       | 19 +++++
>  lib/flow_classify/rte_flow_classify_parse.c |  8 +-
>  4 files changed, 142 insertions(+), 13 deletions(-)

This patch contains changes to the flow_classify example and the flow_classify  library.
It needs to be split in two, a patch for the flow_classify example and a patch for the flow classify library.

> 
> diff --git a/examples/flow_classify/flow_classify.c
> b/examples/flow_classify/flow_classify.c
> index ff4c8bc2f5..8f6aacae03 100644
> --- a/examples/flow_classify/flow_classify.c
> +++ b/examples/flow_classify/flow_classify.c
> @@ -723,11 +723,6 @@ add_classify_rule(struct rte_eth_ntuple_filter
> *ntuple_filter,
>  		return ret;
>  	}
> 
> -	/* XXX but this only adds table_type of
> -	 * RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE
> -	 * i.e., it only ever does allocate_acl_ipv4_5tuple_rule()
> -	 * so the tcp_flags is ignored!
> -	 */
>  	rule = rte_flow_classify_table_entry_add(
>  			cls_app->cls, &attr, pattern_ipv4_5tuple,
>  			actions, &key_found, &error);
> @@ -856,7 +851,8 @@ main(int argc, char *argv[])
>  	int ret;
>  	int socket_id;
>  	struct rte_table_acl_params table_acl_params;
> -	struct rte_flow_classify_table_params cls_table_params;
> +	struct rte_table_acl_params table_acl_tcp_params;
> +	struct rte_flow_classify_table_params cls_table_params[2];
>  	struct flow_classifier *cls_app;
>  	struct rte_flow_classifier_params cls_params;
>  	uint32_t size;
> @@ -923,21 +919,42 @@ main(int argc, char *argv[])
>  	memcpy(table_acl_params.field_format, ipv4_defs,
> sizeof(ipv4_defs));
> 
>  	/* initialise table create params */
> -	cls_table_params.ops = &rte_table_acl_ops;
> -	cls_table_params.arg_create = &table_acl_params;
> -	cls_table_params.type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
> +	cls_table_params[0].ops = &rte_table_acl_ops;
> +	cls_table_params[0].arg_create = &table_acl_params;
> +	cls_table_params[0].type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
> +
> +	/* initialise ACL table params */
> +	table_acl_tcp_params.name = "table_acl_ipv4_tcp_5tuple";
> +	table_acl_tcp_params.n_rules = FLOW_CLASSIFY_MAX_RULE_NUM;
> +	table_acl_tcp_params.n_rule_fields = RTE_DIM(ipv4_defs);
> +	memcpy(table_acl_tcp_params.field_format, ipv4_defs,
> +sizeof(ipv4_defs));
> +
> +	/* initialise table create params */
> +	cls_table_params[1].ops = &rte_table_acl_ops;
> +	cls_table_params[1].arg_create = &table_acl_tcp_params;
> +	cls_table_params[1].type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE;
> 
> -	ret = rte_flow_classify_table_create(cls_app->cls,
> &cls_table_params);
> +	ret = rte_flow_classify_table_create(cls_app->cls,
> +					     &cls_table_params[0]);
>  	if (ret) {
>  		rte_flow_classifier_free(cls_app->cls);
>  		rte_free(cls_app);
>  		rte_exit(EXIT_FAILURE, "Failed to create classifier table\n");
>  	}
> +	ret = rte_flow_classify_table_create(cls_app->cls,
> +					     &cls_table_params[1]);
> +	if (ret) {
> +		rte_flow_classifier_free(cls_app->cls);
> +		rte_free(cls_app);
> +		rte_exit(EXIT_FAILURE,
> +			 "Failed to create classifier table\n");
> +	}
> +
>  	/* >8 End of initialization of table create params. */
> 
>  	/* read file of IPv4 5 tuple rules and initialize parameters
> -	 * for rte_flow_classify_validate and
> rte_flow_classify_table_entry_add
> -	 * API's.
> +	 * for rte_flow_classify_validate and
> +	 * rte_flow_classify_table_entry_add  API's.
>  	 */
> 
>  	/* Read file of IPv4 tuple rules. 8< */ diff --git
> a/lib/flow_classify/rte_flow_classify.c b/lib/flow_classify/rte_flow_classify.c
> index d3ba2ed227..e960c3b140 100644
> --- a/lib/flow_classify/rte_flow_classify.c
> +++ b/lib/flow_classify/rte_flow_classify.c
> @@ -60,6 +60,7 @@ enum {
>  	DST_FIELD_IPV4,
>  	SRCP_FIELD_IPV4,
>  	DSTP_FIELD_IPV4,
> +	TCP_FLAGS_FIELD,

I think it would be better to rename TCP_FLAGS_FIELD to FLAGS_FIELD_TCP in line with the other values.

>  	NUM_FIELDS_IPV4
>  };
> 
> @@ -72,6 +73,7 @@ struct classify_rules {
>  	enum rte_flow_classify_rule_type type;
>  	union {
>  		struct rte_flow_classify_ipv4_5tuple ipv4_5tuple;
> +		struct rte_flow_classify_ipv4_tcp_5tuple ipv4_tcp_5tuple;
>  	} u;
>  };
> 
> @@ -477,6 +479,84 @@ allocate_acl_ipv4_5tuple_rule(struct
> rte_flow_classifier *cls)
>  	return rule;
>  }
> 
> +static struct rte_flow_classify_rule *
> +allocate_acl_ipv4_tcp_5tuple_rule(struct rte_flow_classifier *cls) {
> +	struct rte_flow_classify_rule *rule;
> +	int log_level;
> +
> +	rule = malloc(sizeof(struct rte_flow_classify_rule));
> +	if (!rule)
> +		return rule;
> +
> +	memset(rule, 0, sizeof(struct rte_flow_classify_rule));
> +	rule->id = unique_id++;
> +	rule->rules.type =
> RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_TCP_5TUPLE;
> +
> +	/* key add values */
> +	rule->u.key.key_add.priority = cls->ntuple_filter.priority;
> +	rule-
> >u.key.key_add.field_value[PROTO_FIELD_IPV4].mask_range.u8 =
> +			cls->ntuple_filter.proto_mask;
> +	rule->u.key.key_add.field_value[PROTO_FIELD_IPV4].value.u8 =
> +			cls->ntuple_filter.proto;
> +	rule->rules.u.ipv4_tcp_5tuple.proto = cls->ntuple_filter.proto;
> +	rule->rules.u.ipv4_tcp_5tuple.proto_mask =
> +			cls->ntuple_filter.proto_mask;
> +
> +	rule->u.key.key_add.field_value[SRC_FIELD_IPV4].mask_range.u32
> =
> +			cls->ntuple_filter.src_ip_mask;
> +	rule->u.key.key_add.field_value[SRC_FIELD_IPV4].value.u32 =
> +			cls->ntuple_filter.src_ip;
> +	rule->rules.u.ipv4_tcp_5tuple.src_ip_mask =
> +			cls->ntuple_filter.src_ip_mask;
> +	rule->rules.u.ipv4_tcp_5tuple.src_ip = cls->ntuple_filter.src_ip;
> +
> +	rule->u.key.key_add.field_value[DST_FIELD_IPV4].mask_range.u32
> =
> +			cls->ntuple_filter.dst_ip_mask;
> +	rule->u.key.key_add.field_value[DST_FIELD_IPV4].value.u32 =
> +			cls->ntuple_filter.dst_ip;
> +	rule->rules.u.ipv4_tcp_5tuple.dst_ip_mask =
> +			cls->ntuple_filter.dst_ip_mask;
> +	rule->rules.u.ipv4_tcp_5tuple.dst_ip = cls->ntuple_filter.dst_ip;
> +
> +	rule-
> >u.key.key_add.field_value[SRCP_FIELD_IPV4].mask_range.u16 =
> +			cls->ntuple_filter.src_port_mask;
> +	rule->u.key.key_add.field_value[SRCP_FIELD_IPV4].value.u16 =
> +			cls->ntuple_filter.src_port;
> +	rule->rules.u.ipv4_tcp_5tuple.src_port_mask =
> +			cls->ntuple_filter.src_port_mask;
> +	rule->rules.u.ipv4_tcp_5tuple.src_port = cls->ntuple_filter.src_port;
> +
> +	rule-
> >u.key.key_add.field_value[DSTP_FIELD_IPV4].mask_range.u16 =
> +			cls->ntuple_filter.dst_port_mask;
> +	rule->u.key.key_add.field_value[DSTP_FIELD_IPV4].value.u16 =
> +			cls->ntuple_filter.dst_port;
> +	rule->rules.u.ipv4_tcp_5tuple.dst_port_mask =
> +			cls->ntuple_filter.dst_port_mask;
> +	rule->rules.u.ipv4_tcp_5tuple.dst_port = cls->ntuple_filter.dst_port;
> +
> +	rule-
> >u.key.key_add.field_value[TCP_FLAGS_FIELD].mask_range.u32 =
> +			rte_be_to_cpu_32(0xff);
> +	rule->u.key.key_add.field_value[TCP_FLAGS_FIELD].value.u32 =
> +			rte_be_to_cpu_32(cls->ntuple_filter.tcp_flags);
> +	rule->rules.u.ipv4_tcp_5tuple.tcp_flags =
> +cls->ntuple_filter.tcp_flags;
> +
> +	log_level = rte_log_get_level(librte_flow_classify_logtype);
> +
> +	if (log_level == RTE_LOG_DEBUG)
> +		print_acl_ipv4_key_add(&rule->u.key.key_add);
> +
> +	/* key delete values */
> +	memcpy(&rule->u.key.key_del.field_value[PROTO_FIELD_IPV4],
> +	       &rule->u.key.key_add.field_value[PROTO_FIELD_IPV4],
> +	       NUM_FIELDS_IPV4 * sizeof(struct rte_acl_field));
> +
> +	if (log_level == RTE_LOG_DEBUG)
> +		print_acl_ipv4_key_delete(&rule->u.key.key_del);
> +
> +	return rule;
> +}
> +
>  struct rte_flow_classify_rule *
>  rte_flow_classify_table_entry_add(struct rte_flow_classifier *cls,
>  		const struct rte_flow_attr *attr,
> @@ -514,6 +594,13 @@ rte_flow_classify_table_entry_add(struct
> rte_flow_classifier *cls,
>  		rule->tbl_type = table_type;
>  		cls->table_mask |= table_type;
>  		break;
> +	case RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE:
> +		rule = allocate_acl_ipv4_tcp_5tuple_rule(cls);
> +		if (!rule)
> +			return NULL;
> +		rule->tbl_type = table_type;
> +		cls->table_mask |= table_type;
> +		break;
>  	default:
>  		return NULL;
>  	}
> diff --git a/lib/flow_classify/rte_flow_classify.h
> b/lib/flow_classify/rte_flow_classify.h
> index 82ea92b6a6..65585d9f92 100644
> --- a/lib/flow_classify/rte_flow_classify.h
> +++ b/lib/flow_classify/rte_flow_classify.h
> @@ -80,6 +80,8 @@ enum rte_flow_classify_rule_type {
>  	RTE_FLOW_CLASSIFY_RULE_TYPE_NONE,
>  	/** IPv4 5tuple type */
>  	RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_5TUPLE,
> +	/** IPv4 TCP 5tuple type */
> +	RTE_FLOW_CLASSIFY_RULE_TYPE_IPV4_TCP_5TUPLE,
>  };
> 
>  /** Flow classify table type */
> @@ -92,6 +94,8 @@ enum rte_flow_classify_table_type {
>  	RTE_FLOW_CLASSIFY_TABLE_ACL_VLAN_IP4_5TUPLE = 1 << 2,
>  	/** ACL QinQ IP4 5TUPLE */
>  	RTE_FLOW_CLASSIFY_TABLE_ACL_QINQ_IP4_5TUPLE = 1 << 3,
> +	/** ACL IP4 5TUPLE with tcp_flags */
> +	RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE = 1 << 4,
> 
>  };
> 
> @@ -131,6 +135,21 @@ struct rte_flow_classify_ipv4_5tuple {
>  	uint8_t proto_mask;      /**< Mask of L4 protocol. */
>  };
> 
> +/** IPv4 5-tuple data with tcp flags*/
> +struct rte_flow_classify_ipv4_tcp_5tuple {
> +	uint32_t dst_ip;         /**< Destination IP address in big endian. */
> +	uint32_t dst_ip_mask;    /**< Mask of destination IP address. */
> +	uint32_t src_ip;         /**< Source IP address in big endian. */
> +	uint32_t src_ip_mask;    /**< Mask of destination IP address. */
> +	uint16_t dst_port;       /**< Destination port in big endian. */
> +	uint16_t dst_port_mask;  /**< Mask of destination port. */
> +	uint16_t src_port;       /**< Source Port in big endian. */
> +	uint16_t src_port_mask;  /**< Mask of source port. */
> +	uint8_t proto;           /**< L4 protocol. */
> +	uint8_t proto_mask;      /**< Mask of L4 protocol. */
> +	uint8_t tcp_flags;       /**< Tcp only */
> +};
> +
>  /**
>   * Flow stats
>   *
> diff --git a/lib/flow_classify/rte_flow_classify_parse.c
> b/lib/flow_classify/rte_flow_classify_parse.c
> index 465330291f..fe4ee05b6f 100644
> --- a/lib/flow_classify/rte_flow_classify_parse.c
> +++ b/lib/flow_classify/rte_flow_classify_parse.c
> @@ -216,6 +216,7 @@ classify_parse_ntuple_filter(const struct
> rte_flow_attr *attr,
>  	const struct rte_flow_action_count *count;
>  	const struct rte_flow_action_mark *mark_spec;
>  	uint32_t index;
> +	bool have_tcp = false;
> 
>  	/* parse pattern */
>  	index = 0;
> @@ -375,6 +376,8 @@ classify_parse_ntuple_filter(const struct
> rte_flow_attr *attr,
>  		filter->dst_port  = tcp_spec->hdr.dst_port;
>  		filter->src_port  = tcp_spec->hdr.src_port;
>  		filter->tcp_flags = tcp_spec->hdr.tcp_flags;
> +		if (filter->tcp_flags != 0)
> +			have_tcp = true;
>  	} else if (item->type == RTE_FLOW_ITEM_TYPE_UDP) {
>  		udp_mask = item->mask;
> 
> @@ -434,7 +437,10 @@ classify_parse_ntuple_filter(const struct
> rte_flow_attr *attr,
>  		return -EINVAL;
>  	}
> 
> -	table_type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
> +	if (have_tcp)
> +		table_type =
> RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_TCP_5TUPLE;
> +	else
> +		table_type = RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE;
> 
>  	/* parse attr */
>  	/* must be input direction */
> --
> 2.17.1

Regards,

Bernard.

Re: [dpdk-dev] [PATCH v2 2/2] examples/flow_classify: add an ACL table for tcp

[Sowmini Varadhan]

On (08/19/21 15:06), Iremonger, Bernard wrote:
> > v2 updates: typo fixes for checkpatch,  bernard.iremonger comments
> 
> The above line should not be added to the commit message.
> ~/dpdk/devtools/check-git-log.sh -1
> Wrong tag:
>         v2 fixes:  typo fixes, get_tcp_flags returns -EINVAL
> 
> The check-git-log.sh  script is in your dpdk checkout, best to run this script before sending patches

apologies, I was not aware of this, I will fix and send out v3. 
I did run check-git-log.sh but before editing the vX updates.

thanks for your patience.

--Sowmini

Re: [dpdk-dev] [PATCH v2 1/2] examples/flow_classify: hooks for filters on tcp flags

[Iremonger, Bernard]

HI Sowmini,

> -----Original Message-----
> From: Sowmini Varadhan <sovaradh@linux.microsoft.com>
> Sent: Wednesday, August 18, 2021 4:01 PM
> To: sowmini05@gmail.com; Iremonger, Bernard
> <bernard.iremonger@intel.com>; dev@dpdk.org;
> sovaradh@linux.microsoft.com
> Cc: thomas@monjalon.net
> Subject: [PATCH v2 1/2] examples/flow_classify: hooks for filters on tcp flags
> 
> v2 fixes:  typo fixes, get_tcp_flags returns -EINVAL

The above line should not be added to the commit message.
~/dpdk/devtools/check-git-log.sh -1
Wrong tag:
        v2 fixes:  typo fixes, get_tcp_flags returns -EINVAL

The check-git-log.sh  script is in your dpdk checkout, best to run this script before sending patches

> 
> The rte_eth_ntuple_filter allows tcp_flags which can check for things like
>     #define RTE_TCP_CWR_FLAG 0x80 /**< Congestion Window Reduced */
>     #define RTE_TCP_ECE_FLAG 0x40 /**< ECN-Echo */
>     #define RTE_TCP_URG_FLAG 0x20 /**< Urgent Pointer field significant */
>     #define RTE_TCP_ACK_FLAG 0x10 /**< Acknowledgment field significant
> */
>     #define RTE_TCP_PSH_FLAG 0x08 /**< Push Function */
>     #define RTE_TCP_RST_FLAG 0x04 /**< Reset the connection */
>     #define RTE_TCP_SYN_FLAG 0x02 /**< Synchronize sequence numbers */
>     #define RTE_TCP_FIN_FLAG 0x01 /**< No more data from sender */ but
> there are no existing examples that demonstrate how to use this feature.
> 
> This patch extends the existing classification support to allow an optional
> flags in the input file. The flags string can be any concatenation of characters
> from {C, E, U, A, P, R, S, F} and "*" indicates "don't care". These flags are set
> in the ntuple_filter and are used to construct the tcp_spec and tcp_mask
> sent to the driver
> 
> The rte_acl_field_def is updated to use the (u8) tcp flag as lookup key.
> Note that, as per
>   https://doc.dpdk.org/guides/prog_guide/packet_classif_access_ctrl.html
> this field MUST be allocated for 4 bytes, thus it has sizeof(uint32_t).
> 
> However, also note the XXX in this commit: additional updates are needed to
> the rte_flow_classify_table_entry_add() so that it does not ignore any key
> fields other than the 5-tuple.
> 
> Signed-off-by: Sowmini Varadhan <sovaradh@linux.microsoft.com>
> ---

If you want comment on the patch updates (optional), it can be added here after the --- line

>  examples/flow_classify/flow_classify.c     | 87 ++++++++++++++++++++--
>  examples/flow_classify/ipv4_rules_file.txt | 22 +++---
>  2 files changed, 91 insertions(+), 18 deletions(-)
> 
> diff --git a/examples/flow_classify/flow_classify.c
> b/examples/flow_classify/flow_classify.c
> index db71f5aa04..ff4c8bc2f5 100644
> --- a/examples/flow_classify/flow_classify.c
> +++ b/examples/flow_classify/flow_classify.c
> @@ -51,6 +51,7 @@ enum {
>  	CB_FLD_DST_PORT_MASK,
>  	CB_FLD_PROTO,
>  	CB_FLD_PRIORITY,
> +	CB_FLD_TCP_FLAGS,
>  	CB_FLD_NUM,
>  };
> 
> @@ -86,6 +87,7 @@ enum {
>  	DST_FIELD_IPV4,
>  	SRCP_FIELD_IPV4,
>  	DSTP_FIELD_IPV4,
> +	TCP_FLAGS_FIELD,

I think it would be better to rename TCP_FLAGS_FIELD to TCP_FLAGS_FIELD_IPV4  inline with the other values.

>  	NUM_FIELDS_IPV4
>  };
> 
> @@ -93,7 +95,8 @@ enum {
>  	PROTO_INPUT_IPV4,
>  	SRC_INPUT_IPV4,
>  	DST_INPUT_IPV4,
> -	SRCP_DESTP_INPUT_IPV4
> +	SRCP_DESTP_INPUT_IPV4,
> +	TCP_FLAGS_INDEX,

I think it would be better to rename  TCP_FLAGS_INDEX to TCP_FLAGS_INPUT_IPV4 inline with the other values.

>  };
> 
>  static struct rte_acl_field_def ipv4_defs[NUM_FIELDS_IPV4] = { @@ -150,6
> +153,17 @@ static struct rte_acl_field_def ipv4_defs[NUM_FIELDS_IPV4] = {
>  			sizeof(struct rte_ipv4_hdr) +
>  			offsetof(struct rte_tcp_hdr, dst_port),
>  	},
> +	/* next field must be 4 bytes, even though flags is only 1 byte */
> +	{
> +		/* rte_flags */
> +		.type = RTE_ACL_FIELD_TYPE_BITMASK,
> +		.size = sizeof(uint32_t),
> +		.field_index = TCP_FLAGS_FIELD,
> +		.input_index = TCP_FLAGS_INDEX,
> +		.offset = sizeof(struct rte_ether_hdr) +
> +			sizeof(struct rte_ipv4_hdr) +
> +			offsetof(struct rte_tcp_hdr, tcp_flags),
> +	},
>  };
>  /* >8 End of creation of ACL table. */
> 
> @@ -285,12 +299,14 @@ lcore_main(struct flow_classifier *cls_app)
>  	int ret;
>  	int i = 0;
> 
> -	ret = rte_flow_classify_table_entry_delete(cls_app->cls,
> -			rules[7]);
> -	if (ret)
> -		printf("table_entry_delete failed [7] %d\n\n", ret);
> -	else
> -		printf("table_entry_delete succeeded [7]\n\n");
> +	if (rules[7]) {
> +		ret = rte_flow_classify_table_entry_delete(cls_app->cls,
> +				rules[7]);
> +		if (ret)
> +			printf("table_entry_delete failed [7] %d\n\n", ret);
> +		else
> +			printf("table_entry_delete succeeded [7]\n\n");
> +	}
> 
>  	/*
>  	 * Check that the port is on the same NUMA node as the polling
> thread @@ -410,6 +426,53 @@ parse_ipv4_net(char *in, uint32_t *addr,
> uint32_t *mask_len)
>  	return 0;
>  }
> 
> +static int
> +get_tcp_flags(char *in, struct rte_eth_ntuple_filter *ntuple_filter) {
> +	int len = strlen(in);
> +	int i;
> +	uint8_t flags = 0;
> +
> +	if (strcmp(in, "*") == 0) {
> +		ntuple_filter->tcp_flags = 0;
> +		return 0;
> +	}
> +
> +	for (i = 0; i < len; i++) {
> +		switch (in[i]) {
> +		case 'S':
> +			flags |= RTE_TCP_SYN_FLAG;
> +			break;
> +		case 'F':
> +			flags |= RTE_TCP_FIN_FLAG;
> +			break;
> +		case 'R':
> +			flags |= RTE_TCP_RST_FLAG;
> +			break;
> +		case 'P':
> +			flags |= RTE_TCP_PSH_FLAG;
> +			break;
> +		case 'A':
> +			flags |= RTE_TCP_ACK_FLAG;
> +			break;
> +		case 'U':
> +			flags |= RTE_TCP_URG_FLAG;
> +			break;
> +		case 'E':
> +			flags |= RTE_TCP_ECE_FLAG;
> +			break;
> +		case 'C':
> +			flags |= RTE_TCP_CWR_FLAG;
> +			break;
> +		default:
> +			fprintf(stderr, "unknown flag %c\n", in[i]);
> +			return -EINVAL;
> +		}
> +	}
> +	ntuple_filter->tcp_flags = flags;
> +	return 0;
> +}
> +
>  static int
>  parse_ipv4_5tuple_rule(char *str, struct rte_eth_ntuple_filter
> *ntuple_filter)  { @@ -481,6 +544,8 @@ parse_ipv4_5tuple_rule(char *str,
> struct rte_eth_ntuple_filter *ntuple_filter)
>  	ntuple_filter->priority = (uint16_t)temp;
>  	if (ntuple_filter->priority > FLOW_CLASSIFY_MAX_PRIORITY)
>  		ret = -EINVAL;
> +	if (get_tcp_flags(in[CB_FLD_TCP_FLAGS], ntuple_filter))
> +		return -EINVAL;
> 
>  	return ret;
>  }
> @@ -597,10 +662,13 @@ add_classify_rule(struct rte_eth_ntuple_filter
> *ntuple_filter,
>  		memset(&tcp_spec, 0, sizeof(tcp_spec));
>  		tcp_spec.hdr.src_port = ntuple_filter->src_port;
>  		tcp_spec.hdr.dst_port = ntuple_filter->dst_port;
> +		tcp_spec.hdr.tcp_flags = ntuple_filter->tcp_flags;
> 
>  		memset(&tcp_mask, 0, sizeof(tcp_mask));
>  		tcp_mask.hdr.src_port = ntuple_filter->src_port_mask;
>  		tcp_mask.hdr.dst_port = ntuple_filter->dst_port_mask;
> +		if (tcp_spec.hdr.tcp_flags != 0)
> +			tcp_mask.hdr.tcp_flags = 0xff;
> 
>  		tcp_item.type = RTE_FLOW_ITEM_TYPE_TCP;
>  		tcp_item.spec = &tcp_spec;
> @@ -655,6 +723,11 @@ add_classify_rule(struct rte_eth_ntuple_filter
> *ntuple_filter,
>  		return ret;
>  	}
> 
> +	/* XXX but this only adds table_type of
> +	 * RTE_FLOW_CLASSIFY_TABLE_ACL_IP4_5TUPLE
> +	 * i.e., it only ever does allocate_acl_ipv4_5tuple_rule()
> +	 * so the tcp_flags is ignored!
> +	 */
>  	rule = rte_flow_classify_table_entry_add(
>  			cls_app->cls, &attr, pattern_ipv4_5tuple,
>  			actions, &key_found, &error);
> diff --git a/examples/flow_classify/ipv4_rules_file.txt
> b/examples/flow_classify/ipv4_rules_file.txt
> index dfa0631fcc..415573732a 100644
> --- a/examples/flow_classify/ipv4_rules_file.txt
> +++ b/examples/flow_classify/ipv4_rules_file.txt
> @@ -1,14 +1,14 @@
>  #file format:
> -#src_ip/masklen dst_ip/masklen src_port : mask dst_port : mask
> proto/mask priority
> +#src_ip/masklen dst_ip/masklen src_port : mask dst_port : mask
> +proto/mask priority tcp_flags
>  #
> -2.2.2.3/24 2.2.2.7/24 32 : 0xffff 33 : 0xffff 17/0xff 0
> -9.9.9.3/24 9.9.9.7/24 32 : 0xffff 33 : 0xffff 17/0xff 1
> -9.9.9.3/24 9.9.9.7/24 32 : 0xffff 33 : 0xffff 6/0xff 2
> -9.9.8.3/24 9.9.8.7/24 32 : 0xffff 33 : 0xffff 6/0xff 3
> -6.7.8.9/24 2.3.4.5/24 32 : 0x0000 33 : 0x0000 132/0xff 4
> -6.7.8.9/32 192.168.0.36/32 10 : 0xffff 11 : 0xffff 6/0xfe 5
> -6.7.8.9/24 192.168.0.36/24 10 : 0xffff 11 : 0xffff 6/0xfe 6
> -6.7.8.9/16 192.168.0.36/16 10 : 0xffff 11 : 0xffff 6/0xfe 7
> -6.7.8.9/8 192.168.0.36/8 10 : 0xffff 11 : 0xffff 6/0xfe 8
> +2.2.2.3/24 2.2.2.7/24 32 : 0xffff 33 : 0xffff 17/0xff 0 *
> +9.9.9.3/24 9.9.9.7/24 32 : 0xffff 33 : 0xffff 17/0xff 1 *
> +9.9.9.3/24 9.9.9.7/24 32 : 0xffff 33 : 0xffff 6/0xff 2 *
> +9.9.8.3/24 9.9.8.7/24 32 : 0xffff 33 : 0xffff 6/0xff 3 *
> +6.7.8.9/24 2.3.4.5/24 32 : 0x0000 33 : 0x0000 132/0xff 4 *
> +6.7.8.9/32 192.168.0.36/32 10 : 0xffff 11 : 0xffff 6/0xfe 5 *
> +6.7.8.9/24 192.168.0.36/24 10 : 0xffff 11 : 0xffff 6/0xfe 6 *
> +6.7.8.9/16 192.168.0.36/16 10 : 0xffff 11 : 0xffff 6/0xfe 7 *
> +6.7.8.9/8 192.168.0.36/8 10 : 0xffff 11 : 0xffff 6/0xfe 8 *
>  #error rules
> -#9.8.7.6/8 192.168.0.36/8 10 : 0xffff 11 : 0xffff 6/0xfe 9 \ No newline at end
> of file
> +#9.8.7.6/8 192.168.0.36/8 10 : 0xffff 11 : 0xffff 6/0xfe 9 *
> --
> 2.17.1

Regards,

Bernard.

Re: [dpdk-dev] [PATCH v2 2/2] examples/flow_classify: add an ACL table for tcp

[Iremonger, Bernard]

Hi Sowmini,

Looking closer at this patchset, I am not sure that a second ACL table is needed.
The existing ACL table handles UDP, TCP and SCP, however it is not processing the TCP flags.
I think it just needs to be modified to process the TCP flags.
Could you take another look to see if the above proposed solution will work for you.

Regards,

Bernard. 

> -----Original Message-----
> From: Sowmini Varadhan <sovaradh@linux.microsoft.com>
> Sent: Thursday, August 19, 2021 4:21 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>
> Cc: sowmini05@gmail.com; dev@dpdk.org; thomas@monjalon.net
> Subject: Re: [PATCH v2 2/2] examples/flow_classify: add an ACL table for tcp
> 
> On (08/19/21 15:06), Iremonger, Bernard wrote:
> > > v2 updates: typo fixes for checkpatch,  bernard.iremonger comments
> >
> > The above line should not be added to the commit message.
> > ~/dpdk/devtools/check-git-log.sh -1
> > Wrong tag:
> >         v2 fixes:  typo fixes, get_tcp_flags returns -EINVAL
> >
> > The check-git-log.sh  script is in your dpdk checkout, best to run this script
> before sending patches
> 
> apologies, I was not aware of this, I will fix and send out v3.
> I did run check-git-log.sh but before editing the vX updates.
> 
> thanks for your patience.
> 
> --Sowmini

Re: [dpdk-dev] [PATCH v2 2/2] examples/flow_classify: add an ACL table for tcp

[Sowmini Varadhan]

On (08/19/21 16:21), Iremonger, Bernard wrote:
> 
> Looking closer at this patchset, I am not sure that a second ACL table is needed.
> The existing ACL table handles UDP, TCP and SCP, however it is not processing the TCP flags.
> I think it just needs to be modified to process the TCP flags.
> Could you take another look to see if the above proposed solution will work for you.

I'm not sure it would. As I pointed out in the original rfc  at
https://inbox.dpdk.org/dev/cover.1578936382.git.sowmini.varadhan@microsoft.com/

we need to add a key for the 8 bit flags, but the multibit
trie lookup moves in steps of 4 bytes.

However, it has admittedly been a while since I tinkereed with this,
and I can give it a shot and get back.

Thanks
--Sowmini

Re: [dpdk-dev] [PATCH v2 2/2] examples/flow_classify: add an ACL table for tcp

[Stephen Hemminger]

On Thu, 19 Aug 2021 12:34:46 -0700
Sowmini Varadhan <sovaradh@linux.microsoft.com> wrote:

> On (08/19/21 16:21), Iremonger, Bernard wrote:
> > 
> > Looking closer at this patchset, I am not sure that a second ACL table is needed.
> > The existing ACL table handles UDP, TCP and SCP, however it is not processing the TCP flags.
> > I think it just needs to be modified to process the TCP flags.
> > Could you take another look to see if the above proposed solution will work for you.  
> 
> I'm not sure it would. As I pointed out in the original rfc  at
> https://inbox.dpdk.org/dev/cover.1578936382.git.sowmini.varadhan@microsoft.com/
> 
> we need to add a key for the 8 bit flags, but the multibit
> trie lookup moves in steps of 4 bytes.
> 
> However, it has admittedly been a while since I tinkereed with this,
> and I can give it a shot and get back.
> 
> Thanks
> --Sowmini
> 

Doing a cleanup of patch backlog prior to next LTS release.
The patch never got updated based on comments.
Please resubmit if you still want to pursue it.