From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f194.google.com (mail-wr0-f194.google.com [209.85.128.194]) by dpdk.org (Postfix) with ESMTP id 2CDD48E7C for ; Thu, 19 Apr 2018 16:49:17 +0200 (CEST) Received: by mail-wr0-f194.google.com with SMTP id q3-v6so4784661wrj.6 for ; Thu, 19 Apr 2018 07:49:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=JRuT5qwl5ITLFrc9tGAq81W64SjukYTGNyAWOpJVd18=; b=JUUcd2SrRp7s8MBdA3BS8SLbJ0sdR+54nN81XKVfAxlQX/plNSvEa1j/iNvWz/vEpP KcxqNsQCypr+ay/oJhm+enAJ4U3U2LVu/Uo9TMclMwIcFN9NXiHIHpbTRRXfSL3xB5bD 0kz4DtIpNMka5irCOBmhs7spSeVwvkeaqFbmOpQdNzSOZ6XAWMwuN1qOCYqf0kzYCVla dw0eMYQHv5KjSi4lVr8jE2D6A+m5H/gfBwQtORIrUmEUR+6S7mFBoYdrtGOp784SP8LX VOqWo59VX+5ZSPizB8lu3scqYjQ9Dg6zLylzdBxZ5zNnZQGvOgDK+yTditYr01i5OB0u c6CQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=JRuT5qwl5ITLFrc9tGAq81W64SjukYTGNyAWOpJVd18=; b=ar7Gy3EH0Oj26AuUo4RjviQdCH/ydRx7QW0+zbfIv7p2PIoODXy1YcOY4mZo4C32Dy p5ZeB53Yf5EfVXkiRwfwByk7vf9gdwudM1Puv1tqS0H3SMj5zzuKeOi7AK0iPx+EM/Y/ T+f1jVCWjmlPu1sVFmUoPG50IoNNkwV9t/vN/Is2mjmu7OxOYMl9HMNisKMXbTTBQh89 i6eOWfMBpR02IY4e0BDNGT6CajZsB/qbCQJ4CrAu9LCD+RovQhHGEDihf4JHml5UhbWp 1U5QjlHgEOpidPTHf31eRJMUF7Oucrs+1fbdBS/EyAp0/z50ekYwSijKNEEzxb8Muyo2 Juqg== X-Gm-Message-State: ALQs6tBBWXnJ6Rr5nT5oPLhjuCuUqbh+3ADp+aLVrSVoYlDO21Ogn690 br6p4OWMH7bywC1aF1dW1mqWSQ== X-Google-Smtp-Source: AIpwx48JCfpEbvzPudDVJJ68hPSvEdeBsu31JGHRL5zJQadrmYLMiadyu88fZLiB2rGwYysnBLivpw== X-Received: by 10.28.147.83 with SMTP id v80mr5021442wmd.91.1524149356853; Thu, 19 Apr 2018 07:49:16 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 36-v6sm4663867wro.63.2018.04.19.07.49.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 Apr 2018 07:49:16 -0700 (PDT) Date: Thu, 19 Apr 2018 16:49:02 +0200 From: Adrien Mazarguil To: Qi Zhang Cc: dev@dpdk.org, declan.doherty@intel.com, sugesh.chandran@intel.com, michael.j.glynn@intel.com, yu.y.liu@intel.com, konstantin.ananyev@intel.com, bruce.richardson@intel.com Message-ID: <20180419144902.GH4957@6wind.com> References: <1522279780-34842-1-git-send-email-qi.z.zhang@intel.com> <20180416061042.785-1-qi.z.zhang@intel.com> <20180416061042.785-5-qi.z.zhang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180416061042.785-5-qi.z.zhang@intel.com> Subject: Re: [dpdk-dev] [PATCH v3 4/4] ethdev: add VLAN and MPLS pop push action in flow API 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: , X-List-Received-Date: Thu, 19 Apr 2018 14:49:19 -0000 On Mon, Apr 16, 2018 at 02:10:42PM +0800, Qi Zhang wrote: > Add couple Openflow frienldy VLAN/MPLS push/pop actions. Openflow => OpenFlow frienldy => friendly > > RTE_FLOW_ACTION_VLAN_POP - pop a VLAN header. > RTE_FLOW_ACTION_VLAN_PUSH - push a VLAN header. > RTE_FLOW_ACTION_MPLS_POP - pop a MPLS header. > RTE_FLOW_ACTION_MPLS_PUSH - push a MPLS header. > > Signed-off-by: Qi Zhang Same fundamental comment as for the previous patch ("ethdev: add TTL change actions in flow API"), "OF" must be part of the name of these new actions, e.g.: RTE_FLOW_ACTION_OF_VLAN_POP More below. > --- > app/test-pmd/cmdline_flow.c | 82 +++++++++++++++++++++++++++++ > doc/guides/prog_guide/rte_flow.rst | 69 ++++++++++++++++++++++++ > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 14 +++++ > lib/librte_ether/rte_flow.h | 71 +++++++++++++++++++++++++ > 4 files changed, 236 insertions(+) > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c > index 417cbecc9..736d79bef 100644 > --- a/app/test-pmd/cmdline_flow.c > +++ b/app/test-pmd/cmdline_flow.c > @@ -201,6 +201,13 @@ enum index { > ACTION_IP_TTL_DEC_LVL, > ACTION_TTL_COPY_OUT, > ACTION_TTL_COPY_IN, > + ACTION_VLAN_POP, > + ACTION_VLAN_PUSH, > + ACTION_VLAN_PUSH_TYPE, > + ACTION_MPLS_POP, > + ACTION_MPLS_POP_TYPE, > + ACTION_MPLS_PUSH, > + ACTION_MPLS_PUSH_TYPE, > }; > > /** Size of pattern[] field in struct rte_flow_item_raw. */ > @@ -678,6 +685,10 @@ static const enum index next_action[] = { > ACTION_IP_TTL_DEC, > ACTION_TTL_COPY_OUT, > ACTION_TTL_COPY_IN, > + ACTION_VLAN_POP, > + ACTION_VLAN_PUSH, > + ACTION_MPLS_POP, > + ACTION_MPLS_PUSH, > ZERO, > }; > > @@ -728,6 +739,21 @@ static const enum index action_ip_ttl_dec[] = { > ZERO, > }; > > +static const enum index action_vlan_push[] = { > + ACTION_VLAN_PUSH_TYPE, > + ZERO, > +}; > + > +static const enum index action_mpls_pop[] = { > + ACTION_MPLS_POP_TYPE, > + ZERO, > +}; > + > +static const enum index action_mpls_push[] = { > + ACTION_MPLS_PUSH_TYPE, > + ZERO, > +}; > + > static int parse_init(struct context *, const struct token *, > const char *, unsigned int, > void *, unsigned int); > @@ -1921,6 +1947,62 @@ static const struct token token_list[] = { > .next = NEXT(NEXT_ENTRY(ACTION_NEXT)), > .call = parse_vc, > }, > + > + [ACTION_VLAN_POP] = { > + .name = "vlan_pop", vlan_pop => of_vlan_pop > + .help = "pop the outermost VLAN header.", These help strings shouldn't be terminated with a "." for consistency. > + .priv = PRIV_ACTION(DROP, 0), DROP => OF_VLAN_POP > + .next = NEXT(NEXT_ENTRY(ACTION_NEXT)), > + .call = parse_vc, > + }, > + [ACTION_VLAN_PUSH] = { > + .name = "vlan_push", vlan_push => of_vlan_push > + .help = "push new VLAN header onto the packet", > + .priv = PRIV_ACTION(VLAN_PUSH, > + sizeof(struct rte_flow_action_vlan_push)), > + .next = NEXT(action_vlan_push), > + .call = parse_vc, > + }, > + [ACTION_VLAN_PUSH_TYPE] = { > + .name = "type", > + .help = "Ethertype of VLAN header", > + .next = NEXT(action_vlan_push, NEXT_ENTRY(UNSIGNED)), Ethertype => EtherType (here and everywhere else, this is the official name for that field) > + .args = ARGS(ARGS_ENTRY(struct rte_flow_action_vlan_push, > + ether_type)), > + .call = parse_vc, > + }, > + [ACTION_MPLS_POP] = { > + .name = "mpls_pop", mpls_pop => of_mpls_pop > + .help = "pop the outermost MPLS header", > + .priv = PRIV_ACTION(MPLS_POP, > + sizeof(struct rte_flow_action_mpls_pop)), > + .next = NEXT(action_mpls_pop), > + .call = parse_vc, > + }, > + [ACTION_MPLS_POP_TYPE] = { > + .name = "type", > + .help = "Ethertype of MPLS header", > + .next = NEXT(action_mpls_pop, NEXT_ENTRY(UNSIGNED)), > + .args = ARGS(ARGS_ENTRY(struct rte_flow_action_mpls_pop, > + ether_type)), > + .call = parse_vc, > + }, > + [ACTION_MPLS_PUSH] = { > + .name = "mpls_push", mpls_push => of_mpls_push > + .help = "push new MPLS header onto the packet", > + .priv = PRIV_ACTION(MPLS_PUSH, > + sizeof(struct rte_flow_action_mpls_push)), > + .next = NEXT(action_mpls_push), > + .call = parse_vc, > + }, > + [ACTION_MPLS_PUSH_TYPE] = { > + .name = "type", > + .help = "Ethertype of MPLS header", > + .next = NEXT(action_mpls_push, NEXT_ENTRY(UNSIGNED)), > + .args = ARGS(ARGS_ENTRY(struct rte_flow_action_mpls_push, > + ether_type)), > + .call = parse_vc, > + }, > }; > > /** Remove and return last entry from argument stack. */ > diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst > index 65f2d4a16..a009f1aac 100644 > --- a/doc/guides/prog_guide/rte_flow.rst > +++ b/doc/guides/prog_guide/rte_flow.rst > @@ -1684,6 +1684,75 @@ required MPLS or IP headers. > | no properties | > +---------------+ > > +Action: ``ACTION_VLAN_POP`` > +^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Pop the outer-most VLAN header from the packet. > + > +.. _table_rte_flow_action_vlan_pop: > + > +.. table:: VLAN_POP > + > + +---------------+ > + | Field | > + +===============+ > + | no properties | > + +---------------+ Missing reference to OF specification etc. > + > +Action: ``ACTION_VLAN_PUSH`` > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Push a new VLAN header onto the packet, Ethertype 0x8100 or 0x88a8 should > +be used. The value of VLAN ID and VLAN priority for the new header is copied > +from an exist VLAN header in the packet, they will be set to 0 if no such > +VLAN header exist. > + > +.. _table_rte_flow_action_vlan_push: > + > +.. table:: VLAN_PUSH > + > + +----------------+------------------------+ > + | Field | Value | > + +================+========================+ > + | ``ether_type`` | Ethertype for VLAN tag | > + +----------------+------------------------+ Beside the missing reference to OF specification, I think this patch should also provide a "VLAN action change" action, otherwise what's the point of pushing a VLAN header without the ability to pick a VID? (see ofp_action_vlan_vid). > + > +Action: ``ACTION_MPLS_POP`` > +^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Pop the outer-most MPLS header from the packet, Ethertype is required for > +the resulting packet if it is not the last MPLS header be popped, Ethertype > +must be 0x8847 or 0x8848. > + > +.. _table_rte_flow_action_mpls_pop: > + > +.. table:: MPLS_POP > + > + +----------------+---------------------------+ > + | Field | Value | > + +================+===========================+ > + | ``ether_type`` | Ethertype for MPLS header | > + +----------------+---------------------------+ Name this field "ethertype" (without "_") like its OF counterpart. > + > +Action: ``ACTION_MPLS_PUSH`` > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Push a new MPLS header onto the packet, Ethertype 0x8847 or 0x8848 should > +be used. The value of MPLS label and traffic class is copied from exist > +outermost MPLS header, and TTL is copied from exist outermost MPLS or IP > +header, they will be set to 0 if exist packet does not contain required > +header. > + > +.. _table_rte_flow_action_mpls_push: > + > +.. table:: MPLS_PUSH > + > + +----------------+---------------------------+ > + | Field | Value | > + +================+===========================+ > + | ``ether_type`` | Ethertype for MPLS header | > + +----------------+---------------------------+ Ditto. > + > Negative types > ~~~~~~~~~~~~~~ > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > index d2fe6637b..c06f2f0b0 100644 > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > @@ -3461,6 +3461,20 @@ This section lists supported actions and their attributes, if any. > > - ``ttl_copy_in``: copy TTL inwards. > > +- ``vlan_pop``: pop the outermost VLAN header. > + > +- ``vlan_push``: push new VLAN header onto the packet. > + > + - ``type``: Ethertype of the VLAN header. > + > +- ``mpls_pop``: pop the outermost MPLS header. > + > + - ``type``: Ethertype of the MPLS header. > + > +- ``mpls_push``: push new MPLS header onto the packet. > + > + - ``type``: Ethertype of the MPLS header. > + > Destroying flow rules > ~~~~~~~~~~~~~~~~~~~~~ > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > index ab181cd83..c5b30363e 100644 > --- a/lib/librte_ether/rte_flow.h > +++ b/lib/librte_ether/rte_flow.h > @@ -1285,6 +1285,41 @@ enum rte_flow_action_type { > * packets that contain required protocol headers. > */ > RTE_FLOW_ACTION_TYPE_TTL_COPY_IN, > + > + /** > + * Pop the outer-most VLAN header from the packet. As for other actions without one, please add: No associated configuration structure. > + */ > + RTE_FLOW_ACTION_TYPE_VLAN_POP, > + > + /** > + * Push a new VLAN header onto the packet, Ethertype 0x8100 or 0x88a8 > + * should be used. The value of VLAN ID and VLAN priority for the new > + * header is copied from an exist VLAN header in the packet, they will > + * be set to 0 if no such VLAN header exist. > + * > + * See struct rte_flow_action_vlan_push. > + */ > + RTE_FLOW_ACTION_TYPE_VLAN_PUSH, > + > + /** > + * Pop the outer-most MPLS header from the packet, Ethertype is > + * required for the resulting packet if it is not the last MPLS > + * header be popped, Ethertype must be 0x8847 or 0x8848. > + * > + * See struct rte_flow_action_mpls_pop. > + */ > + RTE_FLOW_ACTION_TYPE_MPLS_POP, > + > + /** > + * Push a new MPLS header onto the packet, Ethertype 0x8847 or 0x8848 > + * should be used. The value of MPLS label and traffic class is copied > + * from exist outermost MPLS header, and TTL is copied from exist > + * outermost MPLS or IP header, they will be set to 0 if exist packet > + * does not contain required header. > + * > + * See struct rte_flow_action_mpls_push. > + */ > + RTE_FLOW_ACTION_TYPE_MPLS_PUSH, > }; > > /** > @@ -1501,6 +1536,42 @@ struct rte_flow_action_ip_ttl_dec { > }; > > /** > + * RTE_FLOW_ACTION_TYPE_VLAN_PUSH, > + * > + * Push a new VLAN header onto the packet, Ethertype 0x8100 or 0x88a8 > + * should be used. The value of VLAN ID and VLAN priority for the new > + * header is copied from an exist outermost VLAN header in the packet, > + * they will be set to 0 if no such VLAN header exist. > + */ > +struct rte_flow_action_vlan_push { > + rte_be16_t ether_type; /**< Ethertype for vlan tag. */ > +}; > + > +/** > + * RTE_FLOW_ACTION_TYPE_MPLS_POP, > + * > + * Pop the outer-most MPLS header from the packet, Ethertype is > + * required for the resulting packet if it is not the last MPLS > + * header be popped, Ethertype must be 0x8847 or 0x8848. > + */ > +struct rte_flow_action_mpls_pop { > + rte_be16_t ether_type; /**< Ethertype for MPLS header */ > +}; > + > +/** > + * RTE_FLOW_ACTION_TYPE_MPLS_PUSH, > + * > + * Push a new MPLS header onto the packet, Ethertype 0x8847 or 0x8848 > + * should be used. The value of MPLS label and traffic class are copied > + * from exist outermost MPLS header, and TTL is copied from exist outermost > + * MPLS or IP header, they will be set to 0, if exist packet does not > + * contain required header. > + */ > +struct rte_flow_action_mpls_push { > + rte_be16_t ether_type; /**< Ethertype for MPLS header */ > +}; > + > +/** > * Definition of a single action. > * > * A list of actions is terminated by a END action. > -- > 2.13.6 > -- Adrien Mazarguil 6WIND