On 04/10/2018 07:36 PM, Adrien Mazarguil wrote:
TPID handling in rte_flow VLAN and E_TAG pattern item definitions is not
consistent with the normal stacking order of pattern items, which is
confusing to applications.

Problem is that when followed by one of these layers, the EtherType field
of the preceding layer keeps its "inner" definition, and the "outer" TPID
is provided by the subsequent layer, the reverse of how a packet looks like
on the wire:

  Wire:     [ ETH TPID = A | VLAN EtherType = B | B DATA ]
  rte_flow: [ ETH EtherType = B | VLAN TPID = A | B DATA ]

Worse, when QinQ is involved, the stacking order of VLAN layers is
unspecified. It is unclear whether it should be reversed (innermost to
outermost) as well given TPID applies to the previous layer:

  Wire:       [ ETH TPID = A | VLAN TPID = B | VLAN EtherType = C | C DATA ]
  rte_flow 1: [ ETH EtherType = C | VLAN TPID = B | VLAN TPID = A | C DATA ]
  rte_flow 2: [ ETH EtherType = C | VLAN TPID = A | VLAN TPID = B | C DATA ]

While specifying EtherType/TPID is hopefully rarely necessary, the stacking
order in case of QinQ and the lack of documentation remain an issue.

This patch replaces TPID in the VLAN pattern item with an inner
EtherType/TPID as is usually done everywhere else (e.g. struct vlan_hdr),
clarifies documentation and updates all relevant code.

It breaks ABI compatibility for the following public functions:

- rte_flow_copy()
- rte_flow_create()
- rte_flow_query()
- rte_flow_validate()

Summary of changes for PMDs that implement ETH, VLAN or E_TAG pattern

- bnxt: EtherType matching is supported with and without VLAN, but TPID
   matching is not and triggers an error.

- e1000: EtherType matching is only supported with the ETHERTYPE filter,
   which does not support VLAN matching, therefore no impact.

- enic: same as bnxt.

- i40e: same as bnxt with existing FDIR limitations on allowed EtherType
   values. The remaining filter types (VXLAN, NVGRE, QINQ) do not support
   EtherType matching.

- ixgbe: same as e1000, with additional minor change to rely on the new
   E-Tag macro definition.

- mlx4: EtherType/TPID matching is not supported, no impact.

- mlx5: same as bnxt.

- mvpp2: same as bnxt.

- sfc: same as bnxt.

- tap: same as bnxt.

Signed-off-by: Adrien Mazarguil <adrien.mazarg...@6wind.com>
Cc: Ferruh Yigit <ferruh.yi...@intel.com>
Cc: Thomas Monjalon <tho...@monjalon.net>
Cc: Wenzhuo Lu <wenzhuo...@intel.com>
Cc: Jingjing Wu <jingjing...@intel.com>
Cc: Ajit Khaparde <ajit.khapa...@broadcom.com>
Cc: Somnath Kotur <somnath.ko...@broadcom.com>
Cc: John Daley <johnd...@cisco.com>
Cc: Hyong Youb Kim <hyon...@cisco.com>
Cc: Beilei Xing <beilei.x...@intel.com>
Cc: Qi Zhang <qi.z.zh...@intel.com>
Cc: Konstantin Ananyev <konstantin.anan...@intel.com>
Cc: Nelio Laranjeiro <nelio.laranje...@6wind.com>
Cc: Yongseok Koh <ys...@mellanox.com>
Cc: Tomasz Duszynski <t...@semihalf.com>
Cc: Dmitri Epshtein <d...@marvell.com>
Cc: Natalie Samsonov <nsams...@marvell.com>
Cc: Jianbo Liu <jianbo....@arm.com>
Cc: Andrew Rybchenko <arybche...@solarflare.com>
Cc: Pascal Mazon <pascal.ma...@6wind.com>


v3 changes:

Updated mrvl to mvpp2.

Moved unrelated default TCI mask update to separate patch.

Fixed sfc according to Andrew's comments [1], which made so much sense that
I standardized on the same behavior for all other PMDs: matching outer TPID
is never supported when a VLAN pattern item is present.

This is done because many devices accept several TPIDs but do not provide
means to match a given one explicitly, it's all or nothing, and that makes
the resulting flow rule inaccurate.

[1] http://dpdk.org/ml/archives/dev/2018-April/095870.html
  app/test-pmd/cmdline_flow.c                 | 17 +++----
  doc/guides/nics/tap.rst                     |  2 +-
  doc/guides/prog_guide/rte_flow.rst          | 19 ++++++--
  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 +-
  drivers/net/bnxt/bnxt_filter.c              | 35 +++++++++++---
  drivers/net/enic/enic_flow.c                | 19 +++++---
  drivers/net/i40e/i40e_flow.c                | 60 ++++++++++++++++++++----
  drivers/net/ixgbe/ixgbe_ethdev.c            |  3 +-
  drivers/net/mlx5/mlx5_flow.c                | 13 ++++-
  drivers/net/mvpp2/mrvl_flow.c               | 26 +++++++---
  drivers/net/sfc/sfc_flow.c                  | 18 +++++++
  drivers/net/tap/tap_flow.c                  | 14 ++++--
  lib/librte_ether/rte_flow.h                 | 22 ++++++---
  lib/librte_net/rte_ether.h                  |  1 +
  14 files changed, 198 insertions(+), 55 deletions(-)

Generic and net/sfc
Acked-by: Andrew Rybchenko <arybche...@solarflare.com>

Reply via email to