Re: [dpdk-dev] [PATCH v2 07/17] net/i40e: add flow validate function

["Xing, Beilei" <beilei.xing@intel.com>]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, December 28, 2016 5:30 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 07/17] net/i40e: add flow validate
> function
> 
> 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 <beilei.xing@intel.com>
> > > Cc: Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin
> > > <helin.zhang@intel.com>; 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 <beilei.xing@intel.com>
> > > > ---
> > > >  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).

OK, will update the non-NULL condition in next version.
And thanks for all your comments.

> 
> 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