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

["Power, Ciara" <ciara.power@intel.com>]

>-----Original Message-----
>From: Kevin Traynor <ktraynor@redhat.com>
>Sent: Tuesday 5 October 2021 16:15
>To: Richardson, Bruce <bruce.richardson@intel.com>
>Cc: dev@dpdk.org; Power, Ciara <ciara.power@intel.com>; David Marchand
><david.marchand@redhat.com>; Burakov, Anatoly <anatoly.burakov@intel.com>
>Subject: Re: [PATCH v6 3/5] telemetry: use unique socket paths for in-memory
>mode
>
>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@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.

+1 for this approach of using a counter suffix, would be much simpler to use.

Thanks,
Ciara