From: "Zhang, Qi Z" <qi.z.zhang@intel.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>,
"Wang, Ying A" <ying.a.wang@intel.com>
Cc: "Ye, Xiaolong" <xiaolong.ye@intel.com>,
"Yang, Qiming" <qiming.yang@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2] ethdev: add more protocol support in flow API
Date: Mon, 19 Aug 2019 11:53:12 +0000 [thread overview]
Message-ID: <039ED4275CED7440929022BC67E7061153D80195@SHSMSX105.ccr.corp.intel.com> (raw)
In-Reply-To: <20190814090810.GM4512@6wind.com>
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, August 14, 2019 5:08 PM
> To: Wang, Ying A <ying.a.wang@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Yang, Qiming <qiming.yang@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: add more protocol support in flow
> API
>
> Hi Wang Ying,
>
> On Wed, Aug 14, 2019 at 11:24:30AM +0800, Wang Ying A wrote:
> > Add new protocol header match support as below
> >
> > RTE_FLOW_ITEM_TYPE_GTP_PSC
> > - matches a GTP PDU extension header (type is 0x85:
> > PDU Session Container)
> > RTE_FLOW_ITEM_TYPE_PPPOES
> > - matches a PPPoE Session header.
> > RTE_FLOW_ITEM_TYPE_PPPOED
> > - matches a PPPoE Discovery stage header.
> >
> > Signed-off-by: Wang Ying A <ying.a.wang@intel.com>
>
> OK, please split this patch, one for GTP and the other for PPPoE to make title
> less vague than "add more protocol support".
>
> For PPPoE, the distinction between session and discovery is not a bad idea but
> since discovery packets typically have a different format (tags in place of
> protocol), I'm not sure they should share a common structure.
>
> How about a single "PPPOE" item without proto_id to cover both session and
> discovery, then later add separate tag items on a needed basis for each
> possible/optional tag (e.g. PPPOE_TAG_SERVICE_NAME)? Likewise proto_id
> would be provided through a separate optional item if relevant
> (PPPOE_PROTO_ID).
PPPoE Discovery had PPPoE Session has different ethertype 0x8863 , 0x8864, so I think they should belong to separate
match item,( image the case we only need to match a PPPoE Session flow but don't care what kind of protocol payload it have )
but I agree they should only share the common part of the header,
then for PPPoED, it can append optional PPPoE Tag item(s) and for PPPoES , it can append optional PPPoE Protocol item(s)
> Such an approach is already used for IPV6 and IPV6_EXT.
>
> Another benefit is that applications could match PPPoE regardless of session or
> discovery when they do not care, while PPPOED/PPPOES make that distinction
> mandatory.
>
> Also by inserting new entries in the middle of the pattern items list, this patch
> breaks ABI. I think it's not on purpose, so please move them to the end (not
> grouping them with existing GTP stuff is fine, ABI compat is more
> important.) This must be reflected in rte_flow.h, rte_flow.c, testpmd and
> documentation.
>
> More nits below.
>
> > ---
> > ---
> > v2: Remove Gerrit Change-Id's.
> > ---
> > app/test-pmd/cmdline_flow.c | 80
> +++++++++++++++++++++++++++++
> > doc/guides/prog_guide/rte_flow.rst | 25 +++++++++
> > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 10 ++++
> > lib/librte_ethdev/rte_flow.c | 3 ++
> > lib/librte_ethdev/rte_flow.h | 71
> +++++++++++++++++++++++++
> > 5 files changed, 189 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> [...]
> > + [ITEM_PPPOES] = {
> > + .name = "pppoes",
> > + .help = "match PPPoE Session header",
>
> Session => session
>
> > + .priv = PRIV_ITEM(PPPOES, sizeof(struct rte_flow_item_pppoe)),
> > + .next = NEXT(item_pppoe),
> > + .call = parse_vc,
> > + },
> > + [ITEM_PPPOED] = {
> > + .name = "pppoed",
> > + .help = "match PPPoE Discovery stage header",
>
> Discovery => discovery
>
> > + .priv = PRIV_ITEM(PPPOED, sizeof(struct rte_flow_item_pppoe)),
> > + .next = NEXT(item_pppoe),
> > + .call = parse_vc,
> > + },
> > + [ITEM_PPPOE_SEID] = {
> > + .name = "seid",
> > + .help = "Session identifier",
>
> Session => session
>
> [...]
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index 821b524b3..d09c42071 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -1055,6 +1055,31 @@ flow rules.
> > - ``teid``: tunnel endpoint identifier.
> > - Default ``mask`` matches teid only.
> >
> > +Item: ``GTP_PSC``
> > +^^^^^^^^^^^^^^^^^^^^^^^
>
> Too many "^^^"'s.
>
> [...]
> > +Item: ``PPPOES``, ``PPPOED``
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +Matches a PPPOE header.
>
> PPPOE => PPPoE
>
> > +
> > +Note: PPPOES and PPPOED use the same structure. PPPOES and PPPOED
> > +item
>
> item => items (or better, "pattern items")
>
> > +are defined for a user-friendly API when creating PPPOE-Session and
> > +PPPOE-Discovery flow rules.
>
> Super nit: use "PPPoE" when mentioning the protocol itself and "PPPOE" when
> mentioning the pattern item.
>
> > +
> > +- ``v_t_flags``: version (4b), type (4b).
>
> Why "flags"? I don't see any so you could name it "version_type" (same in
> documentation).
>
> > +- ``code``: Message type.
>
> Message => message
>
> > +- ``session_id``: Session identifier.
>
> Session => session
>
> > +- ``length``: Payload length.
>
> Payload => payload
>
> > +- ``proto_id``: PPP Protocol identifier.
>
> Protocol => protocol
>
> > +- Default ``mask`` matches session_id,proto_id.
>
> Missing space between session_id and proto_id.
>
> > +
> > Item: ``ESP``
> > ^^^^^^^^^^^^^
> >
> > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > index 313e0707e..0da36d5f1 100644
> > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> > @@ -3904,6 +3904,16 @@ This section lists supported pattern items and
> their attributes, if any.
> >
> > - ``teid {unsigned}``: tunnel endpoint identifier.
> >
> > +- ``gtp_psc``: match GTPv1 entension header (type is 0x85).
> > +
> > + - ``pdu_type {unsigned}``: PDU type (0 or 1).
> > + - ``qfi {unsigned}``: QoS flow identifier.
> > +
> > +- ``pppoes``, ``pppoed``: match PPPOE header.
>
> PPPOE => PPPoE
>
> [...]
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index b66bf1495..ad5e46190 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -328,6 +328,34 @@ enum rte_flow_item_type {
> > */
> > RTE_FLOW_ITEM_TYPE_GTPU,
> >
> > + /**
> > + * Matches a GTP PDU extension header (type is 0x85:
> > + * PDU Session Container).
>
> Session Container => session container
>
> > + *
> > + * Configure flow for GTP packets with extension header type 0x85.
> > + *
> > + * See struct rte_flow_item_gtp_psc.
> > + */
> > + RTE_FLOW_ITEM_TYPE_GTP_PSC,
> > +
> > + /**
> > + * Matches a PPPOE header.
> > + *
> > + * Configure flow for PPPoE Session packets.
>
> Session => session
>
> > + *
> > + * See struct rte_flow_item_pppoe.
> > + */
> > + RTE_FLOW_ITEM_TYPE_PPPOES,
> > +
> > + /**
> > + * Matches a PPPOE header.
> > + *
> > + * Configure flow for PPPoE Discovery stage packets.
>
> Discovery => discovery
>
> > + *
> > + * See struct rte_flow_item_pppoe.
> > + */
> > + RTE_FLOW_ITEM_TYPE_PPPOED,
> > +
> > /**
> > * Matches a ESP header.
> > *
> > @@ -922,6 +950,49 @@ static const struct rte_flow_item_gtp
> > rte_flow_item_gtp_mask = { }; #endif
> >
> > +/**
> > + * RTE_FLOW_ITEM_TYPE_GTP_PSC.
> > + *
> > + * Matches a GTP-extension header
> > + * (type is 0x85: PDU Session Container).
>
> Session Container => session container
>
> (crusade against superfluous caps!)
>
> [...]
> > +/**
> > + * RTE_FLOW_ITEM_TYPE_PPPOE.
> > + *
> > + * Matches a PPPOE header.
> > + */
> > +struct rte_flow_item_pppoe {
> > + /**
> > + * Version (4b), type (4b).
> > + */
> > + uint8_t v_t_flags;
>
> v_t_flags => version_type
>
> > + uint8_t code; /**< Message type. */
> > + rte_be16_t session_id; /**< Session identifier. */
> > + rte_be16_t length; /**< Payload length. */
> > + rte_be16_t proto_id; /**< PPP Protocol identifier. */
>
> As discussed, I suggest dropping proto_id to make this a generic match for
> PPPoE.
>
> > +};
> > +
> > +/** Default mask for RTE_FLOW_ITEM_TYPE_PPPOE. */ #ifndef __cplusplus
> > +static const struct rte_flow_item_pppoe rte_flow_item_pppoe_mask = {
> > + .session_id = RTE_BE16(0xffff),
> > + .proto_id = RTE_BE16(0xffff),
> > +};
>
> I think the default for PPPoE should be an empty mask that simply says "match
> PPPoE" since session_id only becomes known after negotiation and proto_id
> shouldn't be part of this object.
>
> --
> Adrien Mazarguil
> 6WIND
next prev parent reply other threads:[~2019-08-19 11:53 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-14 3:00 [dpdk-dev] [PATCH] " Wang Ying A
2019-08-14 3:24 ` [dpdk-dev] [PATCH v2] " Wang Ying A
2019-08-14 9:08 ` Adrien Mazarguil
2019-08-19 11:53 ` Zhang, Qi Z [this message]
2019-08-20 6:50 ` [dpdk-dev] [PATCH v3 0/2] add GTP/PPPoE to " Wang Ying A
2019-08-20 6:50 ` [dpdk-dev] [PATCH v3 1/2] ethdev: add GTP extension header " Wang Ying A
2019-08-28 6:00 ` [dpdk-dev] [PATCH v4 0/2] add GTP/PPPoE " Wang Ying A
2019-08-28 6:00 ` [dpdk-dev] [PATCH v4 1/2] ethdev: add GTP extension header " Wang Ying A
2019-09-28 19:02 ` Ori Kam
2019-08-28 6:00 ` [dpdk-dev] [PATCH v4 2/2] ethdev: add PPPoE " Wang Ying A
2019-09-28 19:05 ` Ori Kam
2019-09-23 23:44 ` [dpdk-dev] [PATCH v4 0/2] add GTP/PPPoE " Zhang, Qi Z
2019-10-01 7:52 ` Ferruh Yigit
2019-08-20 6:50 ` [dpdk-dev] [PATCH v3 2/2] ethdev: add PPPoE " Wang Ying A
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=039ED4275CED7440929022BC67E7061153D80195@SHSMSX105.ccr.corp.intel.com \
--to=qi.z.zhang@intel.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=qiming.yang@intel.com \
--cc=xiaolong.ye@intel.com \
--cc=ying.a.wang@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).