From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-f178.google.com (mail-wj0-f178.google.com [209.85.210.178]) by dpdk.org (Postfix) with ESMTP id 25474532E for ; Tue, 27 Dec 2016 13:40:16 +0100 (CET) Received: by mail-wj0-f178.google.com with SMTP id c11so112257635wjx.3 for ; Tue, 27 Dec 2016 04:40:16 -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=mpxbwyy6t8ieCmGaomuPtYQiKTb73Py//eJ5NyBo+OA=; b=Lxxzot1UtZ1e0G7TtHIawd/pLgXuIJVvZbssZQmAoczattrzknyWccw/gEMRVpLtM1 k6BLIi3RfHQURrXD3PCF47r2zUht0mWfqMvCsSdQrS/sJTWZB0YRHZLTvjvmFi/3WDs3 wE7WqAfMktTkZAxKAmau7RwbUOrBRnYx/EAjUoVRy2CNVH/vJfo7aJQzPze/nsCzpXNB iDCyk2uQB1bfHjByyfzbfxdva9vyeuWkROsz3CI5BEEwLQFO5VDfuBcn3r0b3Q49vNN0 Kjn6Y7it8ZS/bWeDxov700UCG1VGdtw3CTflpgOAE667XqUy6c6lLp8oZ4sufLYtFV+/ HRJQ== 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=mpxbwyy6t8ieCmGaomuPtYQiKTb73Py//eJ5NyBo+OA=; b=Wf8EFMYbPgJmxeme7MCBbOtTv12lnHBCallkWxJdle6CPwFFL0t+e/1KvxJsMHTyDe +mPg7f/qS2xxa58AJYmy8dzUMsbZzI936pWPnV5W9cQicutabLIjD2rA60NF+nJT8rFX 0k3XlhngIGkqVec6tTxJJeyWR9frVSfJBu+dOVYWWf5nUoQfZh5rFh/hNYa0noKyhUBk K4J7tdIMmMXEWFvrb5UjAZFjWldW1i+1TXOYE1tqroozAurfLu8LQudfbzMRyN+oknwI I6O6g0JrDTbhmT8Q3eMI6hTD/J3c6yp/F4hAxFfnwxlZfl8rQLRCuQKf8usz8+f3Gghq djHQ== X-Gm-Message-State: AIkVDXKoauywbjGz9FgoDCX959oVQtY2qPItKfCq3sUTAhYLE4Q1FzryCv+lkdkbPrVBOWsd X-Received: by 10.194.115.132 with SMTP id jo4mr29218714wjb.27.1482842415807; Tue, 27 Dec 2016 04:40:15 -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 wp2sm21451448wjc.35.2016.12.27.04.40.14 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 27 Dec 2016 04:40:15 -0800 (PST) Date: Tue, 27 Dec 2016 13:40:04 +0100 From: Adrien Mazarguil To: Beilei Xing Cc: jingjing.wu@intel.com, helin.zhang@intel.com, dev@dpdk.org Message-ID: <20161227124004.GA3737@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1482819984-14120-8-git-send-email-beilei.xing@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: Tue, 27 Dec 2016 12:40:16 -0000 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. > + return -EINVAL; While this is perfectly valid, you could also return -rte_errno to avoid duplicating EINVAL. [...] > + } > + 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(). > + 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 [...] > + 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