Re: [dpdk-dev] [PATCH] net/i40e: Fail rte_flow MARK requests if RX func was vectorized

From: "Zhang, Qi Z" <qi.z.zhang@intel.com>
To: "Ergin, Mesut A" <mesut.a.ergin@intel.com>,
	"Xing, Beilei" <beilei.xing@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] net/i40e: Fail rte_flow MARK requests if RX func was vectorized
Date: Fri, 24 May 2019 02:48:31 +0000	[thread overview]
Message-ID: <039ED4275CED7440929022BC67E7061153386AB5@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <1558634242-441514-1-git-send-email-mesut.a.ergin@intel.com>

Hi Mesut:

> -----Original Message-----
> From: Ergin, Mesut A
> Sent: Friday, May 24, 2019 1:57 AM
> To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Ergin, Mesut A <mesut.a.ergin@intel.com>
> Subject: [PATCH] net/i40e: Fail rte_flow MARK requests if RX func was
> vectorized
> 
> Runtime requests to install an rte_flow with MARK action should fail if a vector
> RX function was already chosen for the device during configuration time.
> 
> Currently, i40e rte_flow driver would successfully install the flow with MARK
> action, even when vector RX functions are in use. However, those vector RX
> functions will fail to retrieve the MARK data from the device descriptor into
> the mbuf. The original app  installing the flow would never know what went
> wrong. The change introduced in this patch must be reverted if/when vector
> RX functions start supporting correct FDIR processing for MARK actions.
> 
> Signed-off-by: Mesut Ali Ergin <mesut.a.ergin@intel.com>
> ---
>  drivers/net/i40e/i40e_flow.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c index
> 3afd779..9a14a1c 100644
> --- a/drivers/net/i40e/i40e_flow.c
> +++ b/drivers/net/i40e/i40e_flow.c
> @@ -22,6 +22,7 @@
>  #include "base/i40e_type.h"
>  #include "base/i40e_prototype.h"
>  #include "i40e_ethdev.h"
> +#include "i40e_rxtx.h"
> 
>  #define I40E_IPV6_TC_MASK	(0xFF << I40E_FDIR_IPv6_TC_OFFSET)
>  #define I40E_IPV6_FRAG_HEADER	44
> @@ -3079,6 +3080,16 @@ i40e_flow_parse_fdir_action(struct rte_eth_dev
> *dev,
>  		filter->action.behavior = I40E_FDIR_PASSTHRU;
>  		break;
>  	case RTE_FLOW_ACTION_TYPE_MARK:
> +		if (dev->rx_pkt_burst == i40e_recv_scattered_pkts_vec ||
> +		    dev->rx_pkt_burst == i40e_recv_pkts_vec ||
> +		    dev->rx_pkt_burst == i40e_recv_scattered_pkts_vec_avx2 ||
> +		    dev->rx_pkt_burst == i40e_recv_pkts_vec_avx2) {
> +			/* Vector RX Functions do not support MARK*/
> +			rte_flow_error_set(error, EINVAL,
> +					   RTE_FLOW_ERROR_TYPE_ACTION, act,
> +					   "Action not supported when vector RX is in use.");
> +			return -rte_errno;
> +		}

We may not need to prevent this if the device is already stopped (check dev->data->dev_started), 
since with the fdir flow be created here, vector path will not be selected in following dev_start.

>  		filter->action.behavior = I40E_FDIR_PASSTHRU;
>  		mark_spec = act->conf;
>  		filter->action.report_status = I40E_FDIR_REPORT_ID; @@ -3103,6
> +3114,16 @@ i40e_flow_parse_fdir_action(struct rte_eth_dev *dev,
>  			   "Invalid action.");
>  			return -rte_errno;
>  		}
> +		if (dev->rx_pkt_burst == i40e_recv_scattered_pkts_vec ||
> +		    dev->rx_pkt_burst == i40e_recv_pkts_vec ||
> +		    dev->rx_pkt_burst == i40e_recv_scattered_pkts_vec_avx2 ||
> +		    dev->rx_pkt_burst == i40e_recv_pkts_vec_avx2) {
> +			/* Vector RX Functions do not support MARK*/
> +			rte_flow_error_set(error, EINVAL,
> +					   RTE_FLOW_ERROR_TYPE_ACTION, act,
> +					   "Action not supported when vector RX is in use.");
> +			return -rte_errno;
> +		}
>  		mark_spec = act->conf;
>  		filter->action.report_status = I40E_FDIR_REPORT_ID;
>  		filter->soft_id = mark_spec->id;
> --
> 2.7.4