From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id D2B199E3 for ; Mon, 30 Jan 2017 22:44:21 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP; 30 Jan 2017 13:44:20 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,312,1477983600"; d="scan'208";a="1100892543" Received: from rhorton-mobl.ger.corp.intel.com (HELO [10.252.8.52]) ([10.252.8.52]) by fmsmga001.fm.intel.com with ESMTP; 30 Jan 2017 13:44:18 -0800 From: Remy Horton To: Thomas Monjalon References: <1484751943-22877-1-git-send-email-remy.horton@intel.com> <1484751943-22877-2-git-send-email-remy.horton@intel.com> <1659974.13GHXkxjEr@xps13> Cc: dev@dpdk.org Organization: Intel Shannon Limited Message-ID: Date: Mon, 30 Jan 2017 21:44:17 +0000 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <1659974.13GHXkxjEr@xps13> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v9 1/7] 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: Mon, 30 Jan 2017 21:44:22 -0000 Some points addressed. Will cover other ones later. On 30/01/2017 15:50, Thomas Monjalon wrote: [..] >> +CONFIG_RTE_LIBRTE_METRICS=y > I know the config file is not so well sorted. > However it would be a bit more logical below CONFIG_RTE_LIBRTE_JOBSTATS. Done. Rebase merging doesn't help with sorting here. >> + [Device Metrics] (@ref rte_metrics.h), > No first letter uppercase in this list. Fixed. >> + A library that allows information metrics to be added and update. It is > update -> updated Fixed. > added and updated by who? Elaborated. >> + intended to provide a reporting mechanism that is independent of the >> + ethdev library. > and independent of the cryptodev library? Yes. The aim is to have no sub-dependencies. My original plan was to introduce some form of parameter registration scheme into xstats to replace the current hard-coded tables, since I suspected libbitrate/liblatency would not be the last additions. I decided to spin it out into a library in its own right, as it seemed cleaner than shoving a load of non-driver stuff into xstats. > Does it apply to other types of devices (cryptodev/eventdev)? I've not been following cryptodev/eventdev, but short answer yes. >> This section is a comment. do not overwrite or remove it. >> Also, make sure to start the actual text at the margin. >> ========================================================= > Your text should start below this line, and indented at the margin. Fixed. >> + + librte_bitratestats.so.1 > not part of this patch Fixed. Artefact of sorting out a merge mess-up. >> +DIRS-$(CONFIG_RTE_LIBRTE_METRICS) += librte_metrics > insert it below librte_jobstats is a better choice Done. >> +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; >> +}; > It would be a lot easier to read with comments near each field. > It would avoid to forget some fields like nonport_value in this struct. > You do not need to use a doxygen syntax in a .c file. Noted. Even though Doxygen isn't required, I think it preferable to use a consistent style in both .c and .h files. >> + * RTE Metrics module > RTE is not meaningful here. > Please prefer DPDK. Fixed. >> + * Metric information is populated using a push model, where the >> + * information provider calls an update function on the relevant >> + * metrics. Currently only bulk querying of metrics is supported. >> + */ > This description should explain who is a provider (drivers?) and who > is the reader (registered thread?). Noted. Will elaborate. > What do you mean by "push model"? A callback is used? Updating is done in response to producers having new information. In contrast in a pull model an update would happen in response to a polling by a consumer. Originally (back in August I think) I used a pull model where producers would register callbacks that were called in response to rte_metrics_get() by a consumer, but that assumed that producers and consumers were within the same process. Using shared memory and making it ASLR-safe means not using pointers. Aside: In this former pull model, port_id was passed verbatim to the producers' callbacks to interpret in whatever way they saw fit, so there was no inherant need to stop "magic" values outside the usual 0-255 used for port ids. Hence where the next thing originally came from... >> +/** >> + * Global (rather than port-specific) metric. > It does not say what kind of constant it is. A special metric id? Yes. Elaborated. >> + * >> + * When used instead of port number by rte_metrics_update_metric() >> + * or rte_metrics_update_metric(), the global metrics, which are >> + * not associated with any specific port, are updated. >> + */ >> +#define RTE_METRICS_GLOBAL -1 > > I thought you agreed that "port" is not really a good wording. Certainly within the constant name. Don't see what's wrong with referring to it in description though. >> + * An array of this structure is returned by rte_metrics_get_names(). >> + * The struct rte_eth_stats references these names via their array index. > rte_eth_stats? Good question - was going to put it down to cut'n'paste while baseing the descriptions on Olivier Matz's rewording, but that was for xstats.. >> + * Initializes metric module. This function must be called from >> + * a primary process before metrics are used. > > Why not integrating it in the global init? > Is there some performance drawbacks? There shouldn't be any significant performance penalities, but I am not particularly fond in principle of initalising component libraries regardless of whether they are used. (Actually it was previously initalised on first use, but that had a race condition.) >> + * Registering a metric is the way third-parties declare a parameter > third-party? You mean the provider? Yes. >> + * - \b -EINVAL: Error, invalid parameters >> + * - \b -ENOMEM: Error, maximum metrics reached > Please, no extra formatting in doxygen. Fixed.