From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f179.google.com (mail-wr0-f179.google.com [209.85.128.179]) by dpdk.org (Postfix) with ESMTP id ACF00A48F for ; Tue, 17 Apr 2018 11:55:33 +0200 (CEST) Received: by mail-wr0-f179.google.com with SMTP id q13so30307606wre.3 for ; Tue, 17 Apr 2018 02:55:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=mkdNvenbH3ooRXuPTOwdKgQ/iZ4W324JIQ2oJlLz9Io=; b=If0qqtVvV6wKCy2hlBUNXK6k4m/krMSIXEo/kJZkU3ynK7cQw0DQV6zYk0OJxCI2eG J36wKZQc5LJxeFDsl4dgy+JFO7hiFj46GU12FXV1qHf7n1YedIURuU6Tb99cowWMJ2zs juEbFnrKNQ3ujuqFtYKNozwBqZ0bWm4WaU5k9Q36vuUxLuZCRbVrouIwSrcoOtfSXAWv c76rOrDHeqUpYEMHp3/M8i81HcuHS/2szj9oqlASnQR97EDBD/206XHuIqpbHSlhdkiQ 4An9p9CfymXAYMkLeMf4wAauYLziCQzZZ6DTn6BHUS4a9LSeS30Ires4tydG9lmZ7b55 nYdA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=mkdNvenbH3ooRXuPTOwdKgQ/iZ4W324JIQ2oJlLz9Io=; b=VJDw7CvIppa1RlqufyUrQD9JeavgYoHR5USiYdAsmeAWOGO1TmeSGwQtdaNMk64rMs B4/UaEn0FaWGzfVA5I9V9eaVXsGsFT35WeyEQSOTMJVu0IyyVx4f/2yA0OsQhoAfuue+ /AkRc12GhQEwMBI7zl448yEH6R/aSarRf5pmdnXAGyQkZ3DRD2TUhhx+gmdsdUAaqC/2 10dH43QPuKxJis/Dfa1F5vIISLS92Hra3NBfOQ7rBH5ZhxB/FXSwu7EqikR6G7oxRc+n n5xvA7Yqhm5I8yLKzIDvVq738J5vv44fijXsN9CXjGd7i9tvtjWZlpbhl1cf/eXrh2ZU 5JSQ== X-Gm-Message-State: ALQs6tCIOckQRuPCqIeBNUZZRMXNcAF7NisGgogrK8uW4m1/UEvIvmr0 rVYURC424KC1wyDZpgPBuDqENw== X-Google-Smtp-Source: AIpwx49uCrx8bL0DQUD6IeDAFuQamN+Ku2SCrImtO1u0j5hMxw9987F8uj3b2N8Ty/P7WK6dS6AiUQ== X-Received: by 10.28.145.139 with SMTP id t133mr472741wmd.138.1523958933381; Tue, 17 Apr 2018 02:55:33 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id v75sm224877wrc.65.2018.04.17.02.55.32 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 17 Apr 2018 02:55:32 -0700 (PDT) Date: Tue, 17 Apr 2018 11:55:19 +0200 From: Adrien Mazarguil To: "Zhang, Qi Z" Cc: "dev@dpdk.org" , "Doherty, Declan" , "Chandran, Sugesh" , "Glynn, Michael J" , "Liu, Yu Y" , "Ananyev, Konstantin" , "Richardson, Bruce" Message-ID: <20180417095519.GE4957@6wind.com> References: <1522279780-34842-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-1-git-send-email-qi.z.zhang@intel.com> <1522617562-25940-5-git-send-email-qi.z.zhang@intel.com> <20180412070343.GO4957@6wind.com> <039ED4275CED7440929022BC67E70611531903F5@SHSMSX103.ccr.corp.intel.com> <20180412102255.GQ4957@6wind.com> <039ED4275CED7440929022BC67E7061153191023@SHSMSX103.ccr.corp.intel.com> <20180416133059.GY4957@6wind.com> <039ED4275CED7440929022BC67E70611531924ED@SHSMSX103.ccr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <039ED4275CED7440929022BC67E70611531924ED@SHSMSX103.ccr.corp.intel.com> Subject: Re: [dpdk-dev] [PATCH v2 4/4] ether: add packet modification aciton in flow API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Apr 2018 09:55:33 -0000 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 > > Cc: dev@dpdk.org; Doherty, Declan ; Chandran, > > Sugesh ; Glynn, Michael J > > ; Liu, Yu Y ; Ananyev, > > Konstantin ; Richardson, Bruce > > > > 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 > > > > Cc: dev@dpdk.org; Doherty, Declan ; > > > > Chandran, Sugesh ; Glynn, Michael J > > > > ; Liu, Yu Y ; > > > > Ananyev, Konstantin ; Richardson, > > > > Bruce > > > > 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 > > > > > > Cc: dev@dpdk.org; Doherty, Declan ; > > > > > > Chandran, Sugesh ; Glynn, Michael J > > > > > > ; Liu, Yu Y ; > > > > > > Ananyev, Konstantin ; Richardson, > > > > > > Bruce > > > > > > 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. > > > > > > > > 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. > 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