DPDK patches and discussions
 help / color / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Jerin Jacob <jerinjacobk@gmail.com>
Cc: Andrew Rybchenko <arybchenko@solarflare.com>,
	Ferruh Yigit <ferruh.yigit@intel.com>,
	Pavan Nikhilesh <pbhagavatula@marvell.com>,
	Neil Horman <nhorman@tuxdriver.com>,
	John McNamara <john.mcnamara@intel.com>,
	Marko Kovacevic <marko.kovacevic@intel.com>,
	dpdk-dev <dev@dpdk.org>, Ori Kam <orika@mellanox.com>,
	David Marchand <david.marchand@redhat.com>,
	Olivier Matz <olivier.matz@6wind.com>,
	"Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Subject: Re: [dpdk-dev] [PATCH v2 3/3] ethdev: improve flow mark Rx offload deprecation notice
Date: Mon, 25 Nov 2019 12:39:52 +0100
Message-ID: <1788171.neaCWyZYis@xps> (raw)
In-Reply-To: <CALBAE1PYQkceo1_41reQSS1mT8p7Q06pZ4TJ-e3gpCqNo=RnXA@mail.gmail.com>

25/11/2019 11:44, Jerin Jacob:
> On Sun, Nov 24, 2019 at 3:12 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 23/11/2019 10:42, Jerin Jacob:
> > > On Sat, Nov 23, 2019 at 3:58 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > 22/11/2019 12:53, Andrew Rybchenko:
> > > > > On 11/22/19 2:15 PM, Thomas Monjalon wrote:
> > > > > > 22/11/2019 11:12, Andrew Rybchenko:
> > > > > >> On 11/22/19 1:01 AM, Thomas Monjalon wrote:
> > > > > >>> 19/11/2019 13:12, Andrew Rybchenko:
> > > > > >>>> The deprecation notice is required since it adds more requirements
> > > > > >>>> when RTE flow mark and flag actions may be used and require
> > > > > >>>> changes in applications.
> > > > > >>> I am still not sure what is the best solution here.
> > > > > >>> I continued to think about it in this thread:
> > > > > >>>   http://mails.dpdk.org/archives/dev/2019-November/151960.html
> > > > > >>>
> > > > > >>> I think we cannot require any application change until 20.11
> > > > > >>> in order to keep API (and behaviour) compatibility.
> > > > > >> Expected, but still very disappointing.
> > > > > >>
> > > > > >> The feature is implemented by Pavan (@ Marvell), supported by me,
> > > > > >> used by Qi (@ Intel), looks better than alternatives from application
> > > > > >> developer point of view [1] and finally postponed for 1 year without really
> > > > > >> strong motivation.
> > > > > >
> > > > > > I see different valuable point of views. This is enough motivation.
> > > > >
> > > > > It looks like I miss it in previous discussion, I would be thankful if
> > > > > you give me links to read or hints how to find.
> > > >
> > > > http://mails.dpdk.org/archives/dev/2019-November/150793.html
> > > >
> > > > > Introducing new types of controls would make configuration more and
> > > > > more complex. I think that many different types of control would
> > > > > over-complicate it. May be it is unavoidable, but it should be clear
> > > > > why the problem cannot be solved using existing types of controls
> > > > > (e.g. offloads).
> > > >
> > > > The offload control is used as an effective configuration for now.
> > > > The features which are configured with DEV_RX_OFFLOAD_*
> > > > do not need any other API to be used.
> > > > Extending DEV_RX_OFFLOAD_* bits for enabling features which
> > > > must be configured via other API anyway, is possible.
> > > > The real problem is that features in DEV_RX_OFFLOAD_* are supposed
> > > > to be disabled by default. If we add some opt-in features here,
> > > > we cannot enable them by default for API compatibility and do the
> > > > right thing by default.
> > > >
> > > > Choosing DEV_RX_OFFLOAD_* bits or rte_eth_dev_opt* functions is a detail.
> > > > The real decision is to change the API for using all these features.
> > > > Can we keep all features available by default (opt-out)?
> > >
> > > IMO, *rte_eth_dev_opt* has following problems
> > >
> > > 1) It is not multi-process friendly. If we are changing the Rx/Tx
> > > function pointer, based on
> > > the selected offload, then, using *rte_eth_dev_opt* scheme won't
> > > really work(if the new API
> > > called after the secondary process launch)
> >
> > Yes it must be used before launching the secondary process.
> > It is the same as DEV_RX_OFFLOAD_* config.
> 
> Yes. rte_eth_dev_opt_* has another dimension to enable and disable as API.
> So, we need to document, opt-in -> start() -> opt-out case won't work
> in multi process
> case.
> 
> >
> > > 2) If we are taking rte_eth_dev_opt path then by default feature has
> > > to be enabled to
> > > not break the functional ABI. That scheme won't scale if as when we
> > > keep adding the new features.
> > > It is always easy for the application to define "what it wants" vs
> > > "what it does not want"
> >
> > Yes, opt-in may look more natural than opt-out.
> > But opt-in makes the default more complex, and changes the API.
> >
> > > 3) Defining the device state after the reconfigure operation.
> > >
> > > IMO, if any operation is connected to fastpath it is better to use
> > > DEV_RX_OFFLOAD_ like
> > > this feature where enable or disable PMDs from updating
> > > ``rte_mbuf::hash::fdir`` so that if possible
> > > we can use different Rx function pointer if possible(Hence it can work
> > > with the multi-process case case)
> >
> > I reply to 2 and 3 together.
> >
> > We decided that offloads must be disabled by default.
> > This is what we have in 19.11:
> >         - Some offloads are enabled before start with DEV_?X_OFFLOAD_*
> >         - Some offloads are enabled with functions at any time
> >
> > For the second type of offloads, you want to know, before start,
> > whether it will be used or not.
> > If adding the second type of offloads (like rte_flow ones)
> > to DEV_?X_OFFLOAD_*, it means it must be enabled 2 times:
> >         - before start with offload bits
> >         - later with more precise functions
> >
> > I would like to avoid changing the default behaviour,
> > which is to enable an offload only one time.
> > That's why I think this second category of offloads should
> > offer opt-out (global disabling), so it will continue
> > to work by default if they are configured.
> >
> > I hope you understand the difference between the two categories.
> 
> I understand the difference. The only point of "difference in opinion" is
> the default behavior of the feature/offload. If it is in RX_OFFLOAD scheme then
> by default it is disabled. opt_* scheme makes this new feature/offload
> enabled default to avoid changing the default behavior.

