From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f43.google.com (mail-wm0-f43.google.com [74.125.82.43]) by dpdk.org (Postfix) with ESMTP id 7FDCA3777 for ; Wed, 28 Dec 2016 10:29:54 +0100 (CET) Received: by mail-wm0-f43.google.com with SMTP id k184so109042321wme.1 for ; Wed, 28 Dec 2016 01:29:54 -0800 (PST) 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=hGVmLxOMo3kKdHIQLZf/GtWzK/eFVXYiMOTUBttAP9k=; b=Rl7K+91iLFN1piqt/CxQhKp+8tTcwzYpcm8nfHkKq/UiY1CLA166MN47m/ewzSVQHY 2pT2NlQF6jTSx+JaNhvA7tpT8Wgk7epRqtsJ2NhL/rHO1kwVB9nLSbg8eywiZK1aiVCa KMezASDf/JoddfqIQvrYNia3ZmQWvPzhexF6lOrjIGmxNXW3b5LF7Jf+x3TcYPC9qEhL wF7f3nT4YNq7VDJpP9AJt3mVLtdEPFrz3pyTXjgV2T5cJlwuZHv8MG5uUomwllIoVYNM ZrT3JnVIIqerwsy8oAi/4V34A1pIfYlamHUpAcPRmPzJFDXykzO7Z5kg7kGSEqgY7gUb Sg9g== 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=hGVmLxOMo3kKdHIQLZf/GtWzK/eFVXYiMOTUBttAP9k=; b=oJ+Fug09K2KGpFpN7sEp7f1OQ507FzNgS74U4xSrJ0oxj084gEl4phUijOmHDHqR2M TWZaSBSmWf4NI7Q/rQjbzrSKRKBPeB+3TZL2cbRQxHVOS5761OVbs1BGjg4iSFfAlJTw +wwXCYexcKi0fIZDFNOZuhJ7dnCHESNHFuFd9CUZPWl8XxlLyCIV1pLmbBTmXKYZt/8G v+gCgE89RC7zOoO/OlSj1NKgZ4xOsixViiWRlymlnxZMLagB2SD5YOLGGW0SmM6omN4t Vu7Y4upTCVsR7BcFUwAGlok8/HxOdqmDQ1w43YOG/qLfmEyXcRxj468h7IL9wuRAtVZ/ NlmQ== X-Gm-Message-State: AIkVDXIAL/5meX870kaa/L9YyskQxQOC4kHBB0G8/HcY4exNxzDpZDU9YQk/1vkMMBCs5i8P X-Received: by 10.28.19.196 with SMTP id 187mr34302541wmt.81.1482917394224; Wed, 28 Dec 2016 01:29:54 -0800 (PST) Received: from 6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id d184sm60202601wmd.8.2016.12.28.01.29.52 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 28 Dec 2016 01:29:53 -0800 (PST) Date: Wed, 28 Dec 2016 10:29:45 +0100 From: Adrien Mazarguil To: "Xing, Beilei" Cc: "Wu, Jingjing" , "Zhang, Helin" , "dev@dpdk.org" , "Lu, Wenzhuo" Message-ID: <20161228092945.GF3737@6wind.com> References: <1480679625-4157-1-git-send-email-beilei.xing@intel.com> <1482819984-14120-1-git-send-email-beilei.xing@intel.com> <1482819984-14120-8-git-send-email-beilei.xing@intel.com> <20161227124004.GA3737@6wind.com> <94479800C636CB44BD422CB454846E013158C17B@SHSMSX101.ccr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <94479800C636CB44BD422CB454846E013158C17B@SHSMSX101.ccr.corp.intel.com> Subject: Re: [dpdk-dev] [PATCH v2 07/17] net/i40e: add flow validate function 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: Wed, 28 Dec 2016 09:29:54 -0000 Hi Beilei, On Wed, Dec 28, 2016 at 09:00:03AM +0000, Xing, Beilei wrote: > > > > -----Original Message----- > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > > Sent: Tuesday, December 27, 2016 8:40 PM > > To: Xing, Beilei > > Cc: Wu, Jingjing ; Zhang, Helin > > ; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v2 07/17] net/i40e: add flow validate > > function > > > > Hi Beilei, > > > > A few comments below. > > > > On Tue, Dec 27, 2016 at 02:26:14PM +0800, Beilei Xing wrote: > > > This patch adds i40e_flow_validation function to check if a flow is > > > valid according to the flow pattern. > > > i40e_parse_ethertype_filter is added first, it also gets the ethertype > > > info. > > > i40e_flow.c is added to handle all generic filter events. > > > > > > Signed-off-by: Beilei Xing > > > --- > > > drivers/net/i40e/Makefile | 1 + > > > drivers/net/i40e/i40e_ethdev.c | 5 + > > > drivers/net/i40e/i40e_ethdev.h | 20 ++ > > > drivers/net/i40e/i40e_flow.c | 431 > > +++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 457 insertions(+) > > > create mode 100644 drivers/net/i40e/i40e_flow.c > > [...] > > > diff --git a/drivers/net/i40e/i40e_flow.c > > > b/drivers/net/i40e/i40e_flow.c new file mode 100644 index > > > 0000000..bf451ef > > > --- /dev/null > > > +++ b/drivers/net/i40e/i40e_flow.c > > [...] > > > + if (ethertype_filter->queue >= pf->dev_data->nb_rx_queues) { > > > + rte_flow_error_set(error, EINVAL, > > > + RTE_FLOW_ERROR_TYPE_ACTION, > > > + NULL, "Invalid queue ID for" > > > + " ethertype_filter."); > > > > When setting an error type related to an existing object provided by the > > application, you should set the related cause pointer to a non-NULL value. In > > this particular case, retrieving the action object seems difficult so it can > > remain that way, however there are many places in this series where it can > > be done. > > OK, I got the meaning and usage of cause pointer now. Thanks for the explaination. > > > > > > + return -EINVAL; > > > > While this is perfectly valid, you could also return -rte_errno to avoid > > duplicating EINVAL. > > Yes, agree. > > > > > [...] > > > + } > > > + if (ethertype_filter->ether_type == ETHER_TYPE_IPv4 || > > > + ethertype_filter->ether_type == ETHER_TYPE_IPv6) { > > > + rte_flow_error_set(error, ENOTSUP, > > > + RTE_FLOW_ERROR_TYPE_ITEM, > > > + NULL, "Unsupported ether_type in" > > > + " control packet filter."); > > > + return -ENOTSUP; > > > + } > > > + if (ethertype_filter->ether_type == ETHER_TYPE_VLAN) > > > + PMD_DRV_LOG(WARNING, "filter vlan ether_type in" > > > + " first tag is not supported."); > > > + > > > + return ret; > > > +} > > [...] > > > +/* Parse attributes */ > > > +static int > > > +i40e_parse_attr(const struct rte_flow_attr *attr, > > > + struct rte_flow_error *error) > > > +{ > > > + /* Must be input direction */ > > > + if (!attr->ingress) { > > > + rte_flow_error_set(error, EINVAL, > > > + RTE_FLOW_ERROR_TYPE_ATTR_INGRESS, > > > + NULL, "Only support ingress."); > > > > Regarding my previous comment, &attr could replace NULL here as well as in > > subsequent calls to rte_flow_error_set(). > > Got it, thanks. > > > > > > + return -EINVAL; > > > + } > > > + > > > + /* Not supported */ > > > + if (attr->egress) { > > > + rte_flow_error_set(error, EINVAL, > > > + RTE_FLOW_ERROR_TYPE_ATTR_EGRESS, > > > + NULL, "Not support egress."); > > > + return -EINVAL; > > > + } > > > + > > > + /* Not supported */ > > > + if (attr->priority) { > > > + rte_flow_error_set(error, EINVAL, > > > + RTE_FLOW_ERROR_TYPE_ATTR_PRIORITY, > > > + NULL, "Not support priority."); > > > + return -EINVAL; > > > + } > > > + > > > + /* Not supported */ > > > + if (attr->group) { > > > + rte_flow_error_set(error, EINVAL, > > > + RTE_FLOW_ERROR_TYPE_ATTR_GROUP, > > > + NULL, "Not support group."); > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int > > > +i40e_parse_ethertype_pattern(const struct rte_flow_item *pattern, > > > + struct rte_flow_error *error, > > > + struct rte_eth_ethertype_filter *filter) { > > > + const struct rte_flow_item *item = pattern; > > > + const struct rte_flow_item_eth *eth_spec; > > > + const struct rte_flow_item_eth *eth_mask; > > > + enum rte_flow_item_type item_type; > > > + > > > + for (; item->type != RTE_FLOW_ITEM_TYPE_END; item++) { > > > + item_type = item->type; > > > + switch (item_type) { > > > + case RTE_FLOW_ITEM_TYPE_ETH: > > > + eth_spec = (const struct rte_flow_item_eth *)item- > > >spec; > > > + eth_mask = (const struct rte_flow_item_eth *)item- > > >mask; > > > + /* Get the MAC info. */ > > > + if (!eth_spec || !eth_mask) { > > > + rte_flow_error_set(error, EINVAL, > > > + > > RTE_FLOW_ERROR_TYPE_ITEM, > > > + NULL, > > > + "NULL ETH spec/mask"); > > > + return -EINVAL; > > > + } > > > > While optional, I think you should allow eth_spec and eth_mask to be NULL > > here as described in [1]: > > > > - If eth_spec is NULL, you can match anything that looks like a valid > > Ethernet header. > > > > - If eth_mask is NULL, you should assume a default mask (for Ethernet it > > usually means matching source/destination MACs perfectly). > > > > - You must check the "last" field as well, if non-NULL it may probably be > > supported as long as the following condition is satisfied: > > > > (spec & mask) == (last & mask) > > > > [1] http://dpdk.org/doc/guides/prog_guide/rte_flow.html#pattern-item > > > > Thanks for the specification. In fact, we don't support the "last" for both ixgbe and i40e currently according to the original design, so we only support perfect match till now. We will support it in the future, as the deadline is coming, what do you think? If you want to handle it later it's fine, however in that case you need to at least generate an error when last is non-NULL (I did not see such a check in this code). Note that supporting it properly as defined in the API could be relatively easy by implementing the above condition, it's just a small step above simply checking for a NULL value. > > [...] > > > + const struct rte_flow_action_queue *act_q; > > > + uint32_t index = 0; > > > + > > > + /* Check if the first non-void action is QUEUE or DROP. */ > > > + NEXT_ITEM_OF_ACTION(act, actions, index); > > > + if (act->type != RTE_FLOW_ACTION_TYPE_QUEUE && > > > + act->type != RTE_FLOW_ACTION_TYPE_DROP) { > > > + rte_flow_error_set(error, EINVAL, > > RTE_FLOW_ERROR_TYPE_ACTION, > > > + NULL, "Not supported action."); > > > > Again, you could report &act instead of NULL here (please check all remaining > > calls to rte_flow_error_set()). > > > > [...] > > > > -- > > Adrien Mazarguil > > 6WIND -- Adrien Mazarguil 6WIND