From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Xing, Beilei" Subject: Re: [PATCH v2 07/17] net/i40e: add flow validate function 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "Wu, Jingjing" , "Zhang, Helin" , "dev@dpdk.org" , "Lu, Wenzhuo" To: Adrien Mazarguil 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) In-Reply-To: <20161227124004.GA3737@6wind.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----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