OK, this is where we disagree.
I am for keeping what we agreed this year: all offloads are disabled by default.
But I am against the need for double enablement.
The offloads which are enabled with a specific function should not need
to be also enabled (opt-in) before start.

> It is good to avoid functional ABI change. But bad as,
> 1) New API starts bloating the ethdev API.

In general, I want to clean-up the ethdev API during next year.

> 2) It is diffcult for application guys to figure out what are features need to
> be disabled to performance as he/she does not know, for the given release,
> the enabled features.

Yes this is a good point.

> Item (1) is a trade-off between elegance vs ABI compatibility. No
> strong opinion on this.
> 
> To fix the item (2), Can we get have an API in ethdev to get enabled
> features so that
> the application can probe and disable if required?

We can think about something like that.
Note that there is also a need to better advertise all capabilities.

> For example, rte_eth_dev_set_ptypes() comes in same category, By default,
> ptype parsing is enabled. I think, we can have a general interface to
> "probe" the by default enabled features
> and disable it if required. Not scattered API for each feature.

This is an issue. The packet type parsing should be disabled by default.

> The above scheme fixe my concerns.
> 
> Thoughts?




  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-19 12:05 [dpdk-dev] [PATCH 1/3] ethdev: remove deprecation notice for packet type set Andrew Rybchenko
2019-11-19 12:05 ` [dpdk-dev] [PATCH 2/3] ethdev: remove deprecation notice about RSS hash offload Andrew Rybchenko
2019-11-19 12:05 ` [dpdk-dev] [PATCH 3/3] ethdev: improve flow mark Rx offload deprecation notice Andrew Rybchenko
2019-11-19 12:12 ` [dpdk-dev] [PATCH v2 1/3] ethdev: remove deprecation notice for packet type set Andrew Rybchenko
2019-11-19 12:12   ` [dpdk-dev] [PATCH v2 2/3] ethdev: remove deprecation notice about RSS hash offload Andrew Rybchenko
2019-11-19 15:04     ` Ferruh Yigit
2019-11-25 16:38       ` Ferruh Yigit
2019-11-19 12:12   ` [dpdk-dev] [PATCH v2 3/3] ethdev: improve flow mark Rx offload deprecation notice Andrew Rybchenko
2019-11-21 22:01     ` Thomas Monjalon
2019-11-22 10:12       ` Andrew Rybchenko
2019-11-22 11:15         ` Thomas Monjalon
2019-11-22 11:53           ` Andrew Rybchenko
2019-11-22 13:32             ` Jerin Jacob
2019-11-22 18:58             ` Thomas Monjalon
2019-11-23  9:42               ` Jerin Jacob
2019-11-23 18:12                 ` Thomas Monjalon
2019-11-25 10:44                   ` Jerin Jacob
2019-11-25 11:39                     ` Thomas Monjalon [this message]
2019-12-02  4:21                       ` Jerin Jacob
2019-12-02  9:15                         ` Thomas Monjalon
2019-12-02 11:09                           ` Jerin Jacob
2019-12-02 11:57                             ` Andrew Rybchenko
2019-12-05  8:12                               ` Jerin Jacob
2019-12-09  9:17                                 ` Andrew Rybchenko
2019-12-16  7:38                                   ` Jerin Jacob
2019-12-16 10:02                                     ` Andrew Rybchenko
2019-11-19 15:04   ` [dpdk-dev] [PATCH v2 1/3] ethdev: remove deprecation notice for packet type set Ferruh Yigit
2019-11-25 16:38     ` Ferruh Yigit

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=1788171.neaCWyZYis@xps \
    --to=thomas@monjalon.net \
    --cc=arybchenko@solarflare.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jerinjacobk@gmail.com \
    --cc=john.mcnamara@intel.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=nhorman@tuxdriver.com \
    --cc=olivier.matz@6wind.com \
    --cc=orika@mellanox.com \
    --cc=pbhagavatula@marvell.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

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 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 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


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