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

[Kevin Traynor]

On 05/10/2021 15:52, Bruce Richardson wrote:
> On Tue, Oct 05, 2021 at 03:41:17PM +0100, Kevin Traynor wrote:
>> Hi Bruce, I started reviewing v5 before v6 was pushed, so these are just
>> comments from v5,
>>
> 
> No problem. Changes V6-v5 are fairly small anyway. Thanks for the review.
> 
>> 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 at 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?
>>
> 
> Yes, that is an interesting idea, and probably not a bad one.
> 
> Taking things further, I wonder if using the pid is the best choice for a
> suffix for the non-single-process case. If we used .1, .2 etc. it would
> make things more predictable, perhaps easing integration for other tools.
> Each process starting up would use the lowest free suffix, and any
> external monitoring tools could just be set up to monitor the
> first/second/third instance, with reproducable names across process
> restarts.
> 

ok, cool - that sounds better again. Probably you can post and see if 
anyone else finds a hole in it or has a better idea for the next few days.

>>> +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
>>
> 
> I don't think it actually matters since any non-zero value will come
> through as "true". However, will change it in v7.
> 

It was just a nit-pick, I admit it :-)

> /Bruce
>