From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Andrew Rybchenko <arybchenko@solarflare.com>
Cc: dev@dpdk.org, Jingjing Wu <jingjing.wu@intel.com>,
Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: add GENEVE flow pattern item
Date: Thu, 23 Nov 2017 10:39:57 +0100 [thread overview]
Message-ID: <20171123093957.GH4062@6wind.com> (raw)
In-Reply-To: <1511166121-743-2-git-send-email-arybchenko@solarflare.com>
On Mon, Nov 20, 2017 at 08:22:00AM +0000, Andrew Rybchenko wrote:
> From: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
>
> Add new pattern item RTE_FLOW_ITEM_TYPE_GENEVE in flow API.
> This commit also adds default mask for these item.
>
> Signed-off-by: Roman Zhukov <Roman.Zhukov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
OK, the main issue in this patch is you're inserting GENEVE in the middle of
everything (enums, documentation, etc). Just append it to the enum to avoid
ABI breakage as described in my previous message and use the same position
everywhere else for consistency.
It must appear after RTE_FLOW_ITEM_TYPE_ESP.
More comments below.
> ---
> doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
> lib/librte_ether/rte_flow.c | 1 +
> lib/librte_ether/rte_flow.h | 31 +++++++++++++++++++++++++++++++
> 3 files changed, 44 insertions(+)
>
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index d158be5..2f96623 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -863,6 +863,18 @@ Matches a VXLAN header (RFC 7348).
> - ``rsvd1``: reserved, normally 0x00.
> - Default ``mask`` matches VNI only.
>
> +Item: ``GENEVE``
> +^^^^^^^^^^^^^^^
> +
> +Matches a GENEVE header.
> +
> +- ``ver_opt_len_o_c_rsvd0``: version (2b), length of the options fields (6b),
> + OAM packet (1b), critical options present (1b), reserved 0 (6b).
> +- ``protocol``: protocol type.
> +- ``vni``: virtual network identifier.
> +- ``rsvd1``: reserved, normally 0x00.
> +- Default ``mask`` matches protocol type and VNI.
> +
I'm not sure about the default mask. It should be the least common
denominator, not necessarily what the first implementation supports. How
about making it like VXLAN, i.e. VNI only? Does that make sense at all?
> Item: ``E_TAG``
> ^^^^^^^^^^^^^^^
>
> diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
> index 6659063..bf1b253 100644
> --- a/lib/librte_ether/rte_flow.c
> +++ b/lib/librte_ether/rte_flow.c
> @@ -77,6 +77,7 @@ static const struct rte_flow_desc_data rte_flow_desc_item[] = {
> MK_FLOW_ITEM(TCP, sizeof(struct rte_flow_item_tcp)),
> MK_FLOW_ITEM(SCTP, sizeof(struct rte_flow_item_sctp)),
> MK_FLOW_ITEM(VXLAN, sizeof(struct rte_flow_item_vxlan)),
> + MK_FLOW_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),
You should add it at the end but some items are already missing from that
list. Since I plan to send an overhaul for this function, you can leave this
change out for the time being.
> MK_FLOW_ITEM(MPLS, sizeof(struct rte_flow_item_mpls)),
> MK_FLOW_ITEM(GRE, sizeof(struct rte_flow_item_gre)),
> MK_FLOW_ITEM(E_TAG, sizeof(struct rte_flow_item_e_tag)),
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 47c88ea..29d81d4 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -272,6 +272,13 @@ enum rte_flow_item_type {
> RTE_FLOW_ITEM_TYPE_VXLAN,
>
> /**
> + * Matches a GENEVE header.
> + *
> + * See struct rte_flow_item_geneve.
> + */
> + RTE_FLOW_ITEM_TYPE_GENEVE,
> +
> + /**
> * Matches a E_TAG header.
> *
> * See struct rte_flow_item_e_tag.
> @@ -651,6 +658,30 @@ static const struct rte_flow_item_vxlan rte_flow_item_vxlan_mask = {
> #endif
>
> /**
> + * RTE_FLOW_ITEM_TYPE_GENEVE.
> + *
> + * Matches a GENEVE header.
> + */
> +struct rte_flow_item_geneve {
> + /**
> + * Version (2b), length of the options fields (6b), OAM packet (1b),
> + * critical options present (1b), reserved 0 (6b).
> + */
> + rte_be16_t ver_opt_len_o_c_rsvd0;
> + rte_be16_t protocol; /**< Protocol type. */
> + uint8_t vni[3]; /**< Virtual Network Identifier. */
> + uint8_t rsvd1; /**< Reserved, normally 0x00. */
> +};
> +
> +/** Default mask for RTE_FLOW_ITEM_TYPE_GENEVE. */
> +#ifndef __cplusplus
> +static const struct rte_flow_item_geneve rte_flow_item_geneve_mask = {
> + .protocol = RTE_BE16(0xffff),
> + .vni = "\xff\xff\xff",
> +};
> +#endif
So how about removing .protocol from the default mask?
> +
> +/**
> * RTE_FLOW_ITEM_TYPE_E_TAG.
> *
> * Matches a E-tag header.
> --
> 2.7.4
>
--
Adrien Mazarguil
6WIND
next prev parent reply other threads:[~2017-11-23 9:40 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-20 8:21 [dpdk-dev] [PATCH 0/2] ethdev: add GENEVE to flow API Andrew Rybchenko
2017-11-20 8:22 ` [dpdk-dev] [PATCH 1/2] ethdev: add GENEVE flow pattern item Andrew Rybchenko
2017-11-23 9:39 ` Adrien Mazarguil [this message]
2017-11-20 8:22 ` [dpdk-dev] [PATCH 2/2] app/testpmd: support GENEVE pattern item in flow rules Andrew Rybchenko
2017-11-23 9:40 ` Adrien Mazarguil
2017-11-23 9:39 ` [dpdk-dev] [PATCH 0/2] ethdev: add GENEVE to flow API Adrien Mazarguil
2017-11-23 10:07 ` Andrew Rybchenko
2017-12-01 10:43 ` [dpdk-dev] [PATCH v2 " Andrew Rybchenko
2017-12-01 10:43 ` [dpdk-dev] [PATCH v2 1/2] ethdev: add GENEVE flow pattern item Andrew Rybchenko
2017-12-04 14:03 ` Adrien Mazarguil
2017-12-07 23:52 ` Ferruh Yigit
2017-12-08 10:52 ` Adrien Mazarguil
2017-12-08 19:06 ` Ferruh Yigit
2017-12-01 10:43 ` [dpdk-dev] [PATCH v2 2/2] app/testpmd: support GENEVE pattern item in flow rules Andrew Rybchenko
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=20171123093957.GH4062@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=Roman.Zhukov@oktetlabs.ru \
--cc=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=jingjing.wu@intel.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).