From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 05A54376C for ; Wed, 28 Dec 2016 10:00:13 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP; 28 Dec 2016 01:00:12 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,421,1477983600"; d="scan'208";a="23630621" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 28 Dec 2016 01:00:11 -0800 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 28 Dec 2016 01:00:07 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 28 Dec 2016 01:00:07 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.177]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.204]) with mapi id 14.03.0248.002; Wed, 28 Dec 2016 17:00:04 +0800 From: "Xing, Beilei" To: Adrien Mazarguil CC: "Wu, Jingjing" , "Zhang, Helin" , "dev@dpdk.org" , "Lu, Wenzhuo" Thread-Topic: [dpdk-dev] [PATCH v2 07/17] net/i40e: add flow validate function Thread-Index: AQHSYAqRciD6rBySQ0ecu+4N0N42baEbNoAAgAHN7WA= Date: Wed, 28 Dec 2016 09:00:03 +0000 Message-ID: <94479800C636CB44BD422CB454846E013158C17B@SHSMSX101.ccr.corp.intel.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> In-Reply-To: <20161227124004.GA3737@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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:00:15 -0000 > -----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 >=20 > Hi Beilei, >=20 > A few comments below. >=20 > 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 >=3D pf->dev_data->nb_rx_queues) { > > + rte_flow_error_set(error, EINVAL, > > + RTE_FLOW_ERROR_TYPE_ACTION, > > + NULL, "Invalid queue ID for" > > + " ethertype_filter."); >=20 > 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 ca= n > be done. OK, I got the meaning and usage of cause pointer now. Thanks for the expla= ination. >=20 > > + return -EINVAL; >=20 > While this is perfectly valid, you could also return -rte_errno to avoid > duplicating EINVAL. Yes, agree. >=20 > [...] > > + } > > + if (ethertype_filter->ether_type =3D=3D ETHER_TYPE_IPv4 || > > + ethertype_filter->ether_type =3D=3D 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 =3D=3D 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."); >=20 > Regarding my previous comment, &attr could replace NULL here as well as i= n > subsequent calls to rte_flow_error_set(). Got it, thanks. >=20 > > + 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 =3D 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 !=3D RTE_FLOW_ITEM_TYPE_END; item++) { > > + item_type =3D item->type; > > + switch (item_type) { > > + case RTE_FLOW_ITEM_TYPE_ETH: > > + eth_spec =3D (const struct rte_flow_item_eth *)item- > >spec; > > + eth_mask =3D (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; > > + } >=20 > While optional, I think you should allow eth_spec and eth_mask to be NULL > here as described in [1]: >=20 > - If eth_spec is NULL, you can match anything that looks like a valid > Ethernet header. >=20 > - If eth_mask is NULL, you should assume a default mask (for Ethernet it > usually means matching source/destination MACs perfectly). >=20 > - You must check the "last" field as well, if non-NULL it may probably be > supported as long as the following condition is satisfied: >=20 > (spec & mask) =3D=3D (last & mask) >=20 > [1] http://dpdk.org/doc/guides/prog_guide/rte_flow.html#pattern-item >=20 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 supp= ort perfect match till now. We will support it in the future, as the deadli= ne is coming, what do you think? > [...] > > + const struct rte_flow_action_queue *act_q; > > + uint32_t index =3D 0; > > + > > + /* Check if the first non-void action is QUEUE or DROP. */ > > + NEXT_ITEM_OF_ACTION(act, actions, index); > > + if (act->type !=3D RTE_FLOW_ACTION_TYPE_QUEUE && > > + act->type !=3D RTE_FLOW_ACTION_TYPE_DROP) { > > + rte_flow_error_set(error, EINVAL, > RTE_FLOW_ERROR_TYPE_ACTION, > > + NULL, "Not supported action."); >=20 > Again, you could report &act instead of NULL here (please check all remai= ning > calls to rte_flow_error_set()). >=20 > [...] >=20 > -- > Adrien Mazarguil > 6WIND