Hi Beilei,
On Wed, Dec 28, 2016 at 09:00:03AM +0000, Xing, Beilei wrote:
>
>
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com]
> > Sent: Tuesday, December 27, 2016 8:40 PM
> > To: Xing, Beilei <beilei.x...@intel.com>
> > Cc: Wu, Jingjing <jingjing...@intel.com>; Zhang, Helin
> > <helin.zh...@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.x...@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
