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 ACB3C568A for ; Mon, 22 Oct 2018 15:50:16 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 68B254002B for ; Mon, 22 Oct 2018 15:50:15 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 4E16040021; Mon, 22 Oct 2018 15:50:15 +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-173-200.mobileonline.telia.com [90.232.173.200]) (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 C23AD40005; Mon, 22 Oct 2018 15:50:12 +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, bruce.richardson@intel.com, Ciara Power , Brian Archbold References: <20181016155802.2067-1-kevin.laatz@intel.com> <20181022110014.82153-1-kevin.laatz@intel.com> <20181022110014.82153-5-kevin.laatz@intel.com> From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= Message-ID: Date: Mon, 22 Oct 2018 15:50:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20181022110014.82153-5-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 v6 04/13] 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: Mon, 22 Oct 2018 13:50:16 -0000 On 2018-10-22 13:00, 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 > Acked-by: Harry van Haaren > --- > lib/librte_telemetry/rte_telemetry.c | 226 ++++++++++++++++++++++++++ > lib/librte_telemetry/rte_telemetry_internal.h | 4 + > 2 files changed, 230 insertions(+) > > diff --git a/lib/librte_telemetry/rte_telemetry.c b/lib/librte_telemetry/rte_telemetry.c > index ee9c90f..10311be 100644 > --- a/lib/librte_telemetry/rte_telemetry.c > +++ b/lib/librte_telemetry/rte_telemetry.c > @@ -3,23 +3,163 @@ > */ > > #include > +#include > #include > +#include > +#include > > #include > #include > #include > #include > +#include > > #include "rte_telemetry.h" > #include "rte_telemetry_internal.h" > > +#define BUF_SIZE 1024 > #define SLEEP_TIME 10 > > static telemetry_impl *static_telemetry; > > +static void > +rte_telemetry_get_runtime_dir(char *socket_path, size_t size) > +{ > + snprintf(socket_path, size, "%s/telemetry", rte_eal_get_runtime_dir()); > +} > + > +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 == NULL) { > + 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 == NULL) { > + 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; > + > +free_xstats: > + free(eth_xstats); > + free(eth_xstats_names); > + return ret_val; > +} > + > +static int32_t > +rte_telemetry_initial_accept(struct telemetry_impl *telemetry) > +{ > + uint16_t pid; > + > + 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) { Is this intentional? If so, what's the meaning of "accept_fd == 0", as oppose to when accept_fd is -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 && > + 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 == NULL) { > @@ -27,6 +167,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; > } > > @@ -50,6 +196,68 @@ static void > pthread_exit(0); > } > > +static int32_t > +rte_telemetry_set_socket_nonblock(int fd) > +{ > + int flags; > + > + if (fd < 0) { > + TELEMETRY_LOG_ERR("Invalid fd provided"); > + return -1; > + } > + > + flags = fcntl(fd, F_GETFL, 0); > + 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; > + char socket_path[BUF_SIZE]; > + > + if (telemetry == NULL) > + return -1; > + > + telemetry->server_fd = socket(AF_UNIX, SOCK_SEQPACKET, 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"); > + goto close_socket; > + } > + > + addr.sun_family = AF_UNIX; > + rte_telemetry_get_runtime_dir(socket_path, sizeof(socket_path)); > + 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"); > + goto close_socket; > + } > + > + return 0; > + > +close_socket: > + if (close(telemetry->server_fd) < 0) { > + TELEMETRY_LOG_ERR("Close TELEMETRY socket failed"); > + free(telemetry); This free() needs to go away, or use-after-free and a double-free() will occur in rte_telemetry_init(). > + return -EPERM; > + } > + > + return -1; > +} > + > int32_t > rte_telemetry_init() > { > @@ -77,6 +285,14 @@ rte_telemetry_init() > return -EPERM; > } > > + 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; > + } > + > ret = rte_ctrl_thread_create(&static_telemetry->thread_id, > telemetry_ctrl_thread, &attr, rte_telemetry_run_thread_func, > (void *)static_telemetry); > @@ -95,11 +311,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 >