[dpdk-stable] [dpdk-dev] [PATCH v4] eal: fix proc type auto detection

[David Marchand]

On Wed, Jul 24, 2019 at 6:08 PM Anatoly Burakov
<anatoly.burakov at intel.com> wrote:
>
> Currently, primary process holds an exclusive lock on the config
> file, thereby preventing other primaries from spinning up. However,
> when the primary dies, the lock is no longer being held, even though
> there might be other secondary processes still running.
>
> The fix is two-fold. First of all, downgrade the primary process's
> exclusive lock to a shared lock once we have it. Second of all,
> also take out shared locks on the config from the secondaries. We
> are using fcntl() locks, which get dropped when the file handle is
> closed, so also remove the closure of config file handle.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable at dpdk.org
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
> ---
>
> Notes:
>     v4:
>     - Fixed FreeBSD log message to match Linux version
>
>     v3:
>     - Added similar changes to FreeBSD version
>
>     v2:
>     - Adjusted indentation
>
>  lib/librte_eal/freebsd/eal/eal.c | 36 +++++++++++++++++++++++++++----
>  lib/librte_eal/linux/eal/eal.c   | 37 +++++++++++++++++++++++++++-----
>  2 files changed, 64 insertions(+), 9 deletions(-)
>
> diff --git a/lib/librte_eal/freebsd/eal/eal.c b/lib/librte_eal/freebsd/eal/eal.c
> index d53f0fe69..69995bf8f 100644
> --- a/lib/librte_eal/freebsd/eal/eal.c
> +++ b/lib/librte_eal/freebsd/eal/eal.c
> @@ -72,6 +72,13 @@ static struct flock wr_lock = {
>                 .l_len = sizeof(early_mem_config.memsegs),
>  };
>
> +static struct flock rd_lock = {
> +       .l_type = F_RDLCK,
> +       .l_whence = SEEK_SET,
> +       .l_start = offsetof(struct rte_mem_config, memsegs),
> +       .l_len = sizeof(early_mem_config.memsegs),
> +};
> +
>  /* Address of global and public configuration */
>  static struct rte_config rte_config = {
>                 .mem_config = &early_mem_config,
> @@ -249,8 +256,21 @@ rte_eal_config_create(void)
>         if (retval < 0){
>                 close(mem_cfg_fd);
>                 mem_cfg_fd = -1;
> -               RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another primary "
> -                       "process running?\n", pathname);
> +               RTE_LOG(ERR, EAL, "Cannot create exclusive lock on '%s'. "
> +                       "Is another process running?\n", pathname);
> +               return -1;
> +       }
> +
> +       /* we hold an exclusive lock - now downgrade it to a read lock to allow
> +        * other processes to also hold onto this file while preventing other
> +        * primaries from spinning up.
> +        */
> +       retval = fcntl(mem_cfg_fd, F_SETLK, &rd_lock);
> +       if (retval < 0) {
> +               close(mem_cfg_fd);
> +               mem_cfg_fd = -1;
> +               RTE_LOG(ERR, EAL, "Cannot downgrade to shared lock on '%s': %s\n",
> +                       pathname, strerror(errno));
>                 return -1;
>         }
>
> @@ -292,6 +312,16 @@ rte_eal_config_attach(void)
>                         return -1;
>                 }
>         }
> +       /* lock the file to prevent primary from initializing while this
> +        * process is still running.
> +        */
> +       if (fcntl(mem_cfg_fd, F_SETLK, &rd_lock) < 0) {
> +               close(mem_cfg_fd);
> +               mem_cfg_fd = -1;
> +               RTE_LOG(ERR, EAL, "Cannot create shared lock on '%s': %s\n",
> +                               pathname, strerror(errno));
> +               return -1;
> +       }
>
>         rte_mem_cfg_addr = mmap(NULL, sizeof(*rte_config.mem_config),
>                                 PROT_READ, MAP_SHARED, mem_cfg_fd, 0);
> @@ -330,8 +360,6 @@ rte_eal_config_reattach(void)
>         mem_config = (struct rte_mem_config *) mmap(rte_mem_cfg_addr,
>                         sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
>                         mem_cfg_fd, 0);
> -       close(mem_cfg_fd);
> -       mem_cfg_fd = -1;
>
>         if (mem_config == MAP_FAILED) {
>                 RTE_LOG(ERR, EAL, "Cannot mmap memory for rte_config! error %i (%s)\n",

We are missing a mem_cfg_fd cleanup if mmap failed.


> diff --git a/lib/librte_eal/linux/eal/eal.c b/lib/librte_eal/linux/eal/eal.c
> index 34db78753..0f0726703 100644
> --- a/lib/librte_eal/linux/eal/eal.c
> +++ b/lib/librte_eal/linux/eal/eal.c
> @@ -83,6 +83,13 @@ static struct flock wr_lock = {
>                 .l_len = sizeof(early_mem_config.memsegs),
>  };
>
> +static struct flock rd_lock = {
> +       .l_type = F_RDLCK,
> +       .l_whence = SEEK_SET,
> +       .l_start = offsetof(struct rte_mem_config, memsegs),
> +       .l_len = sizeof(early_mem_config.memsegs),
> +};
> +
>  /* Address of global and public configuration */
>  static struct rte_config rte_config = {
>                 .mem_config = &early_mem_config,
> @@ -343,8 +350,21 @@ rte_eal_config_create(void)
>         if (retval < 0){
>                 close(mem_cfg_fd);
>                 mem_cfg_fd = -1;
> -               RTE_LOG(ERR, EAL, "Cannot create lock on '%s'. Is another primary "
> -                       "process running?\n", pathname);
> +               RTE_LOG(ERR, EAL, "Cannot create exclusive lock on '%s'. "
> +                       "Is another process running?\n", pathname);
> +               return -1;
> +       }
> +
> +       /* we hold an exclusive lock - now downgrade it to a read lock to allow
> +        * other processes to also hold onto this file while preventing other
> +        * primaries from spinning up.
> +        */
> +       retval = fcntl(mem_cfg_fd, F_SETLK, &rd_lock);
> +       if (retval < 0) {
> +               close(mem_cfg_fd);
> +               mem_cfg_fd = -1;
> +               RTE_LOG(ERR, EAL, "Cannot downgrade to shared lock on '%s': %s\n",
> +                       pathname, strerror(errno));
>                 return -1;
>         }
>
> @@ -389,6 +409,16 @@ rte_eal_config_attach(void)
>                         return -1;
>                 }
>         }
> +       /* lock the file to prevent primary from initializing while this
> +        * process is still running.
> +        */
> +       if (fcntl(mem_cfg_fd, F_SETLK, &rd_lock) < 0) {
> +               close(mem_cfg_fd);
> +               mem_cfg_fd = -1;
> +               RTE_LOG(ERR, EAL, "Cannot create shared lock on '%s': %s\n",
> +                               pathname, strerror(errno));
> +               return -1;
> +       }
>
>         /* map it as read-only first */
>         mem_config = (struct rte_mem_config *) mmap(NULL, sizeof(*mem_config),
> @@ -427,9 +457,6 @@ rte_eal_config_reattach(void)
>                         sizeof(*mem_config), PROT_READ | PROT_WRITE, MAP_SHARED,
>                         mem_cfg_fd, 0);
>
> -       close(mem_cfg_fd);
> -       mem_cfg_fd = -1;
> -
>         if (mem_config == MAP_FAILED || mem_config != rte_mem_cfg_addr) {
>                 if (mem_config != MAP_FAILED) {
>                         /* errno is stale, don't use */

Idem.

Reviewed-by: David Marchand <david.marchand at redhat.com>

With https://patchwork.dpdk.org/patch/56501/, the code is now really
close between Linux and FreeBSD, could it go to common entirely?

-- 
David Marchand