DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Ilya Maximets <i.maximets@ovn.org>
Cc: dev@dpdk.org, stable@dpdk.org,
	Ajit Khaparde <ajit.khaparde@broadcom.com>,
	Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>,
	Hemant Agrawal <hemant.agrawal@nxp.com>,
	Haiyue Wang <haiyue.wang@intel.com>,
	John Daley <johndale@cisco.com>,
	Guoyang Zhou <zhouguoyang@huawei.com>,
	"Min Hu (Connor)" <humin29@huawei.com>,
	Beilei Xing <beilei.xing@intel.com>,
	Jingjing Wu <jingjing.wu@intel.com>,
	Qi Zhang <qi.z.zhang@intel.com>, Rosen Xu <rosen.xu@intel.com>,
	Matan Azrad <matan@nvidia.com>,
	Viacheslav Ovsiienko <viacheslavo@nvidia.com>,
	Liron Himi <lironh@marvell.com>,
	Jiawen Wu <jiawenwu@trustnetic.com>, Ori Kam <orika@nvidia.com>,
	ferruh.yigit@amd.com, andrew.rybchenko@oktetlabs.ru,
	david.marchand@redhat.com, olivier.matz@6wind.com
Subject: Re: [PATCH] doc: fix support table for ETH and VLAN flow items
Date: Wed, 12 Oct 2022 17:30:14 +0200	[thread overview]
Message-ID: <6479268.QJadu78ljV@thomas> (raw)
In-Reply-To: <96eb4c24-5a2c-98f0-9f27-4445a7f2732e@ovn.org>

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



  reply	other threads:[~2022-10-12 15:30 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-16 12:01 Ilya Maximets
2022-04-20 17:51 ` Ferruh Yigit
2022-04-26  8:55   ` Asaf Penso
2022-04-26 10:47     ` Ferruh Yigit
2022-04-27 11:02       ` Asaf Penso
2022-09-12 10:09 ` Thomas Monjalon
2022-09-12 11:08   ` Ori Kam
2022-10-07 11:50   ` Ilya Maximets
2022-10-07 11:55     ` Ilya Maximets
2022-10-12 15:30       ` Thomas Monjalon [this message]
2022-10-13 11:01         ` Ilya Maximets
2022-10-13 12:18           ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6479268.QJadu78ljV@thomas \
    --to=thomas@monjalon.net \
    --cc=ajit.khaparde@broadcom.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=beilei.xing@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@amd.com \
    --cc=haiyue.wang@intel.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=humin29@huawei.com \
    --cc=i.maximets@ovn.org \
    --cc=jiawenwu@trustnetic.com \
    --cc=jingjing.wu@intel.com \
    --cc=johndale@cisco.com \
    --cc=lironh@marvell.com \
    --cc=matan@nvidia.com \
    --cc=olivier.matz@6wind.com \
    --cc=orika@nvidia.com \
    --cc=qi.z.zhang@intel.com \
    --cc=rahul.lakkireddy@chelsio.com \
    --cc=rosen.xu@intel.com \
    --cc=stable@dpdk.org \
    --cc=viacheslavo@nvidia.com \
    --cc=zhouguoyang@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).