From: "Zhang, Qi Z" <qi.z.zhang@intel.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: "dev@dpdk.org" <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: [dpdk-dev] [PATCH v2 4/4] ether: add packet modification aciton in flow API
Date: Tue, 17 Apr 2018 10:32:53 +0000 [thread overview]
Message-ID: <039ED4275CED7440929022BC67E7061153199D77@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <20180417095519.GE4957@6wind.com>
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Tuesday, April 17, 2018 5:55 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 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 <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
>
> 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
next prev parent reply other threads:[~2018-04-17 10:32 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-28 23:29 [dpdk-dev] [PATCH 0/4] rte_flow extension for vSwitch acceleration Qi Zhang
2018-03-28 23:29 ` [dpdk-dev] [PATCH 1/4] ether: add flow action to redirect packet in a switch domain Qi Zhang
2018-03-29 10:48 ` Pattan, Reshma
2018-03-29 11:20 ` Pattan, Reshma
2018-03-28 23:29 ` [dpdk-dev] [PATCH 2/4] ether: add flow last hit query support Qi Zhang
2018-03-28 23:29 ` [dpdk-dev] [PATCH 3/4] ether: add more protocol support in flow API Qi Zhang
2018-03-29 14:32 ` Pattan, Reshma
2018-03-28 23:29 ` [dpdk-dev] [PATCH 4/4] ether: add packet modification aciton " Qi Zhang
2018-03-29 15:23 ` Pattan, Reshma
2018-04-02 3:35 ` Zhang, Qi Z
2018-04-01 21:19 ` [dpdk-dev] [PATCH v2 0/4] rte_flow extension for vSwitch acceleration Qi Zhang
2018-04-01 21:19 ` [dpdk-dev] [PATCH v2 1/4] ether: add flow action to redirect packet to a port Qi Zhang
2018-04-11 16:30 ` Adrien Mazarguil
2018-04-01 21:19 ` [dpdk-dev] [PATCH v2 2/4] ether: add flow last hit query support Qi Zhang
2018-04-11 16:31 ` Adrien Mazarguil
2018-04-01 21:19 ` [dpdk-dev] [PATCH v2 3/4] ether: add more protocol support in flow API Qi Zhang
2018-04-11 16:32 ` Adrien Mazarguil
2018-04-12 5:12 ` Zhang, Qi Z
2018-04-12 9:19 ` Adrien Mazarguil
2018-04-12 10:00 ` Zhang, Qi Z
2018-04-01 21:19 ` [dpdk-dev] [PATCH v2 4/4] ether: add packet modification aciton " Qi Zhang
2018-04-12 7:03 ` Adrien Mazarguil
2018-04-12 8:50 ` Zhang, Qi Z
2018-04-12 10:22 ` Adrien Mazarguil
2018-04-13 13:47 ` Zhang, Qi Z
2018-04-16 13:30 ` Adrien Mazarguil
2018-04-16 15:03 ` Zhang, Qi Z
2018-04-17 9:55 ` Adrien Mazarguil
2018-04-17 10:32 ` Zhang, Qi Z [this message]
2018-04-10 14:12 ` [dpdk-dev] [PATCH v2 0/4] rte_flow extension for vSwitch acceleration Thomas Monjalon
2018-04-16 5:16 ` [dpdk-dev] [PATCH v3 " Qi Zhang
2018-04-16 5:16 ` [dpdk-dev] [PATCH v3 1/4] ethdev: add more protocol support in flow API Qi Zhang
2018-04-16 5:16 ` [dpdk-dev] [PATCH v3 2/4] ethdev: add packet field set aciton " Qi Zhang
2018-04-16 5:16 ` [dpdk-dev] [PATCH v3 3/4] ethdev: add TTL change actions " Qi Zhang
2018-04-16 5:48 ` Shahaf Shuler
2018-04-16 8:12 ` Adrien Mazarguil
2018-04-16 8:56 ` Shahaf Shuler
2018-04-16 9:59 ` Adrien Mazarguil
2018-04-16 5:16 ` [dpdk-dev] [PATCH v3 4/4] ethdev: add VLAN and MPLS pop push action " Qi Zhang
2018-04-16 6:10 ` [dpdk-dev] [PATCH v3 0/4] rte_flow extension for vSwitch acceleration Qi Zhang
2018-04-16 6:10 ` [dpdk-dev] [PATCH v3 1/4] ethdev: add more protocol support in flow API Qi Zhang
2018-04-19 14:48 ` Adrien Mazarguil
2018-04-16 6:10 ` [dpdk-dev] [PATCH v3 2/4] ethdev: add packet field set aciton " Qi Zhang
2018-04-19 14:48 ` Adrien Mazarguil
2018-04-20 2:24 ` Zhang, Qi Z
2018-04-20 8:54 ` Adrien Mazarguil
2018-04-16 6:10 ` [dpdk-dev] [PATCH v3 3/4] ethdev: add TTL change actions " Qi Zhang
2018-04-19 14:48 ` Adrien Mazarguil
2018-04-16 6:10 ` [dpdk-dev] [PATCH v3 4/4] ethdev: add VLAN and MPLS pop push action " Qi Zhang
2018-04-17 7:34 ` Shahaf Shuler
2018-04-19 14:49 ` Adrien Mazarguil
2018-04-23 6:36 ` [dpdk-dev] [PATCH v4 0/3] rte_flow extension for vSwitch acceleration Qi Zhang
2018-04-23 6:36 ` [dpdk-dev] [PATCH v4 1/3] ethdev: add more protocol support in flow API Qi Zhang
2018-04-23 6:36 ` [dpdk-dev] [PATCH v4 2/3] ethdev: add TTL change actions " Qi Zhang
2018-04-23 6:36 ` [dpdk-dev] [PATCH v4 3/3] ethdev: add VLAN and MPLS " Qi Zhang
2018-04-24 15:58 ` [dpdk-dev] [PATCH v5 0/3] rte_flow extension for vSwitch acceleration Adrien Mazarguil
2018-04-24 15:58 ` [dpdk-dev] [PATCH v5 1/3] ethdev: add neighbor discovery support to flow API Adrien Mazarguil
2018-04-24 15:59 ` [dpdk-dev] [PATCH v5 2/3] ethdev: add TTL change actions " Adrien Mazarguil
2018-04-24 15:59 ` [dpdk-dev] [PATCH v5 3/3] ethdev: add VLAN and MPLS " Adrien Mazarguil
2018-04-25 13:54 ` [dpdk-dev] [PATCH v5 0/3] rte_flow extension for vSwitch acceleration Zhang, Qi Z
2018-04-25 22:44 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=039ED4275CED7440929022BC67E7061153199D77@shsmsx102.ccr.corp.intel.com \
--to=qi.z.zhang@intel.com \
--cc=adrien.mazarguil@6wind.com \
--cc=bruce.richardson@intel.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=konstantin.ananyev@intel.com \
--cc=michael.j.glynn@intel.com \
--cc=sugesh.chandran@intel.com \
--cc=yu.y.liu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).