From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 0E7692A62 for ; 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" To: "Horton, Remy" , "dev@dpdk.org" CC: Thomas Monjalon 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: References: <1484583573-30163-1-git-send-email-remy.horton@intel.com> <1484583573-30163-2-git-send-email-remy.horton@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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 ; dev@dpdk.org > Cc: Thomas Monjalon > 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.