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

[Thomas Monjalon <thomas@monjalon.net>]

07/10/2022 13:55, Ilya Maximets:
> On 10/7/22 13:50, Ilya Maximets wrote:
> > 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?

I'm afraid it can be long and complex to add notes in the page
about what is missing to get complete support for each feature/PMD.
I think you can just re-spin with a clear explanation in the commit,
so PMD maintainers can refer to it.

> >> 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.

No problem, we are still on time for 22.11.

> >>> 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.

You're right, this is the general notice for rte_flow_item_*.

> >> 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.
> 
> There seems to be some outstanding work still though:
>   https://patches.dpdk.org/project/dpdk/patch/20210312110745.31721-1-ivan.malov@oktetlabs.ru/#129161

I think nobody took care of it so far.
I will send a patch to clean the easy ones,
and I will follow up with Ori for a complete plan.

Thanks