From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
To: Jerin Jacob <jerinjacobk@gmail.com>
Cc: Thomas Monjalon <thomas@monjalon.net>, Ori Kam <orika@nvidia.com>,
Ivan Malov <ivan.malov@arknetworks.am>,
Ivan Malov <ivan.malov@oktetlabs.ru>,
Ferruh Yigit <ferruh.yigit@amd.com>,
Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>,
Aman Singh <aman.deep.singh@intel.com>,
Yuying Zhang <yuying.zhang@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>,
Hanumanth Reddy Pothula <hpothula@marvell.com>,
Slava Ovsiienko <viacheslavo@nvidia.com>,
Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
"david.marchand@redhat.com" <david.marchand@redhat.com>
Subject: Re: [EXT] Re: [PATCH v5 2/2] app/testpmd: add command to process Rx metadata negotiation
Date: Wed, 1 Feb 2023 13:58:40 +0300 [thread overview]
Message-ID: <98a80c20-a5e4-deea-f7dc-c6aa5d52800b@oktetlabs.ru> (raw)
In-Reply-To: <CALBAE1MCMmsrFzJC2=U7kZTFX8DMvv1Uv1mDGFBMx6C9pNRgeQ@mail.gmail.com>
On 2/1/23 13:48, Jerin Jacob wrote:
> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru> wrote:
>>
>> On 2/1/23 12:14, Jerin Jacob wrote:
>>> On Wed, Feb 1, 2023 at 2:37 PM Andrew Rybchenko
>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>
>>>> On 2/1/23 12:05, Thomas Monjalon wrote:
>>>>> 01/02/2023 10:00, Ori Kam:
>>>>>> Hi all,
>>>>>>
>>>>>> Sorry for jumping in late,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Jerin Jacob <jerinjacobk@gmail.com>
>>>>>>> Sent: Wednesday, 1 February 2023 10:53
>>>>>>>
>>>>>>> On Wed, Feb 1, 2023 at 12:46 PM Andrew Rybchenko
>>>>>>> <andrew.rybchenko@oktetlabs.ru> wrote:
>>>>>>>>
>>>>>>>> On 2/1/23 09:10, Ivan Malov wrote:
>>>>>>>>> Hello everyone,
>>>>>>>>>
>>>>>>>>> Since making automatic, or implicit, offload decisions does
>>>>>>>>> not belong in testpmd responsibility domain, it should be
>>>>>>>>> safer to avoid calling the "negotiate metadata delivery"
>>>>>>>>> API with some default selection unless the user asks to
>>>>>>>>> do so explicitly, via internal CLI or app options.
>>>>>>>>>
>>>>>>>>> With that in mind, port config <port_id> ... sounds OK.
>>>>>>>>>
>>>>>>>>> PMDs that support flow primitives which can generate metadata
>>>>>>>>> but, if in started state, can't enable its delivery may pass
>>>>>>>>> appropriate rte_error messages to the user suggesting
>>>>>>>>> they enable delivery of such metadata from NIC to PMD
>>>>>>>>> first. This way, if the person operating testpmd
>>>>>>>>> enters a flow create command and that fails,
>>>>>>>>> they can figure out the inconsistency, stop
>>>>>>>>> the port, negotiate, start and try again.
>>>>>>>>>
>>>>>>>>> As for non-debug applications, their developers shall
>>>>>>>>> be properly informed about the problem of enabling
>>>>>>>>> delivery of metadata from NIC to PMD. This way,
>>>>>>>>> they will invoke the negotiate API by default
>>>>>>>>> in their apps, with the feature selection (eg.
>>>>>>>>> MARK) as per nature of the app's business.
>>>>>>>>>
>>>>>>>>> This API should indeed be helpful to some PMDs with
>>>>>>>>> regard to collecting upfront knowledge like this.
>>>>>>>>> At the same time, should be benign to those PMDs
>>>>>>>>> who do not need this knowledge and can enable
>>>>>>>>> delivery of metadata right when inserting the
>>>>>>>>> flow rules. So I hope the API does not create
>>>>>>>>> too much discomfort to vendors not needing it.
>>>>>>>>>
>>>>>>>>> Thank you.
>>>>>>>>>
>>>>>>>>> On Wed, 1 Feb 2023, Thomas Monjalon wrote:
>>>>>>>>>
>>>>>>>>>> 31/01/2023 17:17, Jerin Jacob:
>>>>>>>>>>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon
>>>>>>> <thomas@monjalon.net>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram:
>>>>>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>>>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram:
>>>>>>>>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>>>>>>>>> Ferruh is proposing to have a command "port config <port_id>
>>>>>>> ..."
>>>>>>>>>>>>>>>> to configure the flags to negotiate.
>>>>>>>>>>>>>>>> Are you OK with this approach?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yes, we are fine to have such command to enable and disable the
>>>>>>>>>>>>>>> feature
>>>>>>>>>>>>>>> with default being it disabled if supported by PMD.
>>>>>>>>>>>>>>> Is default being disabled fine if the feature is supported by a
>>>>>>>>>>>>>>> PMD ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I think the default should be enabled for ease of use.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Since testpmd is used extensively for benchmarking purposes, we
>>>>>>>>>>>>> thought it should have minimum features
>>>>>>>>>>>>> enabled by default. The default testpmd doesn't have any Rx/Tx
>>>>>>>>>>>>> offloads enabled(except for FAST FREE), default
>>>>>>>>>>>>> fwd mode being "iofwd" and the Rx metadata is only referenced
>>>>>>> when
>>>>>>>>>>>>> dumping packets.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Do we have similar features disables by default?
>>>>>>>>>>>>>> I mean do we know features in testpmd which require a "double
>>>>>>>>>>>>>> enablement"
>>>>>>>>>>>>>> like one configuration command + one rte_flow rule?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Spec itself is that way i.e "RTE_FLOW_RULE +
>>>>>>>>>>>>> RX_METADATA_NEGOTIATE(once)"
>>>>>>>>>>>>>
>>>>>>>>>>>>> Isn't it enough if
>>>>>>>>>>>>>
>>>>>>>>>>>>> #1 We have enough print when rte_flow is being create without
>>>>>>>>>>>>> negotiation done and ask user to enable rx metadata using
>>>>>>>>>>>>> "port config <port_id>..."
>>>>>>>>>>>>> #2 Provide testpmd app arg to enable Rx metadata(for example "
>>>>>>>>>>>>> --rx-metadata") like other features to avoid calling another
>>>>>>>>>>>>> command before rte flow create.
>>>>>>>>>>>>
>>>>>>>>>>>> I'm not sure what is best.
>>>>>>>>>>>> I will let others discuss this part.
>>>>>>>>>>>
>>>>>>>>>>> IMO, enabling something always defeat the purpose to negotiate. IMO,
>>>>>>>>>>> someone needs to negotiate
>>>>>>>>>>> for a feature if the feature is needed. I think, the double enablement
>>>>>>>>>>> is part of the spec itself. In case, The PMD
>>>>>>>>>>> drivers won't like double enablement, no need to implement the PMD
>>>>>>>>>>> callback. That way, there is no change in the existing flow.
>>>>>>>>>>>
>>>>>>>>>>> The reason why cnxk driver thought of leveraging negotiate() feature
>>>>>>>>>>> so that it helps for optimization. e.s.p
>>>>>>>>>>> function template for multiprocess case as we know what features
>>>>>>>>>>> needed in fastpath upfront.
>>>>>>>>>>>
>>>>>>>>>>> If there still concerns with patch we can take up this to TB decide
>>>>>>>>>>> one way or another to make forward progress. Let us know.
>>>>>>>>>>
>>>>>>>>>> Ferruh, Andrew, Ori, Ivan, what is your opinion?
>>>>>>>>>> Let's progress with this patch to make it in -rc1.
>>>>>>>>
>>>>>>>> As I understand all agreed that we need testpmd command to
>>>>>>>> control negotiated Rx metadata. May be even command-line
>>>>>>>> option would be useful.
>>>>>>>>
>>>>>>>> So, remaining question is what should be the default value in
>>>>>>>> testpmd. Note that it is just testpmd question since default
>>>>>>>> value in an abstract application is nothing negotiated
>>>>>>>> (if I'm not mistaken).
>>>>>>>>
>>>>>>>> The key advantaan ge of the current behaviour is to avoid
>>>>>>>> "double-enabling" in testpmd. It preserves behaviour which
>>>>>>>> we had before before the API addition. It is a strong
>>>>>>>> argument. Basically it puts the feature into the same
>>>>>>>> basket as FAST_FREE - need an action to run faster.
>>>>>>>
>>>>>>> I think, there is a disconnect here. FAST_FREE is enabled by default.
>>>>
>>>> Sorry, I'm lost here. Don't we need to enable FAST_FREE
>>>> offload? As far as I know all offloads are disabled by default.
>>>
>>> Not the case for FAST_FREE as disabling needs "more cycles on processor" side.
>>>
>>> See app/test-pmd/testpmd.c
>>> /*
>>> * Ethernet device configuration.
>>> */
>>> struct rte_eth_rxmode rx_mode;
>>>
>>> struct rte_eth_txmode tx_mode = {
>>> .offloads = RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE,
>>> };
>>>
>>
>> Surprised, thanks. So, it one more difference of the testpmd
>> defaults from an abstract application.
>
> It is not, even l2fwd is same as testpmd. In my view, the ideal
> application just to select what is
> really needed.
>
>>
>>>>
>>>>>>> i.e We don't need any specific action to run faster. To align with performance
>>>>>>> test application, by default the configuration should be run faster. User
>>>>>>> needs to give explicit configuration to allow more offload or the one causes
>>>>>>> the mpps drops. IMO, That is the theme followed in testpmd.
>>>>>>>
>>>>>>>
>>>>>> I agree with Andrew, the default should stay the same, as now, PMD may already implement
>>>>>> logic to only enable the feature if there is a flow rule.
>>>>>> Changing the default will result in breaking applications.
>>>>>
>>>>> That's not what is discussed here.
>>>>> We are talking only about testpmd default.
>>>>>
>>>>>> I want to suggest new approach for this feature,
>>>>>> maybe we can use the rte_flow_configure, and add a new bit that says if those
>>>>>> actions are going to be used.
>>>>>> What do you think?
>>>>>
>>>>> Let's not change the API please.
>>>>>
>>>>>
>>>>>>>> I see no problem in such approach.
>>>>>>>>
>>>>>>>> The key disadvantage is the difference in testpmd and
>>>>>>>> other applications default behaviour.
>
> But none of the other applications in not negotiating by default.
>
>>>>>>>>
>>>>>>>> I'd look at the feature in the following way:
>>>>>>>> if an application theoretically wants to use
>>>>>>>> USER_FLAG, USER_MARK or TUNNEL_ID it must negotiate
>>>>>>>> corresponding Rx metadata to ensure that the feature is
>>>>>>>> available and HW is informed that application may need it.
>>>>>>>> Since testpmd supports corresponding flow API actions and
>>>>>>>> flow tunnels, it tries to negotiate it by default, but do
>>>>>>>> not fail if the negotiation fails.
>>>>>>>>
>>>>>>>> So, I'd would vote to keeping the default value as is.
>>
>> Two above paragraphs still stand.
>>
>> Frankly speaking I don't understand why default value is so
>> important if we have a way to change it. Reasons should be
>> really strong to change existing defaults.
>
> The only reason is, typically testpmd will be used performance
> benchmarking as an industry standard. It is difficult to tell/educate
> the QA or customers
> that, "BTW if you need to get better performance add more flag to
> testpmd command line".
> To make that worst, only some PMD needs to give the additional
> parameter to get better number.
> And also, testpmd usage will be treated as application modeling.
>
> Since this feature only used on sfc and cnxk driver, What is the
> situation with sfc driver?
> Keeping it as negotiated and not use the feature, will impact the per
> core performance of sfc or
> is it just PCI bandwidth thing which really dont show any difference in testpmd?
Yes, sfc could run faster if no Rx metadata are negotiated. So,
it is better to negotiate nothing by default. But it is always
painful to change defaults. You need to explain that now you
need to negotiate Rx metadata to use mark, flag and tunnel offloads.
Yes, it will be required on sfc and cnxk only.
As an sfc maintainer I don't mind to change testpmd defaults.
next prev parent reply other threads:[~2023-02-01 10:58 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-01 4:41 [PATCH] app/testpmd: add command line argument 'rx-metadata' Hanumanth Pothula
2022-08-01 13:11 ` Hanumanth Pothula
2022-08-01 13:13 ` Hanumanth Pothula
2022-08-01 19:41 ` Ivan Malov
2022-08-02 16:45 ` [PATCH] app/testpmd: add command line argument 'nic-to-pmd-rx-metadata' Hanumanth Pothula
2022-08-02 16:45 ` [PATCH v2 1/2] version: 22.11-rc0 Hanumanth Pothula
2022-08-02 16:45 ` [PATCH v2 2/2] app/testpmd: add command line argument 'nic-to-pmd-rx-metadata' Hanumanth Pothula
2022-08-02 17:51 ` [PATCH v2 1/1] " Hanumanth Pothula
2022-08-30 12:36 ` Hanumanth Reddy Pothula
2022-09-01 8:03 ` Singh, Aman Deep
2022-10-04 14:48 ` Andrew Rybchenko
2022-10-06 18:35 ` [PATCH v3 1/1] app/testpmd: control passing Rx metadata to PMD Hanumanth Pothula
2022-10-17 8:32 ` Andrew Rybchenko
2022-10-27 7:34 ` Thomas Monjalon
2022-10-27 12:54 ` Thomas Monjalon
2022-12-02 16:14 ` [EXT] " Hanumanth Reddy Pothula
2022-12-02 19:41 ` Thomas Monjalon
2022-12-05 7:59 ` Hanumanth Reddy Pothula
2022-12-05 8:28 ` Thomas Monjalon
2022-12-05 9:43 ` Slava Ovsiienko
2022-12-20 20:02 ` [PATCH v4 1/2] ethdev: control Rx metadata negotiation Hanumanth Pothula
2022-12-20 20:02 ` [PATCH v4 2/2] app/testpmd: add command to process " Hanumanth Pothula
2022-12-20 21:23 ` Stephen Hemminger
2022-12-21 2:07 ` [PATCH v5 1/2] ethdev: fix ethdev configuration state on reset Hanumanth Pothula
2022-12-21 2:07 ` [PATCH v5 2/2] app/testpmd: add command to process Rx metadata negotiation Hanumanth Pothula
2023-01-18 10:32 ` Thomas Monjalon
2023-01-19 10:33 ` [EXT] " Hanumanth Reddy Pothula
2023-01-25 12:51 ` Thomas Monjalon
2023-01-24 18:04 ` Ferruh Yigit
2023-01-25 9:30 ` [EXT] " Hanumanth Reddy Pothula
2023-01-25 12:55 ` Thomas Monjalon
2023-01-25 13:55 ` Ferruh Yigit
2023-01-25 13:59 ` Thomas Monjalon
2023-01-25 14:42 ` Nithin Kumar Dabilpuram
2023-01-26 11:03 ` Thomas Monjalon
2023-01-27 5:02 ` Nithin Kumar Dabilpuram
2023-01-27 8:54 ` Thomas Monjalon
2023-01-27 10:42 ` Nithin Kumar Dabilpuram
2023-01-27 15:01 ` Thomas Monjalon
2023-01-31 16:17 ` Jerin Jacob
2023-01-31 23:03 ` Thomas Monjalon
2023-02-01 6:10 ` Ivan Malov
2023-02-01 7:16 ` Andrew Rybchenko
2023-02-01 8:53 ` Jerin Jacob
2023-02-01 9:00 ` Ori Kam
2023-02-01 9:05 ` Thomas Monjalon
2023-02-01 9:07 ` Andrew Rybchenko
2023-02-01 9:14 ` Jerin Jacob
2023-02-01 9:29 ` Andrew Rybchenko
2023-02-01 10:48 ` Jerin Jacob
2023-02-01 10:58 ` Andrew Rybchenko [this message]
2023-02-01 11:04 ` Thomas Monjalon
2023-02-01 11:15 ` Jerin Jacob
2023-02-01 11:35 ` Ferruh Yigit
2023-02-01 13:48 ` Jerin Jacob
2023-02-01 14:50 ` Ferruh Yigit
2023-02-01 15:22 ` Jerin Jacob
2023-02-02 8:43 ` Ferruh Yigit
2023-02-02 8:50 ` Ivan Malov
2023-02-02 9:17 ` Ferruh Yigit
2023-02-02 10:41 ` Ivan Malov
2023-02-02 10:48 ` Ivan Malov
2023-02-02 11:41 ` Thomas Monjalon
2023-02-02 11:55 ` Ivan Malov
2023-02-02 12:03 ` Thomas Monjalon
2023-02-02 12:21 ` Andrew Rybchenko
2023-02-01 11:20 ` Ivan Malov
2023-01-25 13:17 ` Ferruh Yigit
2023-01-25 13:21 ` Ferruh Yigit
2023-01-25 13:21 ` Ferruh Yigit
2023-01-16 10:43 ` [PATCH v5 1/2] ethdev: fix ethdev configuration state on reset Hanumanth Reddy Pothula
2023-01-18 10:29 ` Thomas Monjalon
2023-01-24 18:14 ` Ferruh Yigit
Reply instructions:
You may reply publicly 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=98a80c20-a5e4-deea-f7dc-c6aa5d52800b@oktetlabs.ru \
--to=andrew.rybchenko@oktetlabs.ru \
--cc=aman.deep.singh@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=hpothula@marvell.com \
--cc=ivan.malov@arknetworks.am \
--cc=ivan.malov@oktetlabs.ru \
--cc=jerinj@marvell.com \
--cc=jerinjacobk@gmail.com \
--cc=ndabilpuram@marvell.com \
--cc=orika@nvidia.com \
--cc=thomas@monjalon.net \
--cc=viacheslavo@nvidia.com \
--cc=yuying.zhang@intel.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).