On 8/18/19 9:20 AM, Shahaf Shuler wrote:
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.kovace...@intel.com>; Thomas Monjalon
<tho...@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, pbhagavat...@marvell.com wrote:
From: Pavan Nikhilesh <pbhagavat...@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?
I think all arguments are on the table.
Mine:
- possibility to squeeze more performance from HW and SW
- consistency (at least as I see it) in filling in of the information
in mbuf
(direct using corresponding offloads)
Your:
- more complexity in control (which is less painful in the case of
flow mark,
since it should be an error on flow rule setup, and a bit more
painful in
in the case of RSS hash since there is simply no RSS hash if not
enabled)
- API breakage (but there is deprecation notice in v19.08)
Many thanks for the discussion.
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 <pbhagavat...@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
