From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 19521A0471 for ; Mon, 12 Aug 2019 12:03:59 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 548351BE82; Mon, 12 Aug 2019 12:03:58 +0200 (CEST) Received: from mail-vs1-f67.google.com (mail-vs1-f67.google.com [209.85.217.67]) by dpdk.org (Postfix) with ESMTP id B5DC51BE82 for ; Mon, 12 Aug 2019 12:03:56 +0200 (CEST) Received: by mail-vs1-f67.google.com with SMTP id u3so69430084vsh.6 for ; Mon, 12 Aug 2019 03:03:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=6gdYANRzB0oQ3XxPh+4/8m6ghsShS7Q3qpCDfdW8jhQ=; b=XLTTFK/a5nw31XcWen1KjekpUO4JLs4YuUrSMe/egBkgH7m5PRDhKnPoIeATv1P+bS WIKoZtdJvZgHknE8mM6dwgHkZ4A2smubbFwfxlgsrpxD07rkzU6wj8eebEUU2BDO3Fte oGH/ON3cwbQJegcDzW5lgYq7s6A6B/0i2XW30eK7OvDOfy6o0RHRJX1pLV0BdIY6S612 dzH9zGCOeqmnE3zpAFs5Sfx3qJE+t8jYdOsp5cqLLmFljHR2HxbZ7Kbtt3nlOXF2M718 Zjxxi3oQarnkx/82rrlETTZHx0ksizYgUPjLnLrRukc4qZFEs4PLFH5uZAiYHkIEvz0A +ykA== X-Gm-Message-State: APjAAAWmctd3HI8pUVGWXFgWHECmPo5WPHgUe4ij4sajqJ1OYWJcUJnK LdcjtUl5CYasF/Hd+VTqTW3O78EQf3pfYHTg2yzszg== X-Google-Smtp-Source: APXvYqy2YzLDIjjtz33rWyEMN4+hX/c3t7RL513FvTs6VnTsuPz2buNzWTYLQWcM9ChdCnhbOfZVbvJXp9x5+nWFimU= X-Received: by 2002:a67:3251:: with SMTP id y78mr11837445vsy.39.1565604236007; Mon, 12 Aug 2019 03:03:56 -0700 (PDT) MIME-Version: 1.0 References: <67a795e77bc9f5ac79ab78a878ae19abbead9f50.1563984454.git.anatoly.burakov@intel.com> In-Reply-To: <67a795e77bc9f5ac79ab78a878ae19abbead9f50.1563984454.git.anatoly.burakov@intel.com> From: David Marchand Date: Mon, 12 Aug 2019 12:03:44 +0200 Message-ID: To: Anatoly Burakov Cc: dev , Bruce Richardson , Stephen Hemminger , dpdk stable Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v4] eal: fix proc type auto detection X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Jul 24, 2019 at 6:08 PM Anatoly Burakov 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 > --- > > 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 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