From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <beilei.xing@intel.com>
Received: from mga09.intel.com (mga09.intel.com [134.134.136.24])
by dpdk.org (Postfix) with ESMTP id BC0585588
for <dev@dpdk.org>; Tue, 27 Dec 2016 07:37:18 +0100 (CET)
Received: from fmsmga003.fm.intel.com ([10.253.24.29])
by orsmga102.jf.intel.com with ESMTP; 26 Dec 2016 22:37:16 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.33,415,1477983600"; d="scan'208";a="802574189"
Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205])
by FMSMGA003.fm.intel.com with ESMTP; 26 Dec 2016 22:37:03 -0800
Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by
fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS)
id 14.3.248.2; Mon, 26 Dec 2016 22:37:02 -0800
Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by
fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS)
id 14.3.248.2; Mon, 26 Dec 2016 22:37:02 -0800
Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.177]) by
shsmsx102.ccr.corp.intel.com ([169.254.2.88]) with mapi id 14.03.0248.002;
Tue, 27 Dec 2016 14:36:58 +0800
From: "Xing, Beilei" <beilei.xing@intel.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
CC: "Yigit, Ferruh" <ferruh.yigit@intel.com>, "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 10/24] ethdev: parse ethertype filter
Thread-Index: AQHSTFLM3otZCxuiykqEkHYB8jn2SKEQum2AgAEgUiCAAvfCAIAGoHDQ
Date: Tue, 27 Dec 2016 06:36:58 +0000
Message-ID: <94479800C636CB44BD422CB454846E013158B8D8@SHSMSX101.ccr.corp.intel.com>
References: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>
<1480679625-4157-11-git-send-email-beilei.xing@intel.com>
<ff1ef72f-7397-a7d0-071f-ff18be979bf6@intel.com>
<94479800C636CB44BD422CB454846E013157F36E@SHSMSX101.ccr.corp.intel.com>
<20161223084328.GI10340@6wind.com>
In-Reply-To: <20161223084328.GI10340@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 10/24] ethdev: parse ethertype filter
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: Tue, 27 Dec 2016 06:37:19 -0000
> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Friday, December 23, 2016 4:43 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; 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 10/24] ethdev: parse ethertype filter
>=20
> Hi all,
>=20
> On Wed, Dec 21, 2016 at 03:54:50AM +0000, Xing, Beilei wrote:
> > Hi Ferruh,
> >
> > > -----Original Message-----
> > > From: Yigit, Ferruh
> > > Sent: Wednesday, December 21, 2016 2:12 AM
> > > To: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing
> > > <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> > > Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Adrien
> > > Mazarguil <adrien.mazarguil@6wind.com>
> > > Subject: Re: [dpdk-dev] [PATCH 10/24] ethdev: parse ethertype filter
> > >
> > > On 12/2/2016 11:53 AM, Beilei Xing wrote:
> > > > Check if the rule is a ethertype rule, and get the ethertype info B=
TW.
> > > >
> > > > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > > > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > > > ---
> > >
> > > CC: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>=20
> Thanks again for CC'ing me.
>=20
> > > > lib/librte_ether/rte_flow.c | 136
> > > +++++++++++++++++++++++++++++++++++++
> > > > lib/librte_ether/rte_flow_driver.h | 34 ++++++++++
> > >
> > > <...>
> > >
> > > > diff --git a/lib/librte_ether/rte_flow_driver.h
> > > > b/lib/librte_ether/rte_flow_driver.h
> > > > index a88c621..2760c74 100644
> > > > --- a/lib/librte_ether/rte_flow_driver.h
> > > > +++ b/lib/librte_ether/rte_flow_driver.h
> > > > @@ -170,6 +170,40 @@ rte_flow_error_set(struct rte_flow_error
> > > > *error, const struct rte_flow_ops * rte_flow_ops_get(uint8_t
> > > > port_id, struct rte_flow_error *error);
> > > >
> > > > +int cons_parse_ethertype_filter(const struct rte_flow_attr *attr,
> > > > + const struct rte_flow_item *pattern,
> > > > + const struct rte_flow_action *actions,
> > > > + struct rte_eth_ethertype_filter *filter,
> > > > + struct rte_flow_error *error);
> > >
> > > Although this is helper function, it may be good if it follows the
> > > rte_follow namespace.
> >
> > OK, I will rename it in the next version, thanks very much.
>=20
> Agreed, all public symbols exposed by headers must be prefixed with
> rte_flow.
>=20
> Now I'm not so sure about the need to convert a rte_flow rule to a
> rte_eth_ethertype_filter. This definition basically makes rte_flow depend=
on
> rte_eth_ctrl.h (related #include is missing by the way).
>=20
Since the whole implementation of parse function is modified, there'll be n=
o common rte_eth_ethertype_filter here temporarily.
> I understand that both ixgbe and i40e would benefit from it, and consider=
ing
> rte_flow_driver.h is free from ABI versioning I guess it's acceptable, bu=
t
> remember we'll gradually remove existing filter types so we should avoid
> new dependencies on them. Just keep in mind this will be temporary.
>=20
i40e and ixgbe all use existing filter types in rte_flow_driver.h. if all =
existing filter types will be removed, we need to change the fiter info aft=
er applied.
> Please add full documentation as well in Doxygen style like for existing
> symbols. We have to maintain this API properly documented.
>=20
> > > > +
> > > > +#define PATTERN_SKIP_VOID(filter, filter_struct, error_type)
> > > \
> > > > + do { \
> > > > + if (!pattern) { \
> > > > + memset(filter, 0, sizeof(filter_struct)); \
> > > > + error->type =3D error_type; \
> > > > + return -EINVAL;
> > > \
> > > > + } \
> > > > + item =3D pattern + i; \
> > >
> > > I believe macros that relies on variables that not passed as
> > > argument is not good idea.
> >
> > Yes, I'm reworking the macros, and it will be changed in v2.
> >
> > >
> > > > + while (item->type =3D=3D RTE_FLOW_ITEM_TYPE_VOID) {
> > > \
> > > > + i++; \
> > > > + item =3D pattern + i; \
> > > > + } \
> > > > + } while (0)
> > > > +
> > > > +#define ACTION_SKIP_VOID(filter, filter_struct, error_type)
> > > \
> > > > + do { \
> > > > + if (!actions) { \
> > > > + memset(filter, 0, sizeof(filter_struct)); \
> > > > + error->type =3D error_type; \
> > > > + return -EINVAL;
> > > \
> > > > + } \
> > > > + act =3D actions + i; \
> > > > + while (act->type =3D=3D RTE_FLOW_ACTION_TYPE_VOID) { \
> > > > + i++; \
> > > > + act =3D actions + i; \
> > > > + } \
> > > > + } while (0)
> > >
> > > Are these macros generic enough for all rte_flow consumers?
> > >
> > > What do you think separate this patch, and use these after applied,
> > > meanwhile keeping function and MACROS PMD internal?
> >
> > The main purpose of the macros is to reduce the code in PMD, otherwise
> > there'll be many such codes to get the next non-void item in all parse
> > functions, including the parse_ethertype_filter function in
> > rte_flow.c. But actually I'm not very sure if it's generic enough for
> > all consumers, although I think it's general at present:)
>=20
> I'll concede skipping VOIDs can be tedious depending on the parser
> implementation, but I do not think these macros need to be exposed either=
.
> PMDs can duplicate some code such as this.
>=20
> I think ixgbe and i40e share a fair amount of code already, and factoring=
it
> should be part of larger task to create a common Intel-specific library i=
nstead.
Good point. Thanks. We'll consider related implementation for the common co=
de.
In V2 patch set, there'll be no common code temporarily since the implement=
ation of parsing functions is different between ixgbe and i40e.
>=20
> > Thanks for your advice, I'll move the macros to PMD currently, then the=
re'll
> be no macros used in parse_ethertype_filter function, and optimize it aft=
er
> applied.
> >
> > BTW, I plan to send out V2 patch set in this week.
> >
> > Best Regards,
> > Beilei
> >
> > >
> > > > +
> > > > #ifdef __cplusplus
> > > > }
> > > > #endif
> > > >
> >
>=20
> --
> Adrien Mazarguil
> 6WIND