From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <harry.van.haaren@intel.com>
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
 by dpdk.org (Postfix) with ESMTP id 0E7692A62
 for <dev@dpdk.org>; Tue, 17 Jan 2017 15:23:59 +0100 (CET)
Received: from fmsmga006.fm.intel.com ([10.253.24.20])
 by orsmga103.jf.intel.com with ESMTP; 17 Jan 2017 06:23:58 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.33,245,1477983600"; d="scan'208";a="54907649"
Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155])
 by fmsmga006.fm.intel.com with ESMTP; 17 Jan 2017 06:23:58 -0800
Received: from irsmsx111.ger.corp.intel.com (10.108.20.4) by
 IRSMSX102.ger.corp.intel.com (163.33.3.155) with Microsoft SMTP Server (TLS)
 id 14.3.248.2; Tue, 17 Jan 2017 14:23:57 +0000
Received: from irsmsx102.ger.corp.intel.com ([169.254.2.230]) by
 irsmsx111.ger.corp.intel.com ([169.254.2.109]) with mapi id 14.03.0248.002;
 Tue, 17 Jan 2017 14:23:57 +0000
From: "Van Haaren, Harry" <harry.van.haaren@intel.com>
To: "Horton, Remy" <remy.horton@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
CC: Thomas Monjalon <thomas.monjalon@6wind.com>
Thread-Topic: [dpdk-dev] [PATCH v7 1/6] lib: add information metrics library
Thread-Index: AQHScBRy/r6haw2mNk2MQ53E3Uscn6E8eueggAAzZgCAAACg8A==
Date: Tue, 17 Jan 2017 14:23:56 +0000
Message-ID: <E923DB57A917B54B9182A2E928D00FA6129E3D80@IRSMSX102.ger.corp.intel.com>
References: <1484583573-30163-1-git-send-email-remy.horton@intel.com>
 <1484583573-30163-2-git-send-email-remy.horton@intel.com>
 <E923DB57A917B54B9182A2E928D00FA6129E3B53@IRSMSX102.ger.corp.intel.com>
 <0d2e03a5-3eb0-ec3f-9fe7-82e905d3adb3@intel.com>
In-Reply-To: <0d2e03a5-3eb0-ec3f-9fe7-82e905d3adb3@intel.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTcwYzMyNzYtMDY0MS00ZGU2LWI0ZGYtZjBhZmZkMjkxMTI0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX1BVQkxJQyJ9XX1dfSwiU3ViamVjdExhYmVscyI6W10sIlRNQ1ZlcnNpb24iOiIxNS45LjYuNiIsIlRydXN0ZWRMYWJlbEhhc2giOiJ2OUJqaElZdGxvQ2xlblNnMzJNS252S3pGeTl5NFR2aVdEQWsxdW5XMk5ZPSJ9
x-ctpclassification: CTP_PUBLIC
x-originating-ip: [163.33.239.182]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH v7 1/6] lib: add information metrics library
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, 17 Jan 2017 14:24:00 -0000

> -----Original Message-----
> From: Horton, Remy
> Sent: Tuesday, January 17, 2017 1:40 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> Cc: Thomas Monjalon <thomas.monjalon@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v7 1/6] lib: add information metrics libra=
ry
>=20
>=20
> On 17/01/2017 11:01, Van Haaren, Harry wrote:
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton
> [..]
> > Nit: why the _s in rte_metrics_meta_s? Is there a reason why struct
> > rte_metrics_meta {  is not suitable? Same question for
> > rte_metrics_data_s.
>=20
> Think it was originally to avoid a namespace collision, and I left the
> suffix in because this is an internal data-structure.

OK.

> >> +	if (memzone =3D=3D NULL)
> >> +		rte_exit(EXIT_FAILURE, "Unable to allocate stats memzone\n");
> >> +	stats =3D memzone->addr;
> >> +	memset(stats, 0, sizeof(struct rte_metrics_data_s));
> >> +	rte_spinlock_init(&stats->lock);
> >
> > Should this spinlock be initialized before the creation of the
> > memzone? Creating the memzone first, and later initializing the
> > locking-structure for it feels racey to me. The lock should probably
> > be taken and unlocked again after init and memset.
>=20
> Problem is the lock being part of the reserved memzone allocation. It is
> in there so that secondary processes can use the lock.

Ah ok - apologies I missed that the lock is in the memory itself.

>=20
> > Nit: would rte_metrics_reg_name() be a better function name?
> [..]
> > Nit: would rte_metrics_reg_names() be a better function name?
>=20
> Agree. Done.

Great

> > Would rte_metrics_update_single() be a better function name?
> [..]
> > Would rte_metrics_update() be a better function name?
>=20
> Think rte_metrics_update_value & rte_metrics_update_values would make a
> better pair. My preference at the moment is not to nominate a preferred o=
ne.
>=20
>=20
> >> +	if (port_id !=3D RTE_METRICS_GLOBAL &&
> >> +			(port_id < 0 || port_id > RTE_MAX_ETHPORTS))
> >> +		return -EINVAL;
> >> +
> >> +	rte_metrics_init();
> >
> > See above comments on rte_metrics_init() taking a socket_id parameter. =
Here any core
> could call update_metrics(), and if the library was not yet initialized, =
the memory for
> metrics would end up on the socket of this core. This should be avoided b=
y
> > A) adding socket_id parameter to the metrics_init() function
> > B) Demanding that metrics_init() is called by the application before an=
y core uses
> update_metrics()
>=20
> Done. Should also help alleviate the race.

Cool.


> > What is a "set border"? I think this ensures that one set of metrics
> > cannot overwrite the index's of another metrics set - correct?
>=20
> It is intended to stop things like:
>=20
>   idx1 =3D rte_metrics_reg_names(some_names, 2);
>   idx2 =3D rte_metrics_reg_names(more_names, 3);
>    ...
>   rte_metrics_update_values(port_id, idx1, &new_values, 5);
>=20
> as the above assumes idx2=3Didx1+2 which is not guaranteed in concurrent
> enviornments


That confirms my understanding - thanks.