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

[Andrew Rybchenko <arybchenko@solarflare.com>]

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.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?

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 <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