> -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > Sent: Tuesday, April 17, 2018 5:55 PM > To: Zhang, Qi Z <qi.z.zh...@intel.com> > Cc: dev@dpdk.org; Doherty, Declan <declan.dohe...@intel.com>; Chandran, > Sugesh <sugesh.chand...@intel.com>; Glynn, Michael J > <michael.j.gl...@intel.com>; Liu, Yu Y <yu.y....@intel.com>; Ananyev, > Konstantin <konstantin.anan...@intel.com>; Richardson, Bruce > <bruce.richard...@intel.com> > Subject: Re: [PATCH v2 4/4] ether: add packet modification aciton in flow API > > On Mon, Apr 16, 2018 at 03:03:39PM +0000, Zhang, Qi Z wrote: > > > -----Original Message----- > > > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > > > Sent: Monday, April 16, 2018 9:31 PM > > > To: Zhang, Qi Z <qi.z.zh...@intel.com> > > > Cc: dev@dpdk.org; Doherty, Declan <declan.dohe...@intel.com>; > > > Chandran, Sugesh <sugesh.chand...@intel.com>; Glynn, Michael J > > > <michael.j.gl...@intel.com>; Liu, Yu Y <yu.y....@intel.com>; > > > Ananyev, Konstantin <konstantin.anan...@intel.com>; Richardson, > > > Bruce <bruce.richard...@intel.com> > > > 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.mazarg...@6wind.com] > > > > > Sent: Thursday, April 12, 2018 6:23 PM > > > > > To: Zhang, Qi Z <qi.z.zh...@intel.com> > > > > > Cc: dev@dpdk.org; Doherty, Declan <declan.dohe...@intel.com>; > > > > > Chandran, Sugesh <sugesh.chand...@intel.com>; Glynn, Michael J > > > > > <michael.j.gl...@intel.com>; Liu, Yu Y <yu.y....@intel.com>; > > > > > Ananyev, Konstantin <konstantin.anan...@intel.com>; Richardson, > > > > > Bruce <bruce.richard...@intel.com> > > > > > 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.mazarg...@6wind.com] > > > > > > > Sent: Thursday, April 12, 2018 3:04 PM > > > > > > > To: Zhang, Qi Z <qi.z.zh...@intel.com> > > > > > > > Cc: dev@dpdk.org; Doherty, Declan > > > > > > > <declan.dohe...@intel.com>; Chandran, Sugesh > > > > > > > <sugesh.chand...@intel.com>; Glynn, Michael J > > > > > > > <michael.j.gl...@intel.com>; Liu, Yu Y <yu.y....@intel.com>; > > > > > > > Ananyev, Konstantin <konstantin.anan...@intel.com>; > > > > > > > Richardson, Bruce <bruce.richard...@intel.com> > > > > > > > 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. > > > <snip> > > > > > > > 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 acceptable? > > > > > > > > > > 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 behavior > 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 first > 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 > > 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 traffic > regardless, as guaranteed by the API. Actions do not perform a second round > of traffic matching, they mindlessly affect it according to their > documentation. > > With that in mind, what should be the end result of a TCP-specific action on a > packet without a TCP header? > > 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 > > > Also the device does not support TCP filter, so we are not able to create a > flow to filter TCP packet only. > > OK, now I'm starting to understand your concern. > > > Then without "Undefined behavior", the driver has to reject any flow > > with TCP packet modification action, since it can't guarantee no-tcp packet > 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 the > driver? > > I guess expecting applications to rely on PMD-specific (undefined) behavior is > out of the question? :) > > 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 odds > with the rest of the API and is not recommended. > > 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? > > 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? > > -- > Adrien Mazarguil > 6WIND
