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 10:03:14 +0000 Message-ID: <94479800C636CB44BD422CB454846E013158C21C@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> <94479800C636CB44BD422CB454846E013158C17B@SHSMSX101.ccr.corp.intel.com> <20161228092945.GF3737@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 AD4FC37A8 for ; Wed, 28 Dec 2016 11:03:20 +0100 (CET) In-Reply-To: <20161228092945.GF3737@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: Wednesday, December 28, 2016 5:30 PM > To: Xing, Beilei > Cc: Wu, Jingjing ; Zhang, Helin > ; dev@dpdk.org; Lu, Wenzhuo > > Subject: Re: [dpdk-dev] [PATCH v2 07/17] net/i40e: add flow validate > function >=20 > Hi Beilei, >=20 > 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 >=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."); > > > > > > 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 =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."); > > > > > > 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 =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; > > > > + } > > > > > > 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 probabl= y be > > > supported as long as the following condition is satisfied: > > > > > > (spec & mask) =3D=3D (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 sup= port > perfect match till now. We will support it in the future, as the deadline= is > coming, what do you think? >=20 > If you want to handle it later it's fine, however in that case you need t= o at > least generate an error when last is non-NULL (I did not see such a check= in > this code). OK, will update the non-NULL condition in next version. And thanks for all your comments. >=20 > Note that supporting it properly as defined in the API could be relativel= y easy > by implementing the above condition, it's just a small step above simply > checking for a NULL value. >=20 > > > [...] > > > > + 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."); > > > > > > Again, you could report &act instead of NULL here (please check all > > > remaining calls to rte_flow_error_set()). > > > > > > [...] > > > > > > -- > > > Adrien Mazarguil > > > 6WIND >=20 > -- > Adrien Mazarguil > 6WIND