From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by dpdk.org (Postfix) with ESMTP id 420825F72 for ; Wed, 3 Oct 2018 20:40:12 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id B36D14007B for ; Wed, 3 Oct 2018 20:40:11 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id A09C64007A; Wed, 3 Oct 2018 20:40:11 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on bernadotte.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-0.9 required=5.0 tests=ALL_TRUSTED,AWL autolearn=disabled version=3.4.1 X-Spam-Score: -0.9 Received: from [192.168.1.59] (host-90-232-60-155.mobileonline.telia.com [90.232.60.155]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 2807040072; Wed, 3 Oct 2018 20:40:07 +0200 (CEST) To: Kevin Laatz , dev@dpdk.org Cc: harry.van.haaren@intel.com, stephen@networkplumber.org, gaetan.rivet@6wind.com, shreyansh.jain@nxp.com, thomas@monjalon.net, Ciara Power , Brian Archbold References: <1535026093-101872-1-git-send-email-ciara.power@intel.com> <20181003173612.67101-1-kevin.laatz@intel.com> <20181003173612.67101-3-kevin.laatz@intel.com> From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= Message-ID: <72e3e8d1-fa63-2531-a3c3-2d0c9ee5d151@ericsson.com> Date: Wed, 3 Oct 2018 20:40:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181003173612.67101-3-kevin.laatz@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Virus-Scanned: ClamAV using ClamSMTP Subject: Re: [dpdk-dev] [PATCH v2 02/10] 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: Wed, 03 Oct 2018 18:40:12 -0000 On 2018-10-03 19:36, Kevin Laatz wrote: > From: Ciara Power > > 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 > Signed-off-by: Kevin Laatz > --- > lib/librte_telemetry/rte_telemetry.c | 210 ++++++++++++++++++++++++++ > lib/librte_telemetry/rte_telemetry_internal.h | 4 + > 2 files changed, 214 insertions(+) > > diff --git a/lib/librte_telemetry/rte_telemetry.c b/lib/librte_telemetry/rte_telemetry.c > index d9ffec2..0c99d66 100644 > --- a/lib/librte_telemetry/rte_telemetry.c > +++ b/lib/librte_telemetry/rte_telemetry.c > @@ -3,22 +3,158 @@ > */ > > #include > +#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" > + FHS 3.0 recommends "/run" over "/var/run". Maybe a more descriptive macro name would be in order, too. > +const char *socket_path = DEFAULT_DPDK_PATH; Is there really a need for this variable? Why not use the macro directly? > static telemetry_impl *static_telemetry; > > +int32_t > +rte_telemetry_is_port_active(int port_id) > +{ > + int ret; > + > + ret = rte_eth_find_next(port_id); > + if (ret == port_id) > + return 1; > + > + TELEMETRY_LOG_ERR("port_id: %d is invalid, not active", > + port_id); > + return 0; > +} > + > +static int32_t > +rte_telemetry_reg_ethdev_to_metrics(uint16_t port_id) > +{ > + int ret, num_xstats, ret_val, i; > + struct rte_eth_xstat *eth_xstats = NULL; > + struct rte_eth_xstat_name *eth_xstats_names = NULL; > + > + if (!rte_eth_dev_is_valid_port(port_id)) { > + TELEMETRY_LOG_ERR("port_id: %d is invalid", port_id); > + return -EINVAL; > + } > + > + num_xstats = rte_eth_xstats_get(port_id, NULL, 0); > + if (num_xstats < 0) { > + TELEMETRY_LOG_ERR("rte_eth_xstats_get(%u) failed: %d", > + port_id, num_xstats); > + return -EPERM; > + } > + > + eth_xstats = malloc(sizeof(struct rte_eth_xstat) * num_xstats); > + if (!eth_xstats) { eth_stats == NULL per 1.8.1 in the DPDK coding style guide. > + TELEMETRY_LOG_ERR("Failed to malloc memory for xstats"); > + return -ENOMEM; > + } > + > + ret = rte_eth_xstats_get(port_id, eth_xstats, num_xstats); > + const char *xstats_names[num_xstats]; > + eth_xstats_names = malloc(sizeof(struct rte_eth_xstat_name) * num_xstats); > + if (ret < 0 || ret > num_xstats) { > + TELEMETRY_LOG_ERR("rte_eth_xstats_get(%u) len%i failed: %d", > + port_id, num_xstats, ret); > + ret_val = -EPERM; > + goto free_xstats; > + } > + > + if (!eth_xstats_names) { > + TELEMETRY_LOG_ERR("Failed to malloc memory for xstats_names"); > + ret_val = -ENOMEM; > + goto free_xstats; > + } > + > + ret = rte_eth_xstats_get_names(port_id, eth_xstats_names, num_xstats); > + if (ret < 0 || ret > num_xstats) { > + TELEMETRY_LOG_ERR("rte_eth_xstats_get_names(%u) len%i failed: %d", > + port_id, num_xstats, ret); > + ret_val = -EPERM; > + goto free_xstats; > + } > + > + for (i = 0; i < num_xstats; i++) > + xstats_names[i] = eth_xstats_names[eth_xstats[i].id].name; > + > + ret_val = rte_metrics_reg_names(xstats_names, num_xstats); > + if (ret_val < 0) { > + TELEMETRY_LOG_ERR("rte_metrics_reg_names failed - metrics may already be registered"); > + ret_val = -1; > + goto free_xstats; > + } > + > + goto free_xstats; This goto seems a little redundant to me. > + > +free_xstats: > + free(eth_xstats); > + free(eth_xstats_names); > + return ret_val; > +} > + > +static int32_t > +rte_telemetry_initial_accept(struct telemetry_impl *telemetry) > +{ > + int pid; Ethernet port ids are uint16_t. > + > + RTE_ETH_FOREACH_DEV(pid) { > + telemetry->reg_index = > + rte_telemetry_reg_ethdev_to_metrics(pid); > + break; > + } > + > + if (telemetry->reg_index < 0) { > + TELEMETRY_LOG_ERR("Failed to register ethdev metrics"); > + return -1; > + } > + > + telemetry->metrics_register_done = 1; > + > + return 0; > +} > + > +static int32_t > +rte_telemetry_accept_new_client(struct telemetry_impl *telemetry) > +{ > + int ret; > + > + if (telemetry->accept_fd == 0 || telemetry->accept_fd == -1) { > + ret = listen(telemetry->server_fd, 1); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Listening error with server fd"); > + return -1; > + } > + telemetry->accept_fd = accept(telemetry->server_fd, NULL, NULL); > + > + if (telemetry->accept_fd > 0 && accept() returns -1 on error. 0 is a valid fd (although in this case it can't be returned, since at least one fd - the server socket - is open). > + telemetry->metrics_register_done == 0) { > + ret = rte_telemetry_initial_accept(telemetry); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Failed to run initial configurations/tests"); > + return -1; > + } > + } > + } > + > + return 0; > +} > + > static int32_t > rte_telemetry_run(void *userdata) > { > + int ret; > struct telemetry_impl *telemetry = userdata; > > if (!telemetry) { 1.8.1 again. > @@ -26,6 +162,12 @@ rte_telemetry_run(void *userdata) > return -1; > } > > + ret = rte_telemetry_accept_new_client(telemetry); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Accept and read new client failed"); > + return -1; > + } > + > return 0; > } > > @@ -49,6 +191,56 @@ static void > pthread_exit(0); > } > > +static int32_t > +rte_telemetry_set_socket_nonblock(int fd) > +{ > + int flags = fcntl(fd, F_GETFL, 0); > + > + if (fd < 0) { > + TELEMETRY_LOG_ERR("Invalid fd provided"); > + return -1; > + } Shouldn't you do this check before you do the first fcntl()? If at all. > + > + if (flags < 0) > + flags = 0; > + > + return fcntl(fd, F_SETFL, flags | O_NONBLOCK); > +} > + > +static int32_t > +rte_telemetry_create_socket(struct telemetry_impl *telemetry) > +{ > + int ret; > + struct sockaddr_un addr = {0}; Aren't you planning to set all the relevant fields? No need to zero. > + > + if (!telemetry) 1.8.1 > + return -1; > + > + telemetry->server_fd = socket(AF_UNIX, SOCK_STREAM, 0); > + if (telemetry->server_fd == -1) { > + TELEMETRY_LOG_ERR("Failed to open socket"); > + return -1; > + } > + > + ret = rte_telemetry_set_socket_nonblock(telemetry->server_fd); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Could not set socket to NONBLOCK"); Close the socket? > + return -1; > + } > + > + addr.sun_family = AF_UNIX; > + strlcpy(addr.sun_path, socket_path, sizeof(addr.sun_path)); > + unlink(socket_path); > + > + if (bind(telemetry->server_fd, (struct sockaddr *)&addr, > + sizeof(addr)) < 0) { > + TELEMETRY_LOG_ERR("Socket binding error"); Close it here as well. > + return -1; > + } > + > + return 0; > +} > + > int32_t > rte_telemetry_init() > { > @@ -69,6 +261,14 @@ rte_telemetry_init() > > static_telemetry->socket_id = rte_socket_id(); > rte_metrics_init(static_telemetry->socket_id); > + ret = rte_telemetry_create_socket(static_telemetry); > + if (ret < 0) { > + ret = rte_telemetry_cleanup(); > + if (ret < 0) > + TELEMETRY_LOG_ERR("TELEMETRY cleanup failed"); > + return -EPERM; > + } > + > pthread_attr_init(&attr); Not a comment on this patch, but... technically, this call may fail. > ret = rte_ctrl_thread_create(&static_telemetry->thread_id, > telemetry_ctrl_thread, &attr, rte_telemetry_run_thread_func, > @@ -88,11 +288,21 @@ rte_telemetry_init() > int32_t > rte_telemetry_cleanup(void) > { > + int ret; > struct telemetry_impl *telemetry = static_telemetry; > + > + ret = close(telemetry->server_fd); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Close TELEMETRY socket failed"); > + free(telemetry); > + return -EPERM; > + } > + > telemetry->thread_status = 0; > pthread_join(telemetry->thread_id, NULL); > free(telemetry); > static_telemetry = NULL; > + > return 0; > } > > diff --git a/lib/librte_telemetry/rte_telemetry_internal.h b/lib/librte_telemetry/rte_telemetry_internal.h > index 4e810a8..569d56a 100644 > --- a/lib/librte_telemetry/rte_telemetry_internal.h > +++ b/lib/librte_telemetry/rte_telemetry_internal.h > @@ -24,9 +24,13 @@ extern int telemetry_log_level; > TELEMETRY_LOG(INFO, fmt, ## args) > > typedef struct telemetry_impl { > + int accept_fd; > + int server_fd; > pthread_t thread_id; > int thread_status; > uint32_t socket_id; > + int reg_index; > + int metrics_register_done; > } telemetry_impl; > > #endif >