> -----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 Also the device does not support TCP filter, so we are not able to create a flow to filter TCP packet only. 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? Regards Qi > > -- > Adrien Mazarguil > 6WIND
