Re: [PATCH] doc: fix support table for ETH and VLAN flow items

[Ilya Maximets <i.maximets@ovn.org>]

On 9/12/22 12:09, Thomas Monjalon wrote:
> 16/03/2022 13:01, Ilya Maximets:
>> 'has_vlan' attribute is only supported by sfc, mlx5 and cnxk.
>> Other drivers doesn't support it.  Most of them (like i40e) just
>> ignore it silently.  Some drivers (like mlx4) never had a full
>> support of the eth item even before introduction of 'has_vlan'
>> (mlx4 allows to match on the destination MAC only).
>>
>> Same for the 'has_more_vlan' flag of the vlan item.
>>
>> Changing the support level to 'partial' for all such drivers.
>> This doesn't solve the issue, but at least marks the problematic
>> drivers.
> 
> You changed "eth" and "vlan" from "Y" to "P".
> The field "has_vlan" is part of "rte_flow_item_eth",
> and "has_more_vlan" is part of "rte_flow_item_vlan",
> so I agree we need to change both items to "partial support".
> It looks to be a good change, just needs to more explicit,
> adding this kind of explanation about the fields.

I can add this to the commit message.  Should I also add
some note alongside the table in that documentation page?

> 
> We missed this patch in 22.07, let's have some progress quickly.

It was a busy month + PTO, sorry I didn't get to that patch faster.

> 
>> Some details are available in:
>>   https://bugs.dpdk.org/show_bug.cgi?id=958
> 
> About rte_flow_item_eth.{src,dst}, I don't find a deprecation notice
> about it in the history of the file doc/guides/rel_notes/deprecation.rst

It took some time to find the deprecation notice, but I did find it
in the end.  It's in the doc/guides/rel_notes/deprecation.rst, and
it says:

* ethdev: The flow API matching pattern structures, ``struct rte_flow_item_*``,
  should start with relevant protocol header.
  Some matching pattern structures implements this by duplicating protocol header
  fields in the struct. To clarify the intention and to be sure protocol header
  is intact, will replace those fields with relevant protocol header struct.
  In v21.02 both individual protocol header fields and the protocol header struct
  will be added as union, target is switch usage to the protocol header by time.
  In v21.11 LTS, protocol header fields will be cleaned and only protocol header
  struct will remain.

> This is what we have in the code:
>     union {
>         struct {
>             /*
>              * These fields are retained for compatibility.
>              * Please switch to the new header field below.
>              */
>             struct rte_ether_addr dst; /**< Destination MAC. */
>             struct rte_ether_addr src; /**< Source MAC. */
>             rte_be16_t type; /**< EtherType or TPID. */
>         };
>         struct rte_ether_hdr hdr;
>     };
> 
> Do you think we should remove the old fields now?

From the OVS perspective, we do not care much.  OVS is still using
old fields, but it's fairly simple change that we can do while moving
to a new version of DPDK.

From the perspective of the API clarity, it's probably better to clean
up these structures.

Best regards, Ilya Maximets.

> 
>> Fixes: 09315fc83861 ("ethdev: add VLAN attributes to ethernet and VLAN items")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> 
> 
>