From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id CD71510B7 for ; Tue, 17 Apr 2018 12:32:57 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Apr 2018 03:32:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,463,1517904000"; d="scan'208";a="192127403" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga004.jf.intel.com with ESMTP; 17 Apr 2018 03:32:56 -0700 Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 17 Apr 2018 03:32:56 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 17 Apr 2018 03:32:55 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.79]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.6]) with mapi id 14.03.0319.002; Tue, 17 Apr 2018 18:32:54 +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 v2 4/4] ether: add packet modification aciton in flow API Thread-Index: AQHT1Yc999EdaimTG0+PJsbVJ3brcKQDa0EQgADH94CAAI+DsA== Date: Tue, 17 Apr 2018 10:32:53 +0000 Message-ID: <039ED4275CED7440929022BC67E7061153199D77@shsmsx102.ccr.corp.intel.com> References: <1522279780-34842-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-5-git-send-email-qi.z.zhang@intel.com> <20180412070343.GO4957@6wind.com> <039ED4275CED7440929022BC67E70611531903F5@SHSMSX103.ccr.corp.intel.com> <20180412102255.GQ4957@6wind.com> <039ED4275CED7440929022BC67E7061153191023@SHSMSX103.ccr.corp.intel.com> <20180416133059.GY4957@6wind.com> <039ED4275CED7440929022BC67E70611531924ED@SHSMSX103.ccr.corp.intel.com> <20180417095519.GE4957@6wind.com> In-Reply-To: <20180417095519.GE4957@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZTNhYzU4MDgtMWE5OS00NWJhLWEzMzUtNjVlODBhOThjNWMwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJ2SGdlVU15XC9TcjdzMWcyUEtZZXpoVXdcL2NwWmd2MWNDRkt1c0EydzlZaWIzV3hxZlNHTU1vUDZVXC9SMWpZOEtuIn0= 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 v2 4/4] ether: add packet modification 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: Tue, 17 Apr 2018 10:32:58 -0000 > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Tuesday, April 17, 2018 5:55 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 v2 4/4] ether: add packet modification aciton in flow= API >=20 > On Mon, Apr 16, 2018 at 03:03:39PM +0000, Zhang, Qi Z wrote: > > > -----Original Message----- > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > > Sent: Monday, April 16, 2018 9:31 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 v2 4/4] ether: add packet modification aciton in > > > flow API > > > > > > On Fri, Apr 13, 2018 at 01:47:15PM +0000, Zhang, Qi Z wrote: > > > > > -----Original Message----- > > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > > > > Sent: Thursday, April 12, 2018 6:23 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 v2 4/4] ether: add packet modification > > > > > aciton in flow API > > > > > > > > > > On Thu, Apr 12, 2018 at 08:50:14AM +0000, Zhang, Qi Z wrote: > > > > > > Hi Adrien > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > > > > > > Sent: Thursday, April 12, 2018 3:04 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 v2 4/4] ether: add packet modification > > > > > > > aciton in flow API > > > > > > > > > > > > > > On Sun, Apr 01, 2018 at 05:19:22PM -0400, Qi Zhang wrote: > > > > > > > > Add new actions that be used to modify packet content with > > > > > > > > generic > > > > > > > > semantic: > > > > > > > > > > > > > > > > RTE_FLOW_ACTION_TYPE_FIELD_UPDATE: > > > > > > > > - update specific field of packet > > > > > > > > RTE_FLWO_ACTION_TYPE_FIELD_INCREMENT: > > > > > > > > - increament specific field of packet > > > > > > > > RTE_FLWO_ACTION_TYPE_FIELD_DECREMENT: > > > > > > > > - decreament specific field of packet > > > > > > > > RTE_FLWO_ACTION_TYPE_FIELD_COPY: > > > > > > > > - copy data from one field to another in packet. > > > > > > > > > > > > > > > > All action use struct rte_flow_item parameter to match the > > > > > > > > pattern that going to be modified, if no pattern match, > > > > > > > > the action just be skipped. > > > > > > > > > > What happens when this action is attempted on non-matching > > > > > > > traffic must be documented here as well. Refer to discussion > > > > > > > re > > > > > > > "ethdev: Add tunnel encap/decap actions" [3]. To be on the > > > > > > > safe side, it must be documented as resulting in undefined > behavior. > > > > > > > > > > > > so what is "undefined behavior" you means? > > > > > > The rule is: > > > > > > If a packet matched pattern in action, it will be modified, > > > > > > otherwise the action just take no effect is this idea acceptabl= e? > > > > > > > > > > Not really, what happens will depend on the underlying device. > > > > > It's better to document it as undefined because you can't > > > > > predict the result. Some devices will cause packets to be lost, > > > > > others will let them through unchanged, others will crash the > > > > > system after formatting > > > the hard drive, no one knows. > > > > > > > > OK, basically I think "undefined behavior" is not friendly to > > > > application, we > > > should avoid. > > > > But you are right, we need to consider device with different behavi= or > for " > > > modification on non-matched pattern" > > > > I'm thinking why driver can't avoid non-matched pattern > > > > modification if the > > > device does not support? > > > > For example, driver can reject a flow ETH/IPV4 with TCP action, > > > > but may accept ETH/IPV4/TCP with TCP action base on its capability. > > > > > > Drivers are free to accept an action or not depending on what is > > > guaranteed to be matched on the pattern side. It's fine as long as > > > the resulting flow rule works exactly as documented. Consistency is > > > much more important to applications than offloads proper. > > > > > > Depending on device capabilities and the importance given to offload > > > specific use cases by vendors, PMD support may range from a basic > > > 1:1 translation attempt between rte_flow and device format, to an > > > all out processing effort resulting in multiple device flow rules > > > and whatnot to satisfy the request by any means necessary (see mlx5 > > > RSS support on empty patterns in case you're curious). > > > > > > Whichever approach you choose (basic or complex), my recommendation > > > is simply to make sure the PMD reports an error whenever a flow rule > > > is ambiguous and could result in unexpected behavior if applied as > > > is to the device. > > > > > > The error message should also be helpful. A message such as "unable > > > to apply flow rule" is pretty useless, while "this action is not > > > supported when X pattern item is not present" actually gives useful > information. > > > > > > > > "Undefined behavior" is for application writers. It means that if a > > > PMD happens to accept the rule in question, what happens isn't > > > covered by documentation. Ideally a PMD shouldn't accept it in the fi= rst > place though. > > > > OK, I'm trying to understand why "Undefined behavior" is necessary for = this > action. > > For example, we have a device that can offload TCP layer modification, > > for packet that contain TCP, packet be modified, for no-tcp packet, > > the packet will be dropped >=20 > Well, this is how rte_flow is defined at a higher level than just the TCP= action > we're talking about: all actions of a flow rule must happen on matched tr= affic > regardless, as guaranteed by the API. Actions do not perform a second rou= nd > of traffic matching, they mindlessly affect it according to their > documentation. >=20 > With that in mind, what should be the end result of a TCP-specific action= on a > packet without a TCP header? >=20 > In your case, the device happens to let traffic through. On mine, traffic= is > dropped. On another, a deadlock occurs in the device. This is why I think > "undefined behavior" is appropriate here. OK, I'm fine to add "undefined behavior" into document. But besides this, would you help to review my v3 patches Thanks Qi >=20 > > Also the device does not support TCP filter, so we are not able to crea= te a > flow to filter TCP packet only. >=20 > OK, now I'm starting to understand your concern. >=20 > > Then without "Undefined behavior", the driver has to reject any flow > > with TCP packet modification action, since it can't guarantee no-tcp pa= cket > not be impacted, while with " Undefined behavior", a flow with tcp action= is > actually is a "rule in question" and it can "happens to" be accepted by t= he > driver? >=20 > I guess expecting applications to rely on PMD-specific (undefined) behavi= or is > out of the question? :) >=20 > The simplest solution to this problem is obviously to document it on an > action basis like you suggested. However doing so puts such actions at od= ds > with the rest of the API and is not recommended. >=20 > So far we took TCP as an example here, but before going further, is there= an > actual scenario in this series where the device is unable to match the > protocol an action will affect? >=20 > For instance, I assume your device supports IPv4/IPv6 matching before > requesting a TTL-decrementing action. If so, can we stay on "undefined > behavior" for when an application doesn't match IPv4/IPv6 first? >=20 > -- > Adrien Mazarguil > 6WIND