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

[Zhang, Qi Z]

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