Thanks, PSB. > -----Original Message----- > From: Ori Kam <or...@nvidia.com> > Sent: Wednesday, October 7, 2020 3:14 PM > To: Maxime Leroy <maxime.le...@6wind.com>; Dekel Peled > <dek...@nvidia.com>; NBU-Contact-Thomas Monjalon > <tho...@monjalon.net> > Cc: ferruh.yi...@intel.com; arybche...@solarflare.com; dev@dpdk.org > Subject: RE: [PATCH] ethdev: add VLAN attributes to ETH and VLAN items > > Sorry for jumping late,
Please note v2 of this patch is in ML since Oct. 1st. We should continue the discussion on the latest thread. More comments below. > > > > -----Original Message----- > > From: Maxime Leroy <maxime.le...@6wind.com> > > Sent: Wednesday, October 7, 2020 2:52 PM > > Subject: Re: [PATCH] ethdev: add VLAN attributes to ETH and VLAN items > > > > On Mon, Oct 5, 2020 at 11:37 AM Dekel Peled <dek...@nvidia.com> wrote: > > > > > > Thanks, PSB. > > > > > > > -----Original Message----- > > > > From: Maxime Leroy <maxime.le...@6wind.com> > > > > Sent: Friday, October 2, 2020 3:39 PM > > > > To: Dekel Peled <dek...@nvidia.com> > > > > Cc: Ori Kam <or...@nvidia.com>; NBU-Contact-Thomas Monjalon > > > > <tho...@monjalon.net>; ferruh.yi...@intel.com; > > > > arybche...@solarflare.com; dev@dpdk.org; Dekel Peled > > > > <dek...@mellanox.com> > > > > Subject: Re: [PATCH] ethdev: add VLAN attributes to ETH and VLAN > > > > items > > > > > > > > Hi Dekel, > > > > > > > > On Thu, Oct 1, 2020 at 8:49 PM Dekel Peled <dek...@nvidia.com> > wrote: > > > > > > > > > > From: Dekel Peled <dek...@mellanox.com> > > > > > > > > > > This patch implements the change proposes in RFC [1], adding > > > > > dedicated fields to ETH and VLAN items structs, to clearly > > > > > define the required characteristic of a packet, and enable precise > match criteria. > > > > > > > > > > [1] > > > > > > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > > mail > > > > > s.dpdk.org%2Farchives%2Fdev%2F2020- > > > > August%2F177536.html&data=02%7C > > > > > > > > > > > > 01%7Cdekelp%40nvidia.com%7Cc12bfd3f662747f7b7c408d866d0376f%7C430 > > > > 83d15 > > > > > > > > > > > > 727340c1b7db39efd9ccc17a%7C0%7C0%7C637372391779092411&sdata= > > > > yeOKvc > > > > > > 4r0dL09UZ65%2Bt4qWJqJmcp21VyPSK%2FhbablKI%3D&reserved=0 > > > > > > > > > > Signed-off-by: Dekel Peled <dek...@mellanox.com> > > > > > --- > > > > > doc/guides/rel_notes/release_20_11.rst | 7 +++++++ > > > > > lib/librte_ethdev/rte_flow.h | 16 +++++++++++++--- > > > > > 2 files changed, 20 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/doc/guides/rel_notes/release_20_11.rst > > > > > b/doc/guides/rel_notes/release_20_11.rst > > > > > index 7f9d0dd..199c60b 100644 > > > > > --- a/doc/guides/rel_notes/release_20_11.rst > > > > > +++ b/doc/guides/rel_notes/release_20_11.rst > > > > > @@ -173,6 +173,13 @@ API Changes > > > > > * ``_rte_eth_dev_callback_process()`` -> > > > > ``rte_eth_dev_callback_process()`` > > > > > * ``_rte_eth_dev_reset`` -> ``rte_eth_dev_internal_reset()`` > > > > > > > > > > +* ethdev: Added new field ``vlan_exist`` to structure > > > > > +``rte_flow_item_eth``, > > > > > + indicating that at least one VLAN exists in the packet header. > > > > > + > > > > > +* ethdev: Added new field ``more_vlans_exist`` to structure > > > > > + ``rte_flow_item_vlan``, indicating that at least one more > > > > > +VLAN exists in > > > > > + packet header, following this VLAN. > > > > > + > > > > > * rawdev: Added a structure size parameter to the functions > > > > > ``rte_rawdev_queue_setup()``, ``rte_rawdev_queue_conf_get()``, > > > > > ``rte_rawdev_info_get()`` and ``rte_rawdev_configure()``, > > > > > diff --git a/lib/librte_ethdev/rte_flow.h > > > > > b/lib/librte_ethdev/rte_flow.h index da8bfa5..39d04ef 100644 > > > > > --- a/lib/librte_ethdev/rte_flow.h > > > > > +++ b/lib/librte_ethdev/rte_flow.h > > > > > @@ -723,14 +723,18 @@ struct rte_flow_item_raw { > > > > > * If the @p type field contains a TPID value, then only tagged > > > > > packets > > with > > > > the > > > > > * specified TPID will match the pattern. > > > > > * Otherwise, only untagged packets will match the pattern. > > > > > - * If the @p ETH item is the only item in the pattern, and the > > > > > @p type field > > > > > - * is not specified, then both tagged and untagged packets will > > > > > match the > > > > > - * pattern. > > > > > + * The field @p vlan_exist can be used to match specific packet > > > > > + types, instead > > > > > + * of using the @p type field. > > > > > + * This can be used to match any type of tagged packets. > > > > > + * If the @p type and @p vlan_exist fields are not specified, > > > > > + then both tagged > > > > > + * and untagged packets will match the pattern. > > > > > */ > > > > > struct rte_flow_item_eth { > > > > > struct rte_ether_addr dst; /**< Destination MAC. */ > > > > > struct rte_ether_addr src; /**< Source MAC. */ > > > > > rte_be16_t type; /**< EtherType or TPID. */ > > > > > + uint32_t vlan_exist:1; /**< At least one VLAN exist in > > > > > header. */ > > > > > + uint32_t reserved:31; /**< Reserved, must be zero. */ > > > > > }; > > > > > > > > To resume: > > > > - type and vlan_exists fields not specified: tag and untagged > > > > matched > > > > - with vlan_exists, match only tag or untagged > > > > - with type matching specific ethernet type > > > > - vlan_exists and type should not setted at the same time ? > > > > > > PMD should validate they don't conflict. > > > > > > > > > > > With this new specification, I think you address all the use cases. > > > > That's great ! > > > > > > > > > > Glad to see we agree on this. > > > > > > > > > > > > > /** Default mask for RTE_FLOW_ITEM_TYPE_ETH. */ @@ -752,10 > > +756,16 > > > > @@ > > > > > struct rte_flow_item_eth { > > > > > * the preceding pattern item. > > > > > * If a @p VLAN item is present in the pattern, then only > > > > > tagged packets > > > > will > > > > > * match the pattern. > > > > > + * The field @p more_vlans_exist can be used to match specific > > > > > + packet types, > > > > > + * instead of using the @p inner_type field. > > > > > + * This can be used to match any type of tagged packets. > > > > > */ > > > > > > > > Could you please specify what the expected behavior when > > > > inner_type and more_vlans_exist are not specified . > > > > What is the default behavior ? > > > > > > > > > > You wrote above for the eth item, if the user didn't specify it > > > means don't- > > care. > > Could you please add the same comment for the vlan pattern ? I will send v3 with added description. > > > > > > > > > > struct rte_flow_item_vlan { > > > > > rte_be16_t tci; /**< Tag control information. */ > > > > > rte_be16_t inner_type; /**< Inner EtherType or TPID. */ > > > > > + uint32_t more_vlans_exist:1; > > > > > + /**< At least one more VLAN exist in header, following this > VLAN. */ > > > > > + uint32_t reserved:31; /**< Reserved, must be zero. */ > > > > > }; > > > > > > > > > > /** Default mask for RTE_FLOW_ITEM_TYPE_VLAN. */ > > > > > -- > > > > > 1.8.3.1 > > > > > > > > > > > > > I am still wondering, why not using a new item 'NOT' for example > > > > to match only eth packet not tagged ? > > > > example: eth / not vlan. It's a more generic solution. > > > > > > > > Here in this commit, we add a reference on VLAN fields on ethernet > > header. > > > > But tomorrow, we could do the same for mpls by adding mpls_exists > > > > in the eth item and so on. > > > > > > > > In fact, we have the same needs for IPv6 options. To match for > > > > example, > > > > ipv6 packet with no fragment option. > > > > With a NOT field, it can be easily done: > eth / ipv6 / no ipv6_frag. > > > > > > > > Adding new fields 'item'_exists into eth and ipv6 do the jobs, but > > > > having a NOT attribute is a more generic solution. > > > > > > > > It could address many other use cases like matching any udp > > > > packets that > > are > > > > not vxlan ( eth / ipv4 / vxlan / not udp), > > > > > > > > Let me know what you think about that. > > > > > > I agree with Thomas Monjalon response on this. > > > > RTE_FLOW pattern is here to have a generic way to describe a flow. > > > > Of course, hardware nics don't need to support any type of pattern. > > It's why we have rte_flow_validate functions to be sure that the > > hardware can match this type of pattern. > > > > For example, the not attribute could be only supported for vlan item > > with mlx5 nics. (i.e. eth / not vlan). > > > > When a user adds a flow with a pattern including a not attribute, if > > the pmd doesn't support it, it should return -ENOTSUP. > > > > Later, if we add support of not attribute with mpls (i.e. eth / not > > mpls) in mlx5 pmd, modification can be done on the pmd side, without > > any API changes. > > > > You are already adding new '_exits' fields in IPv6 item. It's why I > > think having a generic solution like a NOT attribute, it's a better > > solution. > > > > If we continue to add '_exists' fields in each item (like you are > > doing with IPv6 item), I think we will need to do an API rework to > > have a generic solution. > > > > Regards, > > > > Maxime > > First I'm all in favor of adding a not item, but it is very tricky and should > be > designed very carefully. > Also using a not will get the rules to be very complicated. > For example think about the following case: > Application want only packets without any extensions, using the suggested > API it is very simple just set exits = 0 with mask = 1. > While if we use the not I'm not sure how it should look, since we need > number of not, it is not just enough to say next proto is not XXX since we > need to cover all possible extensions, also there might be ordering issue > which the not can't support. > > So my thinking is that we should go with the suggested approach and > regardless see how can we add the not. > > In any case the exit should not be the goto solution but again in case of > extension it make sense since order is not guaranteed. > > What do you think? > > Best, > Ori >
