From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2FDB3A04BA; Wed, 7 Oct 2020 13:52:28 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 576721B9F0; Wed, 7 Oct 2020 13:52:26 +0200 (CEST) Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by dpdk.org (Postfix) with ESMTP id 570921B9EE for ; Wed, 7 Oct 2020 13:52:24 +0200 (CEST) Received: by mail-wm1-f65.google.com with SMTP id z22so4333679wmi.0 for ; Wed, 07 Oct 2020 04:52:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=fW7sNvyH08ueQGcpIaFNQ1mzrqh31U780Lz8UgzBVj8=; b=jep1oeOerVqsj+qog9V2PPVRBrf/Gd+GfIzfnhnZ8Wgb+RIg5dAFSwzoNigqygLAt4 RN1FRokkDgbU8DV7QZxeYxPl39ba9xUUZPE4VDYrnOOiOKGhj43bEAclL7L3243Afdsc ZYftNeSdEZMLfW4X6i8q6rZjXgUlaOCcwKhBYRmhAY+OR79+Ic/imdrLr0H3B6zLWkbn 4zp6cT+8uFS3OHSlRgH6GyYfhnD93JztZsR1b5KJKCNMCNNYEhfvbU24WXrmeu8cr+Kx IXYo2uNovkQJejW3ffd0libXWxmGyuKH4r71SKNI0SpL6MKss8IYItJt/2+YnfTFjq99 f5Og== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=fW7sNvyH08ueQGcpIaFNQ1mzrqh31U780Lz8UgzBVj8=; b=WFBKs9MJJEoP5osu0iYF7S0KdzgZjIdrZWmWsa9R/6FbLgzTvH6E5TJG29Dr17zkpJ vsLlf36m2hgaUdrtkMoxtfD+JFy1dQpRXJvsgPR3k6+mkWKfccWUlUlQJXGhdAOlaaED AP4tQBCFbJSE13W047s6feqOXjMyRkeZbiePxsveHztHGXlXhhz1fclhpxGLQEJyhYzt cshGNNSQjaKfN4bL7GouVzHxd0eeW/uomIXvJZK6U5/AdyUiDROUKefjK+JDa3/B0ZIQ 5n5/WOh+ssh9SUzQ8DL4N49s33dk/TUH6b4Go+mDGwmDWwiEgly509lzFcLsxrKdRryr WEXQ== X-Gm-Message-State: AOAM533EjXw03zEcJhqggLOl8bFqTovtLNSzDs/Xr7rHTuxOFZun/qE6 YykngYur8VL+rpAoPRQ6jgTNjG7VauL/C1NDQEl5Hg== X-Google-Smtp-Source: ABdhPJzJ4+IKFq3dop5XjFuXWqs7mS9iA5jOjPGgVyzkXz4HTk+h7AbqAVFVi8Gdq8y7Yb3D84e3WtDR94Imu59Clzw= X-Received: by 2002:a1c:6054:: with SMTP id u81mr2887652wmb.10.1602071544019; Wed, 07 Oct 2020 04:52:24 -0700 (PDT) MIME-Version: 1.0 References: <209f5087596180d7866a43f0a0f12c9a032eb7ce.1601577847.git.dekelp@nvidia.com> In-Reply-To: From: Maxime Leroy Date: Wed, 7 Oct 2020 13:52:12 +0200 Message-ID: To: Dekel Peled , NBU-Contact-Thomas Monjalon Cc: Ori Kam , "ferruh.yigit@intel.com" , "arybchenko@solarflare.com" , "dev@dpdk.org" Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH] ethdev: add VLAN attributes to ETH and VLAN items X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Oct 5, 2020 at 11:37 AM Dekel Peled wrote: > > Thanks, PSB. > > > -----Original Message----- > > From: Maxime Leroy > > Sent: Friday, October 2, 2020 3:39 PM > > To: Dekel Peled > > Cc: Ori Kam ; NBU-Contact-Thomas Monjalon > > ; ferruh.yigit@intel.com; > > arybchenko@solarflare.com; dev@dpdk.org; Dekel Peled > > > > 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 wrote: > > > > > > From: Dekel Peled > > > > > > 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 > > > --- > > > 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