From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <beilei.xing@intel.com>
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
by dpdk.org (Postfix) with ESMTP id 05A54376C
for <dev@dpdk.org>; Wed, 28 Dec 2016 10:00:13 +0100 (CET)
Received: from orsmga002.jf.intel.com ([10.7.209.21])
by orsmga103.jf.intel.com with ESMTP; 28 Dec 2016 01:00:12 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.33,421,1477983600"; d="scan'208";a="23630621"
Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203])
by orsmga002.jf.intel.com with ESMTP; 28 Dec 2016 01:00:11 -0800
Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by
FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS)
id 14.3.248.2; Wed, 28 Dec 2016 01:00:07 -0800
Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by
fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS)
id 14.3.248.2; Wed, 28 Dec 2016 01:00:07 -0800
Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.177]) by
SHSMSX151.ccr.corp.intel.com ([169.254.3.204]) with mapi id 14.03.0248.002;
Wed, 28 Dec 2016 17:00:04 +0800
From: "Xing, Beilei" <beilei.xing@intel.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
CC: "Wu, Jingjing" <jingjing.wu@intel.com>, "Zhang, Helin"
<helin.zhang@intel.com>, "dev@dpdk.org" <dev@dpdk.org>, "Lu, Wenzhuo"
<wenzhuo.lu@intel.com>
Thread-Topic: [dpdk-dev] [PATCH v2 07/17] net/i40e: add flow validate function
Thread-Index: AQHSYAqRciD6rBySQ0ecu+4N0N42baEbNoAAgAHN7WA=
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>
In-Reply-To: <20161227124004.GA3737@6wind.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [10.239.127.40]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH v2 07/17] net/i40e: add flow validate function
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Wed, 28 Dec 2016 09:00:15 -0000
> -----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
>=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 <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 >=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