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

From: Andrew Rybchenko <arybchenko@solarflare.com>
To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>,
	"Jerin Jacob Kollanukkaran" <jerinj@marvell.com>,
	"ferruh.yigit@intel.com" <ferruh.yigit@intel.com>,
	John McNamara <john.mcnamara@intel.com>,
	"Marko Kovacevic" <marko.kovacevic@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [EXT] Re: [PATCH 3/7] ethdev: add flow action type update as an offload
Date: Sun, 18 Aug 2019 12:46:55 +0300
Message-ID: <e1f884b3-9d92-b417-f628-a5abb2356572@solarflare.com> (raw)
In-Reply-To: <CY4PR1801MB186343CAD07C1D59443165D8DEAE0@CY4PR1801MB1863.namprd18.prod.outlook.com>

On 8/17/19 5:23 PM, Pavan Nikhilesh Bhagavatula wrote:
>>> 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.
>>
>> Also I think that flow API action MARK documentation should be
>> updated to mentioned the offload.
> Ack will reword the commit log and update the documentation.
>>> 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" ?
> I named it flow action type update because we use the same flag to represent two different
> flow types i.e. RTE_FLOW_ACTION_TYPE_FLAG and RTE_FLOW_ACTION_TYPE_MARK.
>
> I'm not so sure about the name RX_OFFLOAD_FLOW_MARK too because mbuf might have ol_flags
> marked for ACTION_TYPE_FLAG. Let me know your thoughts on this.

I forgot about RTE_FLOW_ACTION_TYPE_FLAG and really care about mark only
since it is 32-bit vs one. Anyway may be it is more correct to care 
about both.
If so,  may be _nic_features_flow_flag_mark and "Flow flag/mark update"?

> I will modify the name once we come to an agreement.
>
>>> +----------------
>>> +
>>> +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.
> Ack, will fix in v2.
>
>>
>>
>>> +* **[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_Udrivers/net/sfc/base/rhead_ev.c.origDP_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