Re: [dpdk-dev] [PATCH v2 3/3] ethdev: improve flow mark Rx offload deprecation notice

[Thomas Monjalon <thomas@monjalon.net>]

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

> > And no, it is not postponed by one year.
> > Next release can implement a new API.
> >
> >> I disagree that it is tightly related to moving
> >> mark/flag to
> >> dynamic field/flag and absolutely blocked by it. Yes, I know that the are
> >> concerns from the very beginning, but the problem is explained [2] and clear
> >> and no full-featured alternative solution is suggested. Solution suggested
> >> by Ori has many significant drawbacks as explained in [2] and highlighted
> >> in further discussion.
> > 
> > I disagree with working only on mark action while there are a lot
> > of other configs which have to be implemented in drivers.
> >
> > The reality is that some drivers decided to have some "optimizations"
> > disabling some features, and you want the application to opt-in
> > in order to allow your optimized paths.
> 
> Strictly speaking it is not about driver optimized paths only, but HW
> configuration as well which can be done on start-up only (not dynamic) and
> could be per-queue in fact.

OK good point, we can optimize both driver and hardware configuration
before enabling a queue.
Note all these threads are long but one of the benefits
is to get the definition of the need, which was lacking.

> > Note that opt-in is different of really enabling an offload.
> > For some basic port-level features like RSS hash,
> > it is enabled with an offload flag before starting the port,
> > acting as an opt-in.
> 
> Could you highlight the difference between opt-in and offload.
> What is the key difference which makes one solution better
> than another? Why different mechanism is required?

Configuring a feature means providing all infos to make
the processing effective.
Opt-in a feature means asking for a processing to be available
when it will be configured later.
Configuration implies opt-in of course.

For now, we have only configuration APIs, no opt-in.

The need you want to address is to opt-in for a feature
before enabling a queue, and configure it later.

> > Some features have some dedicated API, which may be enabled after
> > starting the port, and no way to opt-in (or opt-out) before start.
> 
> It sounds like you have examples in your mind. Please, share.

All rte_flow examples are some examples of configuration API
which can be done after start, without a way to opt-in in advance.
Other examples of APIs not clearly forbidden to use after start:
	- rte_eth_dev_set_mtu()
	- rte_eth_dev_vlan_filter()
	- rte_eth_dev_rss_reta_update()
	- rte_eth_mirror_rule_set()
	- rte_eth_dev_udp_tunnel_port_add()
	- rte_eth_dev_l2_tunnel_offload_set
	- rte_eth_timesync_enable()

> > A lot of features are using rte_flow API which is in this situation.
> > If we take the opt-in path, let's not do it only for the mark action,
> > but let's create a real API for it:
> > 	rte_eth_dev_optin()
> > 	rte_eth_dev_optinall()
> > 	rte_eth_dev_optoutl()
> 
> 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)?

And more importantly, again, it should be done for all features at once,
not only for the rte_flow mark.

> > I think the motivation is strong enough.
> >
> >> [1] http://inbox.dpdk.org/dev/1573203631946.15959@kth.se/
> >> [2]
> >> http://inbox.dpdk.org/dev/f170105b-9c60-1b04-cb18-52e0951ddcdb@solarflare.com/
> >>
> >>> If something would be implemented in 20.02,
> >>> it must be a new and optional API.
> >> 
> >> Flow mark and flag may work without the offload with some drivers,
> >> but some drivers require the offload to make it work. Flow API error
> >> should contain message which says that the offload is disabled and
> >> must be enabled.
> > 
> > Yes, the PMD should return an explicit error about a feature being disabled.
> > How does it impact ethdev API?
> 
> It is still the offload discussed in the deprecation notice.
> The solution is far from ideal, since allows the difference in PMDs
> behaviour and an application debugged on one PMD may not
> work using another PMD (unfortunately it is true in any case, but
> such definition makes it 100% legal).

Do you mean PMDs have different capabilities and optimizations?
I think I don't get your point.

> >>> That's why I think no deprecation notice is required.
> >>>
> >>> [...]
> >>>> +* ethdev: New offload flag ``DEV_RX_OFFLOAD_FLOW_MARK`` will be added in 20.02.
> >>>> +  This will provide application an information if ``RTE_FLOW_ACTION_TYPE_MARK``
> >>>> +  or ``RTE_FLOW_ACTION_TYPE_FLAG`` is supported and, what is more important,
> >>>> +  allow an application to let PMD know that it would like to use these
> >>>> +  features.
> >>>> +  PMD may use the information to choose optimal datapath implementation and
> >>>> +  configure HW appropriately to optimize performance and/or resources usage.