From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id DF7807D0A for ; Fri, 20 Apr 2018 04:24:24 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Apr 2018 19:24:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,299,1520924400"; d="scan'208";a="38914396" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga002.fm.intel.com with ESMTP; 19 Apr 2018 19:24:23 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 19 Apr 2018 19:24:23 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 19 Apr 2018 19:24:22 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.210]) by shsmsx102.ccr.corp.intel.com ([169.254.2.79]) with mapi id 14.03.0319.002; Fri, 20 Apr 2018 10:24:20 +0800 From: "Zhang, Qi Z" To: Adrien Mazarguil CC: "dev@dpdk.org" , "Doherty, Declan" , "Chandran, Sugesh" , "Glynn, Michael J" , "Liu, Yu Y" , "Ananyev, Konstantin" , "Richardson, Bruce" Thread-Topic: [PATCH v3 2/4] ethdev: add packet field set aciton in flow API Thread-Index: AQHT1UmwuIoR18aYA0qsSfTJzW6616QHqlaAgAFBxNA= Date: Fri, 20 Apr 2018 02:24:20 +0000 Message-ID: <039ED4275CED7440929022BC67E70611531A3BE2@SHSMSX103.ccr.corp.intel.com> References: <1522279780-34842-1-git-send-email-qi.z.zhang@intel.com> <20180416061042.785-1-qi.z.zhang@intel.com> <20180416061042.785-3-qi.z.zhang@intel.com> <20180419144843.GF4957@6wind.com> In-Reply-To: <20180419144843.GF4957@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNWYzNTI4N2EtMzZhZC00Y2YzLThmM2EtNDNmYTI5NDJjZGIzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiI4SnIySngxTEZ3XC9xOWoza0REUmFSenRKdXJ4TXZ4MnN6eEVoMGppS2M1cWxYTjZRa0hcLzZab3NVVjZ5Q1BzY1YifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: add packet field set aciton 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: Fri, 20 Apr 2018 02:24:25 -0000 Hi Adrien: > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Thursday, April 19, 2018 10:49 PM > To: Zhang, Qi Z > Cc: dev@dpdk.org; Doherty, Declan ; Chandran, > Sugesh ; Glynn, Michael J > ; Liu, Yu Y ; Ananyev, > Konstantin ; Richardson, Bruce > > Subject: Re: [PATCH v3 2/4] ethdev: add packet field set aciton in flow A= PI >=20 > Typo in commit title: aciton =3D> action >=20 > On Mon, Apr 16, 2018 at 02:10:40PM +0800, Qi Zhang wrote: > > Add new action RTE_FLOW_ACTION_TYPE_FIELD_SET, it is used to modify > > fields of specific protocol layer of the packet, the action only apply > > on packets that contain the requireds protocol layer. >=20 > requireds =3D> required >=20 > > > > Signed-off-by: Qi Zhang >=20 > (more below) >=20 > > --- > > doc/guides/prog_guide/rte_flow.rst | 30 > +++++++++++++++++++++++++++ > > lib/librte_ether/rte_flow.h | 42 > +++++++++++++++++++++++++++++++++++++- > > 2 files changed, 71 insertions(+), 1 deletion(-) > > > > diff --git a/doc/guides/prog_guide/rte_flow.rst > > b/doc/guides/prog_guide/rte_flow.rst > > index 99468bf60..68deb9812 100644 > > --- a/doc/guides/prog_guide/rte_flow.rst > > +++ b/doc/guides/prog_guide/rte_flow.rst > > @@ -1574,6 +1574,36 @@ fields in the pattern items. > > | 1 | END | > > +-------+----------+ > > > > +Action: ``FILED_SET`` > > +^^^^^^^^^^^^^^^^^^^^^ >=20 > FILED_SET =3D> FIELD_SET >=20 > > + > > +Modify the value of fields in a protocol layer, only applies to > > +packets that contain respective protocol layer. > > + > > +.. _table_rte_flow_action_field_set: > > + > > +.. table:: FIELD_SET > > + > > + +---------------+--------------------------------------------------= -----------------+ > > + | Field | Value > | > > + > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D+=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D+ > > + | ``type`` | Specify the type of a protocol layer. (see > RTE_FLOW_ITEM_TYPE_*) | > > + +---------------+--------------------------------------------------= -----------------+ > > + | ``dir_level`` | Specify the level of matched protocol layer. > | > > + | | direction (1b) > | > > + | | 0: match start from outermost. > | > > + | | 1: match start from innermost. > | >=20 > Please remove the direction part. What devices can match is always > outermost up to the point where they can't recognize an inner header. > "innermost" is almost guaranteed to never have the desired effect. >=20 > > + | | level: (31b) > | > > + | | 0: outermost or innermost protocol layer that > matched @type | > > + | | 1: next to outmost or innermost protocol layer > that matched @type | > > + | | 2: and so on ... > | >=20 > Then you can remove any reference to dir_level from here. >=20 > > + +---------------+--------------------------------------------------= -----------------+ > > + | ``new_val`` | Pointer to specific data structure according to > protocol type, | > > + | | the content is the new value to updtae. > | >=20 > updtae =3D> update >=20 > > + +---------------+--------------------------------------------------= -----------------+ > > + | ``mask`` | Bit-mask applied to new_val > | > > + > > + +---------------+--------------------------------------------------- > > + ----------------+ > > + > > Negative types > > ~~~~~~~~~~~~~~ > > > > diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h > > index f84bbfda5..2dc95b6b8 100644 > > --- a/lib/librte_ether/rte_flow.h > > +++ b/lib/librte_ether/rte_flow.h > > @@ -1245,7 +1245,15 @@ enum rte_flow_action_type { > > * > > * See struct rte_flow_action_security. > > */ > > - RTE_FLOW_ACTION_TYPE_SECURITY > > + RTE_FLOW_ACTION_TYPE_SECURITY, > > + > > + /** > > + * Modify the value of fields in a protocol layer, only applies to > > + * packets that contain respective protocol layer. > > + * > > + * See struct rte_flow_action_field_set. > > + */ > > + RTE_FLOW_ACTION_TYPE_FIELD_SET, > > }; > > > > /** > > @@ -1384,6 +1392,38 @@ struct rte_flow_action_security { }; > > > > /** > > + * RTE_FLOW_ACTION_TYPE_FIELD_SET > > + * > > + * Modify the value of fields in a protocol layer, only applies to > > + * packets that contain respective protocol layer. > > + */ > > +struct rte_flow_action_field_set { > > + /** > > + * Specify the type of a protocol layer. > > + */ > > + enum rte_flow_item_type type; > > + /** > > + * Specify the level of matched protocol layer. > > + * > > + * direction (1b) > > + * 0: match start from outermost. > > + * 1: match start from innermost. > > + * > > + * level (31b) > > + * 0: outermost|innermost protocol layer that matched @type. > > + * 1: next to outermost|innermost protocol layer that matched @type. > > + * 2: and so on ... > > + */ > > + uint32_t dir_level; >=20 > See above regarding this field. >=20 > > + /** > > + * Pointer to specific data structure according to protocol type, > > + * the content is the new value to update. > > + */ > > + const void *new_val; > > + const void *mask; /**< Bit-mask applied to new_val. */ }; > > + > > +/** > > * Definition of a single action. > > * > > * A list of actions is terminated by a END action. > > -- > > 2.13.6 > > >=20 > Testpmd implementation and documentation update are also missing, > however > I'm still not convinced by the definition of this new action, it seems to= o > generic to be useful (e.g. compare this with a dedicated "update destinat= ion > IPv4 address" action for instance). >=20 > What existing HW capabilities do you intend to expose through this, what > kind of fields can be updated at this point? For our device, there will be more than 20 actions if we create an action f= or each field like "RTE_FLOW_ACTION_TYPE_IPV4_ADDR_SET", More detail, that will cover fields in IPV4/IPV6/Ether/ICMP/ND/ARP, so I th= ink a generic field set action would be better. For testpmd support, Seems there is no reference to enable an action with v= oid parameters, so it may take me time to figure out a solution, I'm not sure I can capture this on the 18.05, so is that possible we just d= eferred testpmd support for this action? since I saw action like rte_flow_a= ction_security also don't have testpmd support yet. Thanks Qi >=20 > If it's still unclear, I suggest to remove this patch from the series or = at > the very least mark it as experimental. You can even provide a forward > declaration without the contents of struct rte_flow_action_field_set to > prevent applications from using it before it's finalized. >=20 > -- > Adrien Mazarguil > 6WIND