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