[dpdk-dev] [PATCH] ethdev: add VLAN attributes to ETH and VLAN items

Ori Kam orika at nvidia.com
Wed Oct 7 14:13:56 CEST 2020
Previous message: [dpdk-dev] [PATCH] ethdev: add VLAN attributes to ETH and VLAN items
Next message: [dpdk-dev] [PATCH] ethdev: add VLAN attributes to ETH and VLAN items
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Sorry for jumping late,


> -----Original Message-----
> From: Maxime Leroy <maxime.leroy at 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 <dekelp at nvidia.com> wrote:
> >
> > Thanks, PSB.
> >
> > > -----Original Message-----
> > > From: Maxime Leroy <maxime.leroy at 6wind.com>
> > > Sent: Friday, October 2, 2020 3:39 PM
> > > To: Dekel Peled <dekelp at nvidia.com>
> > > Cc: Ori Kam <orika at nvidia.com>; NBU-Contact-Thomas Monjalon
> > > <thomas at monjalon.net>; ferruh.yigit at intel.com;
> > > arybchenko at solarflare.com; dev at dpdk.org; Dekel Peled
> > > <dekelp at 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 <dekelp at nvidia.com> wrote:
> > > >
> > > > From: Dekel Peled <dekelp at 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%2Fmail
> > > > 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 <dekelp at 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 ?
> 
> >
> > > >  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
Previous message: [dpdk-dev] [PATCH] ethdev: add VLAN attributes to ETH and VLAN items
Next message: [dpdk-dev] [PATCH] ethdev: add VLAN attributes to ETH and VLAN items
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the dev mailing list