From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id 2F17298 for ; Tue, 28 Aug 2018 18:40:20 +0200 (CEST) Received: by mail-wr1-f66.google.com with SMTP id a108-v6so2177260wrc.13 for ; Tue, 28 Aug 2018 09:40:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=ToKK+1GqTGP9Oftb6JdDKjnbuvJSzFLLulJO9fOZ5u0=; b=e2xsEiZU2VFYU1xVyhPm2WGgFug9evd3rhzA9H3t9T1aNHtEXKkAreLPlLti7dpOuu faiuZW1yhSd9Iij2544XZqSb6bxD/iBzosmAjl4aJkyYU1ZE/EcIXteNJBKi44v5O7d4 d6BA4DQLwEVJLgt0NiZdprGQXTu1iql0BSDOUZJc9IaDR8CIfcSxC9E8RGXnR4Xa/9qF ZjP+ZB2i3msZFwUYE7QgJTvl93WEAWaPUGEuxmi/D+WRSOYrKpmqZTUmQl7NqXcZS781 +Bwo9biGC0NPt1ycapol26QlPqcyXOqQhQNjGMJLkeob5FmVyMek9dlLWU8MxR8PRG0q 33Yw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=ToKK+1GqTGP9Oftb6JdDKjnbuvJSzFLLulJO9fOZ5u0=; b=U5+QYn19wX+azuo64jmWL/fn41+4ZewFW+xGHF9lSX4LzMlKwIK7D7W+jZQW2NRORF hv1Tml9yVqkIRX7lI1pd+xxp+2whc5fK61OG3ceCn6+jOV77SGkQMyaszcwjwnTJS3z1 fCYwC6lhg6KHxcOk783x2qJ1egZyzM2KZvYQ0eFWjs2TJ1puARZe4qxG9qFuKvMAjPdg uurpT5p7d7soUyzls2azYX+VF6m8Jc3bKzFmpz6G50cxLfiqaW+yv2zwxjkqL28UjIxJ /C+yukpTwa1nktX8HMXStL0JB0PzDRhv12AhfK8ctp9+JcuhJqhEeVfDqNEUNKMLgIPJ /pAQ== X-Gm-Message-State: APzg51AcyV3uOIzmxqmmVkIlLkgBHSp7zGqFz47GqKAi1lVco6ZM8kdH dyrnjAMyNp+YJCaVsh8JN25XlQ== X-Google-Smtp-Source: ANB0VdZY+Ni1czN7zDLou+R+3fSB+6sgy+enul2NOWCqIymVUEB2ktXTFjHRWL5GRMesDAbPlqZ1EQ== X-Received: by 2002:adf:9051:: with SMTP id h75-v6mr1823067wrh.65.1535474419528; Tue, 28 Aug 2018 09:40:19 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id d12-v6sm898004wru.36.2018.08.28.09.40.17 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 28 Aug 2018 09:40:18 -0700 (PDT) Date: Tue, 28 Aug 2018 18:40:02 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Ciara Power Cc: harry.van.haaren@intel.com, brian.archbold@intel.com, emma.kenny@intel.com, dev@dpdk.org Message-ID: <20180828164002.azcco4o3spvi2yc7@bidouze.vm.6wind.com> References: <1535026093-101872-1-git-send-email-ciara.power@intel.com> <1535026093-101872-3-git-send-email-ciara.power@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1535026093-101872-3-git-send-email-ciara.power@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH 02/11] telemetry: add initial connection socket 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, 28 Aug 2018 16:40:20 -0000 Hi Ciara, On Thu, Aug 23, 2018 at 01:08:04PM +0100, Ciara Power wrote: > This patch adds the telemetry UNIX socket. It is used to > allow connections from external clients. > > On the initial connection from a client, ethdev stats are > registered in the metrics library, to allow for their retrieval > at a later stage. > > Signed-off-by: Ciara Power > Signed-off-by: Brian Archbold > --- > lib/librte_telemetry/rte_telemetry.c | 205 ++++++++++++++++++++++++++ > lib/librte_telemetry/rte_telemetry_internal.h | 4 + > 2 files changed, 209 insertions(+) > > diff --git a/lib/librte_telemetry/rte_telemetry.c b/lib/librte_telemetry/rte_telemetry.c > index 8d7b0e3..f984929 100644 > --- a/lib/librte_telemetry/rte_telemetry.c > +++ b/lib/librte_telemetry/rte_telemetry.c > @@ -3,21 +3,159 @@ > */ > > #include > +#include > +#include > +#include > > #include > #include > #include > +#include > > #include "rte_telemetry.h" > #include "rte_telemetry_internal.h" > > #define SLEEP_TIME 10 > > +#define DEFAULT_DPDK_PATH "/var/run/.rte_telemetry" > + > +const char *socket_path = DEFAULT_DPDK_PATH; > static telemetry_impl *static_telemetry; > > +int32_t > +rte_telemetry_check_port_activity(int port_id) > +{ > + int pid; > + > + RTE_ETH_FOREACH_DEV(pid) { > + if (pid == port_id) > + return 1; > + } > + TELEMETRY_LOG_ERR("Error - port_id: %d is invalid, not active\n", > + port_id); > + return 0; > +} > + This function seems more about ethdev than telemetry. Maybe add it as part of librte_ethdev. The "active" semantic is blurry however. With this implementation, this is checking that the device is currently attached and owned by the default user. Should telemetry be limited to devices owned by default user? Also, this function does not seem used in this patch, can it be added when used? > +static int32_t > +rte_telemetry_reg_ethdev_to_metrics(uint16_t port_id) "_to" might not be necessary. > +{ > + int ret, num_xstats, start_index, i; > + struct rte_eth_xstat *eth_xstats; > + > + if (!rte_eth_dev_is_valid_port(port_id)) { > + TELEMETRY_LOG_ERR("Error - port_id: %d is invalid\n", port_id); > + return -EINVAL; > + } > + > + num_xstats = rte_eth_xstats_get(port_id, NULL, 0); > + if (num_xstats < 0) { > + TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get(%u) failed:" > + " %d\n", port_id, num_xstats); I guess there isn't really a consensus yet, as the checkpatch.sh tool might be misconfigured, but the cesura is very awkward here. Same for other logs. > + return -EPERM; > + } > + > + eth_xstats = malloc(sizeof(struct rte_eth_xstat) * num_xstats); > + if (eth_xstats == NULL) { > + TELEMETRY_LOG_ERR("Error - Failed to malloc memory for" > + " xstats\n"); > + return -ENOMEM; > + } > + ret = rte_eth_xstats_get(port_id, eth_xstats, num_xstats); > + if (ret < 0 || ret > num_xstats) { > + free(eth_xstats); > + TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get(%u) len%i" > + " failed: %d\n", port_id, num_xstats, ret); > + return -EPERM; > + } > + struct rte_eth_xstat_name *eth_xstats_names; > + eth_xstats_names = malloc(sizeof(struct rte_eth_xstat_name) * > + num_xstats); > + if (eth_xstats_names == NULL) { You are sometimes checking pointers against NULL, sometimes using "!". You can choose either in your library, but it would be better to be consistent and use a unified coding style. > + free(eth_xstats); > + TELEMETRY_LOG_ERR("Error - Failed to malloc memory for" > + " xstats_names\n"); > + return -ENOMEM; > + } > + ret = rte_eth_xstats_get_names(port_id, eth_xstats_names, > + num_xstats); > + if (ret < 0 || ret > num_xstats) { > + free(eth_xstats); > + free(eth_xstats_names); > + TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get_names(%u)" > + " len%i failed: %d\n", port_id, num_xstats, > + ret); > + return -EPERM; > + } > + const char *xstats_names[num_xstats]; > + > + for (i = 0; i < num_xstats; i++) > + xstats_names[i] = eth_xstats_names[eth_xstats[i].id].name; > + > + start_index = rte_metrics_reg_names(xstats_names, num_xstats); > + > + if (start_index < 0) { > + TELEMETRY_LOG_ERR("Error - rte_metrics_reg_names failed -" > + " metrics may already be registered\n"); > + free(eth_xstats); > + free(eth_xstats_names); > + return -1; > + } > + free(eth_xstats_names); > + free(eth_xstats); At this point, you have repeated 3 times those frees(). It would be cleaner to define proper labels and use goto instead. [snip] -- Gaëtan Rivet 6WIND