From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <SRS0+vCZW=NC=ericsson.com=mattias.ronnblom@lysator.liu.se>
Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3])
by dpdk.org (Postfix) with ESMTP id ACB3C568A
for <dev@dpdk.org>; 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 <dev@dpdk.org>; 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 <kevin.laatz@intel.com>, 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 <ciara.power@intel.com>,
Brian Archbold <brian.archbold@intel.com>
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?= <mattias.ronnblom@ericsson.com>
Message-ID: <f175c961-59b8-fb11-dd1f-4ddc4e5cb0ae@ericsson.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <ciara.power@intel.com>
>
> 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 <ciara.power@intel.com>
> Signed-off-by: Brian Archbold <brian.archbold@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
> 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 <unistd.h>
> +#include <fcntl.h>
> #include <pthread.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
>
> #include <rte_eal.h>
> #include <rte_ethdev.h>
> #include <rte_metrics.h>
> #include <rte_option.h>
> +#include <rte_string_fns.h>
>
> #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
>