Re: [dpdk-dev] [PATCH v6 3/5] telemetry: use unique socket paths for in-memory mode

[Kevin Traynor <ktraynor@redhat.com>]

Hi Bruce, I started reviewing v5 before v6 was pushed, so these are just 
comments from v5,

On 05/10/2021 14:59, Bruce Richardson wrote:
> When DPDK is run using "in-memory" flag, multiple processes can be run
> using the same file-prefix and hence the same runtime directory. To
> avoid problems with conflicting telemetry unix socket paths, we can
> put the pid of the process into the socket name. As with the existing
> telemetry socket files, these sockets are removed on normal program
> exit.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   doc/guides/howto/telemetry.rst     | 17 ++++++++++++++++-
>   lib/eal/freebsd/eal.c              |  1 +
>   lib/eal/linux/eal.c                |  1 +
>   lib/telemetry/telemetry.c          | 15 ++++++++++++---
>   lib/telemetry/telemetry_internal.h |  3 ++-
>   5 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/guides/howto/telemetry.rst b/doc/guides/howto/telemetry.rst
> index 8f4fa1a510..8a61302459 100644
> --- a/doc/guides/howto/telemetry.rst
> +++ b/doc/guides/howto/telemetry.rst
> @@ -13,12 +13,27 @@ ethdev port list, and eal parameters.
>   Telemetry Interface
>   -------------------
>   
> -The :doc:`../prog_guide/telemetry_lib` opens a socket with path
> +For applications run normally, i.e. without the `--in-memory` EAL flag,
> +the :doc:`../prog_guide/telemetry_lib` opens a socket with path
>   *<runtime_directory>/dpdk_telemetry.<version>*. The version represents the
>   telemetry version, the latest is v2. For example, a client would connect to a
>   socket with path  */var/run/dpdk/\*/dpdk_telemetry.v2* (when the primary process
>   is run by a root user).
>   
> +For applications run with the `--in-memory` EAL flag,
> +the socket file is created with an additional suffix of the process PID.
> +This is because multiple independent DPDK processes can be run simultaneously
> +using the same runtime directory when *in-memory* mode is used.
> +For example, when a user with UID 1000 runs processes with in-memory mode,
> +we would find sockets available such as::
> +
> +  /run/user/1000/dpdk/rte/dpdk_telemetry.v2.1982
> +  /run/user/1000/dpdk/rte/dpdk_telemetry.v2.1935
> +

It seems an unnecessary step unless there is multiple process. As a 
suggestion, how about "simplifying" by always adding a check for an 
active socket with the default name. If it is not found, create it with 
the default (pre patches) name. If it is found and active, create a new 
one with process id appended. e.g.

First:
/run/user/1000/dpdk/rte/dpdk_telemetry.v2

Next:
/run/user/1000/dpdk/rte/dpdk_telemetry.v2.1982
/run/user/1000/dpdk/rte/dpdk_telemetry.v2.1935

It means that existing socket can still be used by anything using 
telemetry. I think it is a nice default to keep as it doesn't require 
any changes for anything that will connect (e.g. collectd?)

The downside is that one will have a different name but it seems an 
acceptable trade-off to keep compatibility for single process case.

WDYT?

> +Where `/run/user/<uid>` is the runtime directory for the user given by the
> +`$XDG_RUNTIME_DIR` environment variable,
> +and `rte` is the default DPDK file prefix used for a runtime directory.
> +
>   
>   Telemetry Initialization
>   ------------------------
> diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
> index b06a2c1662..ed39d10b4e 100644
> --- a/lib/eal/freebsd/eal.c
> +++ b/lib/eal/freebsd/eal.c
> @@ -952,6 +952,7 @@ rte_eal_init(int argc, char **argv)
>   		if (tlog < 0)
>   			tlog = RTE_LOGTYPE_EAL;
>   		if (rte_telemetry_init(rte_eal_get_runtime_dir(),
> +				internal_conf->in_memory | internal_conf->no_shconf,
>   				rte_version(),
>   				&internal_conf->ctrl_cpuset, rte_log, tlog) != 0)
>   			return -1;
> diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
> index 0d0fc66668..9db4eb7913 100644
> --- a/lib/eal/linux/eal.c
> +++ b/lib/eal/linux/eal.c
> @@ -1326,6 +1326,7 @@ rte_eal_init(int argc, char **argv)
>   		if (tlog < 0)
>   			tlog = RTE_LOGTYPE_EAL;
>   		if (rte_telemetry_init(rte_eal_get_runtime_dir(),
> +				internal_conf->in_memory | internal_conf->no_shconf,

should be logical OR

>   				rte_version(),
>   				&internal_conf->ctrl_cpuset, rte_log, tlog) != 0)
>   			return -1;
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> index bd804a25a9..24441d100b 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -51,6 +51,7 @@ static struct socket v1_socket; /* socket for v1 telemetry */
>   
>   static const char *telemetry_version; /* save rte_version */
>   static const char *socket_dir;        /* runtime directory */
> +static bool socket_uses_pid;          /* for in-memory mode, we need different socket paths */
>   static rte_cpuset_t *thread_cpuset;
>   static rte_log_fn rte_log_ptr;
>   static uint32_t logtype;
> @@ -432,8 +433,14 @@ static inline char *
>   get_socket_path(const char *runtime_dir, const int version)
>   {
>   	static char path[PATH_MAX];
> -	snprintf(path, sizeof(path), "%s/dpdk_telemetry.v%d",
> -			strlen(runtime_dir) ? runtime_dir : "/tmp", version);
> +	if (!socket_uses_pid)
> +		snprintf(path, sizeof(path), "%s/dpdk_telemetry.v%d",
> +				strlen(runtime_dir) ? runtime_dir : "/tmp", version);
> +	else
> +		snprintf(path, sizeof(path), "%s/dpdk_telemetry.v%d.%u",
> +				strlen(runtime_dir) ? runtime_dir : "/tmp",
> +				version,
> +				(unsigned int)getpid());
>   	return path;
>   }
>   
> @@ -591,11 +598,13 @@ telemetry_v2_init(void)
>   #endif /* !RTE_EXEC_ENV_WINDOWS */
>   
>   int32_t
> -rte_telemetry_init(const char *runtime_dir, const char *rte_version, rte_cpuset_t *cpuset,
> +rte_telemetry_init(const char *runtime_dir, bool in_memory,
> +		const char *rte_version, rte_cpuset_t *cpuset,
>   		rte_log_fn log_fn, uint32_t registered_logtype)
>   {
>   	telemetry_version = rte_version;
>   	socket_dir = runtime_dir;
> +	socket_uses_pid = in_memory; /* for in-memory mode use pid in sock path for uniqueness */
>   	thread_cpuset = cpuset;
>   	rte_log_ptr = log_fn;
>   	logtype = registered_logtype;
> diff --git a/lib/telemetry/telemetry_internal.h b/lib/telemetry/telemetry_internal.h
> index d085c492dc..d8fb37a633 100644
> --- a/lib/telemetry/telemetry_internal.h
> +++ b/lib/telemetry/telemetry_internal.h
> @@ -109,7 +109,8 @@ typedef int (*rte_log_fn)(uint32_t level, uint32_t logtype, const char *format,
>    */
>   __rte_internal
>   int
> -rte_telemetry_init(const char *runtime_dir, const char *rte_version, rte_cpuset_t *cpuset,
> +rte_telemetry_init(const char *runtime_dir, bool in_memory,
> +		const char *rte_version, rte_cpuset_t *cpuset,
>   		rte_log_fn log_fn, uint32_t registered_logtype);
>   
>   #endif
>