Re: [dpdk-dev] [PATCH] eal: avoid unnecessary conflicts over rte_config file

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/14/2016 5:27 PM, ouster at cs.stanford.edu (John Ousterhout) wrote:
> It sounds like my patch would break some existing software, so it probably
> doesn't make sense right now.
> 
> I'd still argue that the current mechanism has a number of problems, and it
> should probably undergo a comprehensive overhaul at some point in the
> future.

Hi John,

It seems there weren't any conclusion on the patch and it is sitting on
patchwork more than two years, I am updating it as rejected, if
it is still relevant please let us know.

Sorry for any inconvenience caused.

For record, patch: https://patches.dpdk.org/patch/16527/

Regards,
ferruh

> 
> -John-
> 
> On Thu, Oct 13, 2016 at 2:39 PM, Tahhan, Maryam <maryam.tahhan at intel.com>
> wrote:
> 
>>> Hi John,
>>>
>>>> Before this patch, DPDK used the file ~/.rte_config as a lock to
>>>> detect potential interference between multiple DPDK applications
>>>> running on the same machine. However, if a single user ran DPDK
>>>> applications concurrently on several different machines, and if the
>>>> user's home directory was shared between the machines via NFS, DPDK
>>>> would incorrectly detect conflicts for all but the first application
>>>> and abort them. This patch fixes the problem by incorporating the
>>>> machine name into the config file name (e.g., ~/.rte_hostname_config).
>>>>
>>>> Signed-off-by: John Ousterhout <ouster at cs.stanford.edu>
>>>> ---
>>>>  doc/guides/prog_guide/multi_proc_support.rst | 11 +++++++----
>>>>  lib/librte_eal/common/eal_common_proc.c      |  8 ++------
>>>>  lib/librte_eal/common/eal_filesystem.h       | 15 +++++++++++++--
>>>>  3 files changed, 22 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/doc/guides/prog_guide/multi_proc_support.rst
>>>> b/doc/guides/prog_guide/multi_proc_support.rst
>>>> index badd102..a54fa1c 100644
>>>> --- a/doc/guides/prog_guide/multi_proc_support.rst
>>>> +++ b/doc/guides/prog_guide/multi_proc_support.rst
>>>> @@ -129,10 +129,13 @@ Support for this usage scenario is provided
>>>> using the ``--file-prefix`` paramete
>>>>
>>>>  By default, the EAL creates hugepage files on each hugetlbfs
>>>> filesystem using the rtemap_X filename,  where X is in the range 0 to
>> the
>>> maximum number of hugepages -1.
>>>> -Similarly, it creates shared configuration files, memory mapped in
>>>> each process, using the /var/run/.rte_config filename, -when run as
>>>> root (or $HOME/.rte_config when run as a non-root user; -if filesystem
>> and
>>> device permissions are set up to allow this).
>>>> -The rte part of the filenames of each of the above is configurable
>> using the
>>> file-prefix parameter.
>>>> +Similarly, it creates shared configuration files, memory mapped in
>> each
>>> process.
>>>> +When run as root, the name of the configuration file will be
>>>> +/var/run/.rte_*host*_config, where *host* is the name of the machine.
>>>> +When run as a non-root user, the the name of the configuration file
>>>> +will be $HOME/.rte_*host*_config (if filesystem and device permissions
>>> are set up to allow this).
>>>> +If the ``--file-prefix`` parameter has been specified, its value will
>>>> +be used in place of "rte" in the file names.
>>>
>>> I am not sure that we need to handle all such cases inside EAL.
>>> User can easily overcome that problem by just adding something like:
>>> --file-prefix=`uname -n`
>>> to his command-line.
>>> Konstantin
>>>
>>
>> I agree with Konstantin, there's no need to include the hostname in the
>> rte config file + I'm not sure this will be backward compatible with
>> existing DPDK applications that use a secondary processes that use the
>> config file (as in, multiprocess DPDK applications in use would all need to
>> be updated). What Konstantin suggests fixes the issue you were encountering
>> without breaking backward compatibility.
>> Is addition, the hostname is not unique... you could in theory have 2
>> hosts with the same hostname and encounter the issue you were seeing again.
>>
>>>>
>>>>  In addition to specifying the file-prefix parameter,  any DPDK
>>>> applications that are to be run side-by-side must explicitly limit
>> their
>>> memory use.
>>>> diff --git a/lib/librte_eal/common/eal_common_proc.c
>>>> b/lib/librte_eal/common/eal_common_proc.c
>>>> index 12e0fca..517aa0c 100644
>>>> --- a/lib/librte_eal/common/eal_common_proc.c
>>>> +++ b/lib/librte_eal/common/eal_common_proc.c
>>>> @@ -45,12 +45,8 @@ rte_eal_primary_proc_alive(const char
>>>> *config_file_path)
>>>>
>>>>     if (config_file_path)
>>>>             config_fd = open(config_file_path, O_RDONLY);
>>>> -   else {
>>>> -           char default_path[PATH_MAX+1];
>>>> -           snprintf(default_path, PATH_MAX, RUNTIME_CONFIG_FMT,
>>>> -                    default_config_dir, "rte");
>>>> -           config_fd = open(default_path, O_RDONLY);
>>>> -   }
>>>> +   else
>>>> +           config_fd = open(eal_runtime_config_path(), O_RDONLY);
>>>>     if (config_fd < 0)
>>>>             return 0;
>>>>
>>>> diff --git a/lib/librte_eal/common/eal_filesystem.h
>>>> b/lib/librte_eal/common/eal_filesystem.h
>>>> index fdb4a70..4929aa3 100644
>>>> --- a/lib/librte_eal/common/eal_filesystem.h
>>>> +++ b/lib/librte_eal/common/eal_filesystem.h
>>>> @@ -41,7 +41,7 @@
>>>>  #define EAL_FILESYSTEM_H
>>>>
>>>>  /** Path of rte config file. */
>>>> -#define RUNTIME_CONFIG_FMT "%s/.%s_config"
>>>> +#define RUNTIME_CONFIG_FMT "%s/.%s_%s_config"
>>>>
>>>>  #include <stdint.h>
>>>>  #include <limits.h>
>>>> @@ -59,11 +59,22 @@ eal_runtime_config_path(void)
>>>>     static char buffer[PATH_MAX]; /* static so auto-zeroed */
>>>>     const char *directory = default_config_dir;
>>>>     const char *home_dir = getenv("HOME");
>>>> +   static char nameBuffer[1000];
>>>> +   int result;
>>>>
>>>>     if (getuid() != 0 && home_dir != NULL)
>>>>             directory = home_dir;
>>>> +
>>>> +   /*
>>>> +    * Include the name of the host in the config file path. Otherwise,
>>>> +    * if DPDK applications run on different hosts but share a home
>>>> +    * directory (e.g. via NFS), they will choose the same config
>>>> +    * file and conflict unnecessarily.
>>>> +    */
>>>> +   result = gethostname(nameBuffer, sizeof(nameBuffer)-1);
>>>>     snprintf(buffer, sizeof(buffer) - 1, RUNTIME_CONFIG_FMT,
>>> directory,
>>>> -                   internal_config.hugefile_prefix);
>>>> +                   internal_config.hugefile_prefix,
>>>> +           (result == 0) ? nameBuffer : "unknown-host");
>>>>     return buffer;
>>>>  }
>>>>
>>>> --
>>>> 2.8.3
>>
>>
>