From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id C87B0DE3 for ; Tue, 17 Jan 2017 12:01:24 +0100 (CET) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga104.fm.intel.com with ESMTP; 17 Jan 2017 03:01:23 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,244,1477983600"; d="scan'208";a="53929837" Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28]) by orsmga005.jf.intel.com with ESMTP; 17 Jan 2017 03:01:22 -0800 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by irsmsx105.ger.corp.intel.com (163.33.3.28) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 17 Jan 2017 11:01:21 +0000 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.230]) by irsmsx112.ger.corp.intel.com ([169.254.1.175]) with mapi id 14.03.0248.002; Tue, 17 Jan 2017 11:01:21 +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/r6haw2mNk2MQ53E3Uscn6E8eueg Date: Tue, 17 Jan 2017 11:01:20 +0000 Message-ID: References: <1484583573-30163-1-git-send-email-remy.horton@intel.com> <1484583573-30163-2-git-send-email-remy.horton@intel.com> In-Reply-To: <1484583573-30163-2-git-send-email-remy.horton@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMGU5Njk1NWItMzg4Yy00ZWZkLThjYjctMWI0OTYzZTE2MGJiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Ilc0dXNpamkydlJ4WGUrRGJtVVVwWkxzTFlEVzJYdHNTOUZ1QXhmWnkwUXM9In0= x-ctpclassification: CTP_IC 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 11:01:25 -0000 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton > This patch adds a new information metric library that allows other > modules to register named metrics and update their values. It is > intended to be independent of ethdev, rather than mixing ethdev > and non-ethdev information in xstats. >=20 > Signed-off-by: Remy Horton Comments inline below. > diff --git a/MAINTAINERS b/MAINTAINERS > index 9645c9b..4a19497 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -596,6 +596,11 @@ F: lib/librte_jobstats/ > F: examples/l2fwd-jobstats/ > F: doc/guides/sample_app_ug/l2_forward_job_stats.rst >=20 > +Metrics > +M: Remy Horton > +F: lib/librte_metrics/ > +F: doc/guides/sample_app_ug/keep_alive.rst Copy paste error, Keep alive sample app guide should not be here. There is = no sample app / guide for usage of the metrics library in this patchset as = it is. > +++ b/lib/librte_metrics/rte_metrics.c > @@ -0,0 +1,310 @@ > +/*- > + * BSD LICENSE > + * > + * Copyright(c) 2016 Intel Corporation. All rights reserved. > + * All rights reserved. Update to 2016-2017 > +/** > + * Internal stats metadata and value entry. > + * > + * @internal > + * @param name > + * Name of metric > + * @param value > + * Current value for metric > + * @param idx_next_set > + * Index of next root element (zero for none) > + * @param idx_next_metric > + * Index of next metric in set (zero for none) > + * > + * Only the root of each set needs idx_next_set but since it has to be > + * assumed that number of sets could equal total number of metrics, > + * having a separate set metadata table doesn't save any memory. > + */ > +struct rte_metrics_meta_s { > + char name[RTE_METRICS_MAX_NAME_LEN]; > + uint64_t value[RTE_MAX_ETHPORTS]; > + uint64_t nonport_value; > + uint16_t idx_next_set; > + uint16_t idx_next_stat; > +}; Nit: why the _s in rte_metrics_meta_s? Is there a reason why struct rte_met= rics_meta { is not suitable? Same question for rte_metrics_data_s. > +void > +rte_metrics_init(void) > +{ > + struct rte_metrics_data_s *stats; > + const struct rte_memzone *memzone; > + > + if (rte_eal_process_type() !=3D RTE_PROC_PRIMARY) > + return; > + > + memzone =3D rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME); > + if (memzone !=3D NULL) > + return; > + memzone =3D rte_memzone_reserve(RTE_METRICS_MEMZONE_NAME, > + sizeof(struct rte_metrics_data_s), rte_socket_id(), 0); In my opinion, passing socket_id to rte_metrics_init() would be a better AP= I, than relying on the metrics initialization core to be on the correct soc= ket. > + 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? Cre= ating the memzone first, and later initializing the locking-structure for i= t feels racey to me. The lock should probably be taken and unlocked again a= fter init and memset. > +} > + > +int > +rte_metrics_reg_metric(const char *name) > +{ Nit: would rte_metrics_reg_name() be a better function name? > + const char * const list_names[] =3D {name}; > + > + return rte_metrics_reg_metrics(list_names, 1); > +} > + > +int > +rte_metrics_reg_metrics(const char * const *names, uint16_t cnt_names) > +{ Nit: would rte_metrics_reg_names() be a better function name? > + > +int > +rte_metrics_update_metric(int port_id, uint16_t key, const uint64_t valu= e) > +{ Would rte_metrics_update_single() be a better function name? I presume upda= ting a single metric at a time is exception case, and the (see below) more = generic rte_metrics_update() would be more commonly used. > + return rte_metrics_update_metrics(port_id, key, &value, 1); > +} > + > +int > +rte_metrics_update_metrics(int port_id, > + uint16_t key, > + const uint64_t *values, > + uint32_t count) Would rte_metrics_update() be a better function name? > +{ > + struct rte_metrics_meta_s *entry; > + struct rte_metrics_data_s *stats; > + const struct rte_memzone *memzone; > + uint16_t idx_metric; > + uint16_t idx_value; > + uint16_t cnt_setsize; > + > + 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 initi= alized, the memory for metrics would end up on the socket of this core. Thi= s should be avoided by A) adding socket_id parameter to the metrics_init() function B) Demanding that metrics_init() is called by the application before any co= re uses update_metrics()=20 > + memzone =3D rte_memzone_lookup(RTE_METRICS_MEMZONE_NAME); > + if (memzone =3D=3D NULL) > + return -EIO; > + stats =3D memzone->addr; > + > + rte_spinlock_lock(&stats->lock); > + idx_metric =3D key; > + cnt_setsize =3D 1; > + while (idx_metric < stats->cnt_stats) { > + entry =3D &stats->metadata[idx_metric]; > + if (entry->idx_next_stat =3D=3D 0) > + break; > + cnt_setsize++; > + idx_metric++; > + } > + /* Check update does not cross set border */ > + if (count > cnt_setsize) { > + rte_spinlock_unlock(&stats->lock); > + return -ERANGE; > + } What is a "set border"? I think this ensures that one set of metrics cannot= overwrite the index's of another metrics set - correct? > + > + if (port_id =3D=3D RTE_METRICS_GLOBAL) > + for (idx_value =3D 0; idx_value < count; idx_value++) { > + idx_metric =3D key + idx_value; > + stats->metadata[idx_metric].nonport_value =3D > + values[idx_value]; > + } > + else > + for (idx_value =3D 0; idx_value < count; idx_value++) { > + idx_metric =3D key + idx_value; > + stats->metadata[idx_metric].value[port_id] =3D > + values[idx_value]; > + } > + rte_spinlock_unlock(&stats->lock); > + return 0; > +} > diff --git a/lib/librte_metrics/rte_metrics.h b/lib/librte_metrics/rte_me= trics.h > new file mode 100644 > index 0000000..fd82af9 > --- /dev/null > +++ b/lib/librte_metrics/rte_metrics.h > @@ -0,0 +1,223 @@ > +/*- > + * BSD LICENSE > + * > + * Copyright(c) 2016 Intel Corporation. All rights reserved. > + * All rights reserved. Add -2017 > + > + > +/** > + * Initializes metric module. This only has to be explicitly called if y= ou > + * intend to use rte_metrics_reg_metric() or rte_metrics_reg_metrics() f= rom a > + * secondary process. This function must be called from a primary proces= s. > + */ > +void rte_metrics_init(void); As noted in the C function above: - accept a socket-id - demand an application calls it if it will use the metrics library.=20