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

[Ye Xiaolong <xiaolong.ye@intel.com>]

On 11/19, Zhang, Qi Z wrote:
>
>
>> -----Original Message-----
>> From: Ye, Xiaolong <xiaolong.ye@intel.com>
>> Sent: Tuesday, November 19, 2019 1:53 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>
>> Cc: Yang, Qiming <qiming.yang@intel.com>; Cao, Yahui
>> <yahui.cao@intel.com>; Xing, Beilei <beilei.xing@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v3] net/ice: add flow mark hint support
>> 
>> On 11/18, Qi Zhang wrote:
>> >Since not all data paths support flow mark, the driver need
>> 
>> s/need/needs
>> 
>> >a hint from application to select the correct data path if flow mark is
>> >required. The patch introduce a devarg
>> 
>> s/introduce/introduces
>> 
>> >"flow-mark-support" as a workaround solution, since a standard way is
>> >still ongoing.
>> >
>> >Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>> >---
>> >
>> >v3:
>> >- add "experimental notification" in ice.rst
>> >
>> >v2:
>> >- fix typo
>> >
>> > doc/guides/nics/ice.rst               | 12 ++++++++++++
>> > 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, 28 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst index
>> >1a426438d..d1b80c2fa 100644
>> >--- a/doc/guides/nics/ice.rst
>> >+++ b/doc/guides/nics/ice.rst
>> >@@ -80,6 +80,18 @@ 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
>> 
>> s/support/supports
>> 
>> >+  by default.
>> >+  NOTE: This is an experimental devarg, it will be removed when any of
>> >+ below conditions  is ready.
>> >+  1) all data path support flow mark (currently vPMD does not)
>> 
>> s/path/paths
>> 
>> >+  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);
>> 
>> Do we need error handling here?
>> 
>> >+	printf("flow_mark = %d\n", ad->devargs.flow_mark_support);
>> 
>> Use PMD_INIT_LOG instead, and in order to provide more informative
>> message, what about we directly say "flow_mark is 0, vector path won't be
>> selected." or something similar according to the value of flow_makr_support.
>> 
>
>This is a mistake forgot to remove the debug code.

Ok, I think it's still meaningful to print some hints in ice_rx_vec_dev_check_default
when it return due to flow_mark_support is set.

Thanks,
Xiaolong