From: Thomas Monjalon <firstname.lastname@example.org> To: Jerin Jacob <email@example.com> Cc: Andrew Rybchenko <firstname.lastname@example.org>, Ferruh Yigit <email@example.com>, Pavan Nikhilesh <firstname.lastname@example.org>, Neil Horman <email@example.com>, John McNamara <firstname.lastname@example.org>, Marko Kovacevic <email@example.com>, dpdk-dev <firstname.lastname@example.org>, Ori Kam <email@example.com>, David Marchand <firstname.lastname@example.org>, Olivier Matz <email@example.com>, "Ananyev, Konstantin" <firstname.lastname@example.org> 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 <email@example.com> wrote: > > > > 23/11/2019 10:42, Jerin Jacob: > > > On Sat, Nov 23, 2019 at 3:58 AM Thomas Monjalon <firstname.lastname@example.org> 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  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?
next prev parent 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 \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.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 \ firstname.lastname@example.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