From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <cristian.dumitrescu@intel.com>
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
by dpdk.org (Postfix) with ESMTP id D560F2A5E
for <dev@dpdk.org>; Mon, 13 Feb 2017 12:32:11 +0100 (CET)
Received: from fmsmga002.fm.intel.com ([10.253.24.26])
by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
13 Feb 2017 03:32:10 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.35,156,1484035200"; d="scan'208";a="1125880867"
Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99])
by fmsmga002.fm.intel.com with ESMTP; 13 Feb 2017 03:32:09 -0800
Received: from irsmsx108.ger.corp.intel.com ([169.254.11.173]) by
IRSMSX107.ger.corp.intel.com ([169.254.10.3]) with mapi id 14.03.0248.002;
Mon, 13 Feb 2017 11:32:09 +0000
From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: "Wiles, Keith" <keith.wiles@intel.com>, Jerin Jacob
<jerin.jacob@caviumnetworks.com>
CC: "dev@dpdk.org" <dev@dpdk.org>, "thomas.monjalon@6wind.com"
<thomas.monjalon@6wind.com>, "hemant.agrawal@nxp.com"
<hemant.agrawal@nxp.com>
Thread-Topic: [dpdk-dev] [PATCH 1/2] ethdev: add capability control API
Thread-Index: AQHShDGGoyuPRxRDB0WUJ/giNuJSG6FjxzQAgAMGu9A=
Date: Mon, 13 Feb 2017 11:32:08 +0000
Message-ID: <3EB4FA525960D640B5BDFFD6A3D891265274BC36@IRSMSX108.ger.corp.intel.com>
References: <1486735550-149878-1-git-send-email-cristian.dumitrescu@intel.com>
<1486735550-149878-2-git-send-email-cristian.dumitrescu@intel.com>
<20170211063833.GB9218@localhost.localdomain>
<8CF597D4-BCDA-4B65-93F6-9EA60334572E@intel.com>
In-Reply-To: <8CF597D4-BCDA-4B65-93F6-9EA60334572E@intel.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTMyYzFiMTgtMDE1Ny00MTBlLThmOGEtZjE1MGYxZjcyZjFlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjIuMTEuMCIsIlRydXN0ZWRMYWJlbEhhc2giOiIwZWFSXC9KNjA5dkE2WVdsNkE2NUp3cDBaNFNPcmhCZ3VLZ1hqQVNYQVg4Yz0ifQ==
x-ctpclassification: CTP_IC
x-originating-ip: [163.33.239.180]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: add capability control API
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: Mon, 13 Feb 2017 11:32:12 -0000
> -----Original Message-----
> From: Wiles, Keith
> Sent: Saturday, February 11, 2017 1:07 PM
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org;
> thomas.monjalon@6wind.com; hemant.agrawal@nxp.com
> Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: add capability control API
>=20
>=20
> > On Feb 11, 2017, at 12:38 AM, Jerin Jacob
> <jerin.jacob@caviumnetworks.com> wrote:
> >
> > On Fri, Feb 10, 2017 at 02:05:49PM +0000, Cristian Dumitrescu wrote:
> >> The rte_flow feature breaks the current monolithic approach for ethdev
> and
> >> introduces the new generic flow API to ethdev using a plugin-like
> approach.
> >>
> >> Basically, the rte_flow API is still logically part of ethdev:
> >> - It extends the ethdev functionality: rte_flow is a new feature/capab=
ility
> >> of ethdev;
> >> - all its functions work on an Ethernet device: the first parameter of=
the
> >> rte_flow functions is Ethernet device port ID.
> >>
> >> At the same time, the rte_flow API is a sort of capability plugin for =
ethdev:
> >> - the rte_flow API functions have their own name space: they are calle=
d
> >> rte_flow_operationXYZ() as opposed to
> rte_eth_dev_flow_operationXYZ());
> >> - the rte_flow API functions are placed in separate files in the same
> >> librte_ether folder as opposed to rte_ethdev.[hc].
> >>
> >> The way it works is by using the existing ethdev API function
> >> rte_eth_dev_filter_ctrl() to query the current Ethernet device port ID=
for
> the
> >> support of the rte_flow capability and return the pointer to the
> >> rte_flow operations when supported and NULL otherwise:
> >>
> >> struct rte_flow_ops *eth_flow_ops;
> >> int rte =3D rte_eth_dev_filter_ctrl(eth_port_id,
> >> RTE_ETH_FILTER_GENERIC, RTE_ETH_FILTER_GET, ð_flow_ops);
> >>
> >> Unfortunately, the rte_flow opportunistically uses the
> rte_eth_dev_filter_ctrl()
> >> API function, which is applicable just to RX-side filters as opposed t=
o
> >> introducing a mechanism that could be used by any capability in a gene=
ric
> way.
> >>
> >> This is the gap that addressed by the current patch. This mechanism is
> intended
> >> to be used to introduce new capabilities into ethdev in a modular plug=
in-
> like
> >> approach, such as hierarchical scheduler. Over time, if agreed, it can=
also
> be
> >> used for exposing the existing Ethernet device capabilities in a modul=
ar
> way,
> >> such as: xstats, filters, multicast, mirroring, tunnels, time stamping=
,
> eeprom,
> >> bypass, etc.
> >>
> >> Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> >> ---
> >> lib/librte_ether/rte_ethdev.c | 13 +++++++++++++
> >> lib/librte_ether/rte_ethdev.h | 29
> +++++++++++++++++++++++++++++
> >> lib/librte_ether/rte_ether_version.map | 7 +++++++
> >> 3 files changed, 49 insertions(+)
> >>
> >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethd=
ev.c
> >> index eb0a94a..ae187c4 100644
> >> --- a/lib/librte_ether/rte_ethdev.c
> >> +++ b/lib/librte_ether/rte_ethdev.c
> >> @@ -2802,6 +2802,19 @@ rte_eth_dev_filter_ctrl(uint8_t port_id, enum
> rte_filter_type filter_type,
> >> return (*dev->dev_ops->filter_ctrl)(dev, filter_type, filter_op, arg)=
;
> >> }
> >>
> >> +int
> >> +rte_eth_dev_capability_control(uint8_t port_id, enum
> rte_eth_capability cap,
> >> + void *arg)
> >> +{
> >> + struct rte_eth_dev *dev;
> >> +
> >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >> +
> >> + dev =3D &rte_eth_devices[port_id];
> >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->cap_ctrl, -
> ENOTSUP);
> >> + return (*dev->dev_ops->cap_ctrl)(dev, cap, arg);
> >> +}
> >> +
> >> void *
> >> rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id,
> >> rte_rx_callback_fn fn, void *user_param)
> >> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethd=
ev.h
> >> index c17bbda..43ffb9e 100644
> >> --- a/lib/librte_ether/rte_ethdev.h
> >> +++ b/lib/librte_ether/rte_ethdev.h
> >> @@ -1073,6 +1073,12 @@ TAILQ_HEAD(rte_eth_dev_cb_list,
> rte_eth_dev_callback);
> >> * structure associated with an Ethernet device.
> >> */
> >>
> >> +enum rte_eth_capability {
> >> + RTE_ETH_CAPABILITY_FLOW =3D 0, /**< Flow */
> >> + RTE_ETH_CAPABILITY_SCHED, /**< Hierarchical Scheduler */
> >> + RTE_ETH_CAPABILITY_MAX
> >> +};
> >
> > Shouldn't it be the FLAG?. Meaning, To represent ethdev port can have
> both.
>=20
> The current API is requesting if the PMD supports the given feature and t=
hen
> returns the void * to the structure of function pointers or NULL similar =
to the
> rte_flow design. The developer would need to ask multiple times to
> understand if all of the features are supported by the PMD. I guess one o=
f
> the options could be to return a list of features the PMD supports. The v=
oid *
> would point to the PMD capability list, which would need to be a const of
> some type to prevent someone from modifying the PMD capability list.
>=20
> enum rte_eth_capability {
> RTE_ETH_CAPABILITY_LIST =3D 0,
> RTE_ETH_CAPABILITY_FLOW =3D 1,
> RTE_ETH_CAPABILITY_SCHED =3D 2,
> RTE_ETH_CAPABILITY_SCHED =3D 4,
> RTE_ETH_CAPABILITY_MAX /* This one does not make
> sense in a bitmap set of enums */
> };
>=20
> The RTE_ETH_CAPABILITY_LIST could return the (void *) as a uint64_t listi=
ng
> the feature bits. The problem I think is the uint64_t is limiting us to 6=
3
> features (which maybe a big number, but maybe not) would be the only
> concern here. The PMD could return a pointer to a uint8_t array of featur=
e
> values, were 0 is used as a no-op then we can have any number of features
> with the enum just being a number between 1-255 or uint16_t 1-65535.
>=20
> Anyway just an option, we could have a different API for the feature list=
.
>=20
Yes, the purpose of the proposed rte_eth_dev_capability_control() function =
is to check if a specific capability/feature is supported and get its ops s=
tructure when supported.
If required, somebody can easily build up the list of capabilities by calli=
ng this function in for loop 0 .. (RTE_ETH_CAPABILITY_MAX - 1), any issues=
with this approach?
> >
> >> +
>=20
> Regards,
> Keith