Re: [dpdk-dev] [PATCH v2 4/4] ether: add packet modification aciton in flow API

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Monday, April 16, 2018 9:31 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Chandran,
> Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@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.mazarguil@6wind.com]
> > > Sent: Thursday, April 12, 2018 6:23 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > Bruce <bruce.richardson@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.mazarguil@6wind.com]
> > > > > Sent: Thursday, April 12, 2018 3:04 PM
> > > > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>;
> > > > > Chandran, Sugesh <sugesh.chandran@intel.com>; Glynn, Michael J
> > > > > <michael.j.glynn@intel.com>; Liu, Yu Y <yu.y.liu@intel.com>;
> > > > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; Richardson,
> > > > > Bruce <bruce.richardson@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