From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 55846A04B5; Mon, 2 Dec 2019 12:09:29 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7DCCA2BD8; Mon, 2 Dec 2019 12:09:28 +0100 (CET) Received: from mail-io1-f48.google.com (mail-io1-f48.google.com [209.85.166.48]) by dpdk.org (Postfix) with ESMTP id DBD311F28 for ; Mon, 2 Dec 2019 12:09:26 +0100 (CET) Received: by mail-io1-f48.google.com with SMTP id z23so2981847iog.11 for ; Mon, 02 Dec 2019 03:09:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=7EoKS7HGDbFG1gdRudV1LqZIsQbP+zHXXOq6QWlJxLI=; b=ebeK4CDNsituBDW9m1blokuf/OgNAk7icNO4+AvNr+81e4/qwd9/Gtejwb0r/pr7/C vPKDBWsa/APy9OlqK2GJ5B+Vn0YeZlUcW88QavE+vCRknMYhm6CP7OT2zocOlORGYwGs lWJ1Fb1HRrxNb37nayQXyMR8dBkj0EfySYZlsGgYntZTqqiZ2W4u6ZcBsqO4qMk8kz+x TjxiKgCDEV92ywFi/ObDli/Fxjg5Nh+lD5V+5FetyrGfl8OCem4oMDHU60mUG64WYlXE LmtdKbb1nkxffCNLFdSsjH8WVcXREMxFa4B25tP7j+GxEEiOTSrX1bjTS4S84NrEF6aQ QBhA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=7EoKS7HGDbFG1gdRudV1LqZIsQbP+zHXXOq6QWlJxLI=; b=ii88zlcr4pVAFYsG/abId3R6spQn7OcdtXiCvNY0+WD2du4EetN1xnJipuMMmHKmGx +ZYnO2FZpKdb0mnHoAAuMeEG8W7j4XksK77bJof+XqF0vDVXmWhOQhcLNNUg5WmqJY85 mMrKPKAreb850CfxZ8mMbrpBTp21yzo3LXWXWbHWb3oQf64wbYI7KxvSLFx0Ac762xQD iS0JFDHvq5BjlWfX4gJ+Q+EcwMSx8XDsiBYCvJCkl4K7lhBWJkmFfrnJObregWxFTAVL FddAVZn3Zcww5BBP2PX1eEeretjVNoI74vlymQ/jQFgqsNL3/Cbe9BoslD1gsN6n4mo0 fLcw== X-Gm-Message-State: APjAAAUsPe+/CmQpQhq0+D8W5blycutvwXkXN9/9Jy2MDHjyxAeiuguv 7Q7927E32Go7iv9tVVUT+avok2TmXK2B8SDMvuo= X-Google-Smtp-Source: APXvYqyG6Tf2d03d68yr57BCsNeSJTsr6uBiCfuvVKO8cBBq7f97zLGzOg2rvxanOGLaBbqzz6QtR3nR7dnxpvWO1zo= X-Received: by 2002:a5e:8516:: with SMTP id i22mr46232296ioj.130.1575284965903; Mon, 02 Dec 2019 03:09:25 -0800 (PST) MIME-Version: 1.0 References: <1574165145-23960-1-git-send-email-arybchenko@solarflare.com> <1788171.neaCWyZYis@xps> <1832509.BOePYM8p3J@xps> In-Reply-To: <1832509.BOePYM8p3J@xps> From: Jerin Jacob Date: Mon, 2 Dec 2019 20:09:09 +0900 Message-ID: To: Thomas Monjalon Cc: Andrew Rybchenko , Ferruh Yigit , Pavan Nikhilesh , Neil Horman , John McNamara , Marko Kovacevic , dpdk-dev , Ori Kam , David Marchand , Olivier Matz , "Ananyev, Konstantin" Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v2 3/3] ethdev: improve flow mark Rx offload deprecation notice X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Dec 2, 2019 at 6:16 PM Thomas Monjalon wrote: > > 02/12/2019 05:21, Jerin Jacob: > > On Mon, Nov 25, 2019 at 8:39 PM Thomas Monjalon wrote: > > > > > > 25/11/2019 11:44, Jerin Jacob: > > > > On Sun, Nov 24, 2019 at 3:12 AM Thomas Monjalon wrote: > > > > > > > > > > 23/11/2019 10:42, Jerin Jacob: > > > > > > On Sat, Nov 23, 2019 at 3:58 AM Thomas Monjalon 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. > > > > OK. > > > > > > > > > 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. > > > > IMO, It makes sense to disable by default. > > > > Isn't conflicting? One thread, we are saying for in order to make, > > existing application work without breaking ABI, Default should be > > enabled. > > > > Thoughts? > > Every offloads should be disabled by default. > This is a good reason to break the behaviour in 20.11. Ack. > > > And what would be DEFAULT behavior for the mbuf MARK updation feature? > > (That's where this thread started). > > As all other features, mark is disabled by default. > Using a rte_flow rule, it can be enabled. > No need to pre-enable it. Ok. > >