Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as an offload

[Shahaf Shuler <shahafs@mellanox.com>]

Sunday, August 18, 2019 8:57 AM, Andrew Rybchenko:
> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as
> an offload
> 
> On 8/18/19 7:59 AM, Shahaf Shuler wrote:
> > Friday, August 16, 2019 11:05 AM, Andrew Rybchenko:
> >> <marko.kovacevic@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type
> >> update as an offload
> >>
> >> On 8/16/19 8:55 AM, pbhagavatula@marvell.com wrote:
> >>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>>
> >>> Add new Rx offload flag `DEV_RX_OFFLOAD_FLOW_MARK` that can be
> >> used to
> >>> enable/disable PMDs write to `rte_mbuf::hash::fdir::hi`.
> >> Notes similar to RSS hash.
> >>
> >> It requires better motivation why. It lets Rx queue know that it will
> >> be used as flow action MARK target and the queue should be configured
> >> to deliver the mark from NIC to PMD and processed in the driver.
> > This one is even worse than the RSS (sorry).
> >
> > First - the API breakage exists also here and if we want to include such
> patch we should have proper doc on RN.
> 
> Yes, there is a deprecation notice for it in v19.08 and I already mentioned in
> review notes for the patch [1/7] that release notes are required.
> 
> > Second - the user explicitly inserted a rte_flow rule w/ action mark. Its
> expectation is to receive his mark on the mbuf (otherwise why would it set
> this action?).  it is not expected from the user to set another offload flag just
> to enable the mark set on the mbuf. It makes the user experience very
> convoluted.
> > Third - so far we never reported rte_flow capabilities, and there is a good
> reason for it - the cap matrix is too big. For rte_flow the chosen approach was
> trail and error. If we start w/ the flow mark, the amount of cap bits the
> application will need to monitor will be huge.
> 
> The feature differs a lot from other rte_flow features since it adds Rx meta
> information.
> And yes, rte_flow rule to set mark should fail with appropriate diagnostics if
> the offload is supported by not enabled or not supported at all. So,
> application will be informed and I think it is less worse than RSS hash.
> 
> > My suggestion here, for PMD that wants to optimize their datapath is to
> check if flow w/ mark action was inserted on the queue. So long there is no
> such flow they can disable the set of the mark.
> 
> Unfortunately the information is required on Rx queue setup stage and
> rte_rule insertion is too late.

See my comments on the RSS patch. Same point of discussion.

The main question we need to answer -
User set flow mark action by rte_flow (that was accepted). Why does it need to set more flag? 

> 
> Anyway, the important point here is all Rx offloads consistency:
> want something - enable it. The only remaining exception is packet type
> which default behaviour is preserved since really flexible solution suggested
> by Konstantin.
> 
> >> Also I think that flow API action MARK documentation should be
> >> updated to mentioned the offload.
> >>
> >>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>> ---
> >>>    doc/guides/nics/features.rst   | 12 ++++++++++++
> >>>    lib/librte_ethdev/rte_ethdev.h |  1 +
> >>>    2 files changed, 13 insertions(+)
> >>>
> >>> diff --git a/doc/guides/nics/features.rst
> >>> b/doc/guides/nics/features.rst index f79b69b38..d67430d90 100644
> >>> --- a/doc/guides/nics/features.rst
> >>> +++ b/doc/guides/nics/features.rst
> >>> @@ -594,6 +594,18 @@ application to set ptypes it is interested in.
> >>>    * **[provides]   mbuf**: ``mbuf.packet_type``.
> >>>
> >>>
> >>> +.. _nic_features_flow_action_type_update:
> >> May be  _nic_features_flow_mark ?
> >>
> >>> +
> >>> +Flow type update
> >> May be "Flow mark delivery" ?
> >>
> >>> +----------------
> >>> +
> >>> +Supports flow action type update to ``mbuf.ol_flags`` and
> >> ``mbuf.hash.fdir.hi``.
> >>> +
> >>> +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**:
> >> ``offloads:DEV_RX_OFFLOAD_FLOW_TYPE``.
> >>
> >> DEV_RX_OFFLOAD_FLOW_MARK, not TYPE.
> >>
> >>
> >>
> >>> +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_FDIR``,
> >>> +``mbuf.ol_flags:PKT_RX_FDIR_ID;``,
> >>> +  ``mbuf.hash.fdir.hi``
> >>> +
> >>> +
> >>>    .. _nic_features_timesync:
> >>>
> >>>    Timesync
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >>> b/lib/librte_ethdev/rte_ethdev.h index 889486a11..4a0cff830 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>> @@ -1014,6 +1014,7 @@ struct rte_eth_conf {
> >>>    #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> >>>    #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> >>>    #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> >>> +#define DEV_RX_OFFLOAD_FLOW_MARK	0x00100000
> >>>
> >>>    #define DEV_RX_OFFLOAD_CHECKSUM
> >> (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> >>>    				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> >> Add to rte_rx_offload_names