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

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

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

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