Re: [dpdk-dev] [PATCH v2] net/mlx5: support e-switch TCP-flags flow filter

[Shahaf Shuler <shahafs@mellanox.com>]

Hi Moty, 

Please see few comments. 

Tuesday, October 9, 2018 2:18 PM, Mordechay Haimovsky:
> Subject: [PATCH v2] net/mlx5: support e-switch TCP-flags flow filter
> 
> This patch adds support for offloading flow rules with tcp-flags filter to mlx5
> eswitch hardware.

Hardware.

> 
> Today, it is possible to offload an interface flow rules to the hardware using
> DPDK flow commands.
> With mlx5 it is also possible to offload a limited set of flow rules to the mlxsw
> (or e-switch) using the same DPDK flow commands.

This part Is not needed. You can say it is supported only under the transfer attribute. 

> A 'transfer' attribute was added to the flow rule creation command in order
> to distinguish between configuring port flows and E-switch flows.
> The commands destined for the switch are transposed to TC flower rules and
> are send, as Netlink messages, to the mlx5 driver, or more precisely to the
> netdev which represents the mlxsw port.

No need for the last phrase. Just say that this patch implement the TCP flags under the transfer attr. 


> The limited set of e-switch flow rules also supports filtering according to the
> values found in the TCP flags. This patch implements this offload capability in
> the mlx5 PMD.
> 
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> ---
> v2:
>  * Code rebase on latest upstream updates.
> ---
>  drivers/net/mlx5/Makefile        | 10 ++++++++++
>  drivers/net/mlx5/mlx5_flow.c     |  9 ++++++++-
>  drivers/net/mlx5/mlx5_flow_tcf.c | 19 +++++++++++++++++++
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile index
> ca1de9f..9097456 100644
> --- a/drivers/net/mlx5/Makefile
> +++ b/drivers/net/mlx5/Makefile
> @@ -342,6 +342,16 @@ mlx5_autoconf.h.new: $(RTE_SDK)/buildtools/auto-
> config-h.sh
>  		enum TCA_FLOWER_KEY_VLAN_ETH_TYPE \
>  		$(AUTOCONF_OUTPUT)
>  	$Q sh -- '$<' '$@' \
> +		HAVE_TCA_FLOWER_KEY_TCP_FLAGS \
> +		linux/pkt_cls.h \
> +		enum TCA_FLOWER_KEY_TCP_FLAGS \
> +		$(AUTOCONF_OUTPUT)
> +	$Q sh -- '$<' '$@' \
> +		HAVE_TCA_FLOWER_KEY_TCP_FLAGS_MASK \
> +		linux/pkt_cls.h \
> +		enum TCA_FLOWER_KEY_TCP_FLAGS_MASK \
> +		$(AUTOCONF_OUTPUT)
> +	$Q sh -- '$<' '$@' \

Need to update also meson.build file. 

>  		HAVE_TC_ACT_VLAN \
>  		linux/tc_act/tc_vlan.h \
>  		enum TCA_VLAN_PUSH_VLAN_PRIORITY \
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index ed60c40..c3dd060 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -1239,6 +1239,13 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
>  			    struct rte_flow_error *error)
>  {
>  	const struct rte_flow_item_tcp *mask = item->mask;
> +	const struct rte_flow_item_tcp nic_mask = {
> +		.hdr = {
> +			.src_port = RTE_BE16(0xffff),
> +			.dst_port = RTE_BE16(0xffff),
> +			.tcp_flags = 0xff,
> +		},
> +	};

This validate function for TCP is shared between verbs, TC and DV flow engines.
The addition of the tcp_flags in this function would mean that verbs/dv also support such filtering, while it is not the case today. 
Maybe you should have different TCP validate function for tcf. 

>  	const int tunnel = !!(item_flags & MLX5_FLOW_LAYER_TUNNEL);
>  	int ret;
> 
> @@ -1261,7 +1268,7 @@ uint32_t mlx5_flow_adjust_priority(struct
> rte_eth_dev *dev, int32_t priority,
>  		mask = &rte_flow_item_tcp_mask;
>  	ret = mlx5_flow_item_acceptable
>  		(item, (const uint8_t *)mask,
> -		 (const uint8_t *)&rte_flow_item_tcp_mask,
> +		 (const uint8_t *)&nic_mask,
>  		 sizeof(struct rte_flow_item_tcp), error);
>  	if (ret < 0)
>  		return ret;
> diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c
> b/drivers/net/mlx5/mlx5_flow_tcf.c
> index 91f6ef6..d39ec2b 100644
> --- a/drivers/net/mlx5/mlx5_flow_tcf.c
> +++ b/drivers/net/mlx5/mlx5_flow_tcf.c
> @@ -148,6 +148,12 @@ struct tc_vlan {
>  #ifndef HAVE_TCA_FLOWER_KEY_VLAN_ETH_TYPE  #define
> TCA_FLOWER_KEY_VLAN_ETH_TYPE 25  #endif
> +#ifndef HAVE_TCA_FLOWER_KEY_TCP_FLAGS
> +#define TCA_FLOWER_KEY_TCP_FLAGS 71
> +#endif
> +#ifndef HAVE_TCA_FLOWER_KEY_TCP_FLAGS_MASK
> +#define TCA_FLOWER_KEY_TCP_FLAGS_MASK 72 #endif
> 
>  #ifndef IPV6_ADDR_LEN
>  #define IPV6_ADDR_LEN 16
> @@ -204,6 +210,7 @@ struct tc_vlan {
>  	.tcp.hdr = {
>  		.src_port = RTE_BE16(0xffff),
>  		.dst_port = RTE_BE16(0xffff),
> +		.tcp_flags = 0xff,
>  	},
>  	.udp.hdr = {
>  		.src_port = RTE_BE16(0xffff),
> @@ -1289,6 +1296,18 @@ struct flow_tcf_ptoi {
> 
> TCA_FLOWER_KEY_TCP_DST_MASK,
>  						 mask.tcp->hdr.dst_port);
>  			}
> +			if (mask.tcp->hdr.tcp_flags) {
> +				mnl_attr_put_u16
> +					(nlh,
> +					 TCA_FLOWER_KEY_TCP_FLAGS,
> +					 rte_cpu_to_be_16
> +						(spec.tcp->hdr.tcp_flags));
> +				mnl_attr_put_u16
> +					(nlh,
> +
> TCA_FLOWER_KEY_TCP_FLAGS_MASK,
> +					 rte_cpu_to_be_16
> +						(mask.tcp->hdr.tcp_flags));
> +			}
>  			break;
>  		default:
>  			return rte_flow_error_set(error, ENOTSUP,
> --
> 1.8.3.1