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

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

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.