DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: Anatoly Burakov <anatoly.burakov@intel.com>
Cc: dev <dev@dpdk.org>, Bruce Richardson <bruce.richardson@intel.com>,
	 Stephen Hemminger <stephen@networkplumber.org>,
	dpdk stable <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v4] eal: fix proc type auto detection
Date: Mon, 12 Aug 2019 12:03:44 +0200
Message-ID: <CAJFAV8w1i_6v7oA01ETJgWcwSWh2m=+8jj+M7cn_BE36GXKbOg@mail.gmail.com> (raw)
In-Reply-To: <67a795e77bc9f5ac79ab78a878ae19abbead9f50.1563984454.git.anatoly.burakov@intel.com>

On Wed, Jul 24, 2019 at 6:08 PM Anatoly Burakov
<anatoly.burakov@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@dpdk.org
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@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@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

  parent reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-23 13:19 [dpdk-dev] [PATCH] " Anatoly Burakov
2019-07-23 18:38 ` Stephen Hemminger
2019-07-24 10:04 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
2019-07-24 16:04   ` [dpdk-dev] [PATCH v3] " Anatoly Burakov
2019-07-24 16:07     ` [dpdk-dev] [PATCH v4] " Anatoly Burakov
2019-07-30  8:13       ` Thomas Monjalon
2019-07-30  9:19         ` Burakov, Anatoly
2019-08-12 10:03       ` David Marchand [this message]
2019-08-12 10:21         ` Van Haaren, Harry
2019-08-12 13:30           ` Burakov, Anatoly
2019-08-12 13:31         ` Burakov, Anatoly

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAJFAV8w1i_6v7oA01ETJgWcwSWh2m=+8jj+M7cn_BE36GXKbOg@mail.gmail.com' \
    --to=david.marchand@redhat.com \
    --cc=anatoly.burakov@intel.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=stable@dpdk.org \
    --cc=stephen@networkplumber.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox