DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Rybchenko <arybchenko@solarflare.com>
To: Shahaf Shuler <shahafs@mellanox.com>,
	"pbhagavatula@marvell.com" <pbhagavatula@marvell.com>,
	"jerinj@marvell.com" <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] [PATCH 3/7] ethdev: add flow action type update as an offload
Date: Sun, 18 Aug 2019 10:08:47 +0300
Message-ID: <e63de405-efcd-0dd7-f06d-2c9c95226f4a@solarflare.com> (raw)
In-Reply-To: <AM0PR0502MB379530AB78CD68D1EE572D22C3A90@AM0PR0502MB3795.eurprd05.prod.outlook.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


  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-16  5:55 [dpdk-dev] [PATCH 0/7] ethdev: add new Rx offload flags pbhagavatula
2019-08-16  5:55 ` [dpdk-dev] [PATCH 1/7] ethdev: add set ptype function pbhagavatula
2019-08-16  8:22   ` Andrew Rybchenko
2019-08-17 16:27     ` [dpdk-dev] [EXT] " Pavan Nikhilesh Bhagavatula
2019-08-16  5:55 ` [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as a offload pbhagavatula
2019-08-16  7:48   ` Andrew Rybchenko
2019-08-17 11:47     ` Pavan Nikhilesh Bhagavatula
2019-08-18  4:52     ` Shahaf Shuler
2019-08-18  5:38       ` Andrew Rybchenko
2019-08-18  6:18         ` Shahaf Shuler
2019-08-18  7:00           ` Andrew Rybchenko
2019-08-18 12:11             ` Shahaf Shuler
2019-08-16  5:55 ` [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as an offload pbhagavatula
2019-08-16  8:05   ` Andrew Rybchenko
2019-08-17 14:23     ` [dpdk-dev] [EXT] " Pavan Nikhilesh Bhagavatula
2019-08-18  9:46       ` Andrew Rybchenko
2019-08-18  4:59     ` [dpdk-dev] " Shahaf Shuler
2019-08-18  5:57       ` Andrew Rybchenko
2019-08-18  6:20         ` Shahaf Shuler
2019-08-18  7:08           ` Andrew Rybchenko [this message]
2019-08-16  5:55 ` [dpdk-dev] [PATCH 4/7] net: update Rx RSS hash offload capabilities pbhagavatula
2019-08-16  7:49   ` Andrew Rybchenko
2019-08-17 12:26     ` Pavan Nikhilesh Bhagavatula
2019-08-16  5:55 ` [dpdk-dev] [PATCH 5/7] net: update Rx flow action " pbhagavatula
2019-08-16  7:50   ` Andrew Rybchenko
2019-08-16  5:55 ` [dpdk-dev] [PATCH 6/7] net: add ptype set default functionality pbhagavatula
2019-08-16  8:30   ` Andrew Rybchenko
2019-08-17 17:24     ` Pavan Nikhilesh Bhagavatula
2019-08-16  5:55 ` [dpdk-dev] [PATCH 7/7] examples/eventdev_pipeline: add new Rx RSS hash offload pbhagavatula
2019-08-16  6:01   ` Jerin Jacob Kollanukkaran
2019-08-16  6:02 ` [dpdk-dev] [PATCH 0/7] ethdev: add new Rx offload flags Jerin Jacob Kollanukkaran

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e63de405-efcd-0dd7-f06d-2c9c95226f4a@solarflare.com \
    --to=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jerinj@marvell.com \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=pbhagavatula@marvell.com \
    --cc=shahafs@mellanox.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox