Re: [dpdk-dev] [PATCH v2] net/ice: add flow mark hint support

["Stillwell Jr, Paul M" <paul.m.stillwell.jr@intel.com>]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Qi Zhang
> Sent: Thursday, November 14, 2019 7:42 PM
> To: Ye, Xiaolong <xiaolong.ye@intel.com>
> Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: [dpdk-dev] [PATCH v2] net/ice: add flow mark hint support
> 
> Since not all data paths support flow mark, the driver need a hint from
> application to select the correct data path if flow mark is required. The patch
> introduce a devarg "flow-mark-support" as a workaround solution, since a
> standard way is still ongoing.
> 

I understand the need for this, but this seems problematic. Once a customer starts using this, then it has to be in the code forever because they are going to expect it to work forever. Maybe this should be marked with something that indicates it is temporary? Something like the experimental tagging that is done in other parts of the code?

> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
> 
> v2:
> - fix typo
> 
>  doc/guides/nics/ice.rst               | 10 ++++++++++
>  drivers/net/ice/ice_ethdev.c          | 11 ++++++++++-
>  drivers/net/ice/ice_ethdev.h          |  1 +
>  drivers/net/ice/ice_rxtx_vec_common.h |  5 +++++
>  4 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst index
> 1a426438d..f7016d338 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -80,6 +80,16 @@ Runtime Config Options
> 
>      -w 80:00.0,pipeline-mode-support=1
> 
> +- ``Flow Mark Support`` (default ``0``)
> +
> +  This is a hint to the driver to select the data path that support
> + flow mark extraction  by default. This hint should be removed when any of
> below condition ready.
> +  1) all data path support flow mark (currently vPMD does not)
> +  2) a new offload like RTE_DEV_RX_OFFLOAD_FLOW_MARK be introduced
> as a standard way to hint.
> +  Example::
> +
> +    -w 80:00.0,flow-mark-support=1
> +
>  - ``Protocol extraction for per queue``
> 
>    Configure the RX queues to do protocol extraction into mbuf for protocol
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index abf00d404..9f2cb2f40 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -23,11 +23,13 @@
>  /* devargs */
>  #define ICE_SAFE_MODE_SUPPORT_ARG "safe-mode-support"
>  #define ICE_PIPELINE_MODE_SUPPORT_ARG  "pipeline-mode-support"
> +#define ICE_FLOW_MARK_SUPPORT_ARG	"flow-mark-support"
>  #define ICE_PROTO_XTR_ARG         "proto_xtr"
> 
>  static const char * const ice_valid_args[] = {
>  	ICE_SAFE_MODE_SUPPORT_ARG,
>  	ICE_PIPELINE_MODE_SUPPORT_ARG,
> +	ICE_FLOW_MARK_SUPPORT_ARG,
>  	ICE_PROTO_XTR_ARG,
>  	NULL
>  };
> @@ -1987,6 +1989,12 @@ static int ice_parse_devargs(struct rte_eth_dev
> *dev)
> 
>  	ret = rte_kvargs_process(kvlist,
> ICE_PIPELINE_MODE_SUPPORT_ARG,
>  				 &parse_bool, &ad-
> >devargs.pipe_mode_support);
> +	if (ret)
> +		goto bail;
> +
> +	ret = rte_kvargs_process(kvlist, ICE_FLOW_MARK_SUPPORT_ARG,
> +				 &parse_bool, &ad-
> >devargs.flow_mark_support);
> +	printf("flow_mark = %d\n", ad->devargs.flow_mark_support);
> 
>  bail:
>  	rte_kvargs_free(kvlist);
> @@ -4571,7 +4579,8 @@ RTE_PMD_REGISTER_KMOD_DEP(net_ice, "*
> igb_uio | uio_pci_generic | vfio-pci");
> RTE_PMD_REGISTER_PARAM_STRING(net_ice,
>  			      ICE_PROTO_XTR_ARG
> "=[queue:]<vlan|ipv4|ipv6|ipv6_flow|tcp>"
>  			      ICE_SAFE_MODE_SUPPORT_ARG "=<0|1>"
> -			      ICE_PIPELINE_MODE_SUPPORT_ARG "=<0|1>");
> +			      ICE_PIPELINE_MODE_SUPPORT_ARG "=<0|1>"
> +			      ICE_FLOW_MARK_SUPPORT_ARG "=<0|1>");
> 
>  RTE_INIT(ice_init_log)
>  {
> diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
> index 4a0d37b32..4d35339a7 100644
> --- a/drivers/net/ice/ice_ethdev.h
> +++ b/drivers/net/ice/ice_ethdev.h
> @@ -391,6 +391,7 @@ struct ice_devargs {
>  	int safe_mode_support;
>  	uint8_t proto_xtr_dflt;
>  	int pipe_mode_support;
> +	int flow_mark_support;
>  	uint8_t proto_xtr[ICE_MAX_QUEUE_NUM];
>  };
> 
> diff --git a/drivers/net/ice/ice_rxtx_vec_common.h
> b/drivers/net/ice/ice_rxtx_vec_common.h
> index 080ca4175..086428898 100644
> --- a/drivers/net/ice/ice_rxtx_vec_common.h
> +++ b/drivers/net/ice/ice_rxtx_vec_common.h
> @@ -268,6 +268,11 @@ ice_rx_vec_dev_check_default(struct rte_eth_dev
> *dev)  {
>  	int i;
>  	struct ice_rx_queue *rxq;
> +	struct ice_adapter *ad =
> +		ICE_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
> +
> +	if (ad->devargs.flow_mark_support)
> +		return -1;
> 
>  	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>  		rxq = dev->data->rx_queues[i];
> --
> 2.13.6