From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v6 1/4] lib: add information metrics library Date: Thu, 12 Jan 2017 14:22:24 +0100 Message-ID: <1951093.Zqtj2Qm3TA@xps13> References: <1484150594-3758-1-git-send-email-remy.horton@intel.com> <1484150594-3758-2-git-send-email-remy.horton@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org To: Remy Horton Return-path: Received: from mail-wm0-f50.google.com (mail-wm0-f50.google.com [74.125.82.50]) by dpdk.org (Postfix) with ESMTP id EF08069FC for ; Thu, 12 Jan 2017 14:22:14 +0100 (CET) Received: by mail-wm0-f50.google.com with SMTP id c85so17916082wmi.1 for ; Thu, 12 Jan 2017 05:22:14 -0800 (PST) In-Reply-To: <1484150594-3758-2-git-send-email-remy.horton@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 2017-01-12 00:03, 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. [...] > --- a/doc/api/doxy-api.conf > +++ b/doc/api/doxy-api.conf > @@ -58,6 +58,7 @@ INPUT = doc/api/doxy-api-index.md \ > lib/librte_reorder \ > lib/librte_ring \ > lib/librte_sched \ > + lib/librte_metrics \ > lib/librte_table \ > lib/librte_timer \ > lib/librte_vhost It is not in the right order. Tip: when you add an item to a list, you should ask yourself what is the order. There are 3 types of order for the lists in DPDK: - chronological (add at the end) - alphabetical - logical/semantic The game is to find the right one :) [...] > @@ -171,6 +177,7 @@ The libraries prepended with a plus sign were incremented in this version. > librte_mbuf.so.2 > librte_mempool.so.2 > librte_meter.so.1 > + + librte_metrics.so.1 > librte_net.so.1 > librte_pdump.so.1 > librte_pipeline.so.3 Right order here ;) [...] > --- /dev/null > +++ b/lib/librte_metrics/rte_metrics.h > +/** Used to indicate port-independent information */ > +#define RTE_METRICS_NONPORT -1 I do not understand this constant. Why using the word "port" to name any device? What means independent? > +/** > + * Metric name > + */ > +struct rte_metric_name { > + /** String describing metric */ > + char name[RTE_METRICS_MAX_NAME_LEN]; > +}; Why a struct for a simple string? > +/** > + * Metric name. Copy/paste typo? > + */ > +struct rte_metric_value { > + /** Numeric identifier of metric */ > + uint16_t key; How the key is bound to the name? Remember how the xstats comments were improved: http://dpdk.org/commit/6d52d1d > + /** Value for metric */ > + uint64_t value; > +}; > + > + > +/** > + * Initializes metric module. This only has to be explicitly called if you > + * intend to use rte_metrics_reg_metric() or rte_metrics_reg_metrics() from a > + * secondary process. This function must be called from a primary process. > + */ > +void rte_metrics_init(void); > + > + > +/** > + * Register a metric You need to explain what is implied in registering. I have the same comment for registering a set of metrics. [...] > +int rte_metrics_reg_metric(const char *name); > + > +/** > + * Register a set of metrics [...] > +int rte_metrics_reg_metrics(const char **names, uint16_t cnt_names); > + > +/** > + * Get metric name-key lookup table. > + * > + * @param names > + * Array of names to receive key names > + * > + * @param capacity > + * Space available in names What happens if there is not enough space? > + * @return > + * - Non-negative: Success (number of names) > + * - Negative: Failure > + */ > +int rte_metrics_get_names( > + struct rte_metric_name *names, > + uint16_t capacity);