Re: [dpdk-dev] [PATCH v4 01/16] ethdev: add error types to flow API

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Wed, Apr 18, 2018 at 10:24:10AM +0100, Ferruh Yigit wrote:
> On 4/18/2018 9:41 AM, Adrien Mazarguil wrote:
> > On Tue, Apr 17, 2018 at 08:37:31PM +0100, Ferruh Yigit wrote:
> >> On 4/16/2018 5:22 PM, Adrien Mazarguil wrote:
> >>> These enable more precise reporting of objects responsible for errors.
> >>>
> >>> This breaks ABI compatibility for the following public functions:
> >>>
> >>> - rte_flow_create()
> >>> - rte_flow_destroy()
> >>> - rte_flow_error_set()
> >>> - rte_flow_flush()
> >>> - rte_flow_isolate()
> >>> - rte_flow_query()
> >>> - rte_flow_validate()
> >>
> >> Is there a deprecation notice for this API break?
> > 
> > A notice covering the main changes in this series (most patches have an ABI
> > impact) was sent but not included [1]. This particular patch rides on the
> > announced ABI breakage in order to add a relatively minor feature while
> > there.
> 
> My take from "announced ABI breakage" is the deprecation notice get three acks
> and merged into release, so it seems there is no deprecation notice and
> according process first deprecation notice should go in this release.

True, so just describe how ABI impact is no worse than what was announced
(and included) [3] and most of them are actually opportune changes to
improve consistency and documentation since ABI would be broken for this
release regardless. Regarding individual patches:

- 01/16 "ethdev: add error types to flow API"
  => New error types are added in the middle of an existing enum.

- 02/16 "ethdev: clarify flow API pattern items and actions"
  => No impact.

- 03/16 "doc: remove flow API migration section"
  => No impact.

- 04/16 "ethdev: remove DUP action from flow API"
  => An action that no PMD supports is removed from an existing enum.

- 05/16 "ethdev: alter behavior of flow API actions"
  => A documentation change in how actions are processed logically
     breaks ABI in the case of repeated actions (currently a corner
     case). Most PMDs do not implement the original behavior correctly
     anyway (see commit log).

- 06/16 "ethdev: remove C99 flexible arrays from flow API"
  => ABI impact is primarily triggered by the RSS action change (already
     covered [3]). The RAW pattern item is also modified for consistency.
     On the API side, except when allocating these structures, there is no
     difference in usage (i.e. array[index] => pointer[index]).

- 07/16 "ethdev: flatten RSS configuration in flow API"
  => Already covered [3].

- 08/16 "ethdev: add hash function to RSS flow API action"
  => Already covered [3].

- 09/16 "ethdev: add encap level to RSS flow API action"
  => Already covered [3].

- 10/16 "ethdev: refine TPID handling in flow API"
  => No PMD supports the poorly defined TPID matching, applications couldn't
     possibly rely on it.

- 11/16 "ethdev: limit default VLAN TCI mask in flow API"
  => No ABI breakage, but a different behavior for applications that rely on
     the default mask. It doesn't look like any PMD supports PCP/DEI
     matching so again applications could not rely on it (they still can do
     it by providing a specific mask).

- 12/16 "ethdev: add transfer attribute to flow API"
  => Minor ABI impact (read: logical) due to the addition of a bit in an
     existing bit-field. No practical impact on applications.

- 13/16 "ethdev: update behavior of VF/PF in flow API"
  => Documentation (API) change. The "transfer" bit must now be set in order to
     use these actions with PMDs that support them.

- 14/16 "ethdev: rename physical port item in flow API"
  => API change for a pattern item supported by no PMD.

- 15/16 "ethdev: add physical port action to flow API"
  => New action added in the middle of an existing enum.

- 16/16 "ethdev: add port ID item and action to flow API"
  => New item/action added in the middle of existing enums.

> Hi Thomas,
> 
> Any comment on issue?
> 
> > 
> > This ABI change was implicitly needed by upcoming work for 18.05 (Xueming's
> > RSS stuff [2][3], Declan's TEP [4], the rest is summarized by a RFC [5]) due
> > to the necessary changes of behavior in flow rules.
> > 
> > Note that Xueming's deprecation notice [3] alone would have triggered such
> > an ABI change because struct rte_flow_action_rss wouldn't have been binary 
> > compatible if struct rte_eth_rss_conf was updated. This change would have
> > propagated back to rte_flow functions manipulating them.
> 
> To be honest I lost track of Xueming's patches, because of split/merge of
> patchset, multiple set with multiple versions out.
> 
> Is it possible to document the dependency graph including your set?

I hopefully didn't miss any:

 Bunch of flow API-related fixes (v5) [6]
 |
 `-- Flow API overhaul for switch offloads (v4) [7]
     |
     +-- additions to support tunnel encap/decap (v4) [8]
     |
     +-- introduce new tunnel types (v5) [9]
     |   |
     |   `-- mlx5 Rx tunnel offloading (v4) [10]
     |
     +-- rte_flow extension for vSwitch acceleration (v3.2) [11]
     |
     `-- net/sfc: RSS improvements [12]

[6] http://dpdk.org/ml/archives/dev/2018-April/097411.html
[7] http://dpdk.org/ml/archives/dev/2018-April/097423.html
[8] http://dpdk.org/ml/archives/dev/2018-April/097956.html
[9] http://dpdk.org/ml/archives/dev/2018-April/097669.html
[10] http://dpdk.org/ml/archives/dev/2018-April/097673.html
[11] http://dpdk.org/ml/archives/dev/2018-April/097266.html
[12] http://dpdk.org/ml/archives/dev/2018-April/095872.html

I gave the current version for these series but some of them are still under
review and may change. I plan to send updates for [6] and [7] shortly.

> > [1] "doc: announce API changes for flow rules"
> >      http://dpdk.org/ml/archives/dev/2018-February/090988.html
> > [2] "MLX5 tunnel Rx offloading"
> >     http://dpdk.org/ml/archives/dev/2018-February/091461.html
> > [3] "doc: annouce ABI change for RSS configuraiton structure"
> >     http://dpdk.org/ml/archives/dev/2018-February/090127.html
> > [4] "tunnel endpoint hw acceleration enablement"
> >     http://dpdk.org/ml/archives/dev/2017-December/084676.html
> > [5] "Switch device offload with DPDK"
> >     http://dpdk.org/ml/archives/dev/2018-March/092513.html
> > 
> >>> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> >>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >>
> >> <...>
> >>
> > 
> 

-- 
Adrien Mazarguil
6WIND