[dpdk-dev] [PATCH] eal: check if hugedir write lock is already being held

[dpdk-dev] [PATCH] eal: check if hugedir write lock is already being held

[Anatoly Burakov]

At hugepage info initialization, EAL takes out a write lock on
hugetlbfs directories, and drops it after the memory init is
finished. However, in non-legacy mode, if "-m" or "--socket-mem"
switches are passed, this leads to a deadlock because EAL tries
to allocate pages (and thus take out a write lock on hugedir)
while still holding a separate hugedir write lock in EAL.

Fix it by checking if write lock in hugepage info is active, and
not trying to lock the directory if the hugedir fd is valid.

Fixes: 1a7dc2252f28 ("mem: revert to using flock and add per-segment lockfiles")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 71 ++++++++++++++++++------------
 1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 00d7886..360d8f7 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -666,7 +666,7 @@ alloc_seg_walk(const struct rte_memseg_list *msl, void *arg)
 	struct alloc_walk_param *wa = arg;
 	struct rte_memseg_list *cur_msl;
 	size_t page_sz;
-	int cur_idx, start_idx, j, dir_fd;
+	int cur_idx, start_idx, j, dir_fd = -1;
 	unsigned int msl_idx, need, i;
 
 	if (msl->page_sz != wa->page_sz)
@@ -691,19 +691,24 @@ alloc_seg_walk(const struct rte_memseg_list *msl, void *arg)
 	 * because file creation and locking operations are not atomic,
 	 * and we might be the first or the last ones to use a particular page,
 	 * so we need to ensure atomicity of every operation.
+	 *
+	 * during init, we already hold a write lock, so don't try to take out
+	 * another one.
 	 */
-	dir_fd = open(wa->hi->hugedir, O_RDONLY);
-	if (dir_fd < 0) {
-		RTE_LOG(ERR, EAL, "%s(): Cannot open '%s': %s\n", __func__,
-			wa->hi->hugedir, strerror(errno));
-		return -1;
-	}
-	/* blocking writelock */
-	if (flock(dir_fd, LOCK_EX)) {
-		RTE_LOG(ERR, EAL, "%s(): Cannot lock '%s': %s\n", __func__,
-			wa->hi->hugedir, strerror(errno));
-		close(dir_fd);
-		return -1;
+	if (wa->hi->lock_descriptor == -1) {
+		dir_fd = open(wa->hi->hugedir, O_RDONLY);
+		if (dir_fd < 0) {
+			RTE_LOG(ERR, EAL, "%s(): Cannot open '%s': %s\n",
+				__func__, wa->hi->hugedir, strerror(errno));
+			return -1;
+		}
+		/* blocking writelock */
+		if (flock(dir_fd, LOCK_EX)) {
+			RTE_LOG(ERR, EAL, "%s(): Cannot lock '%s': %s\n",
+				__func__, wa->hi->hugedir, strerror(errno));
+			close(dir_fd);
+			return -1;
+		}
 	}
 
 	for (i = 0; i < need; i++, cur_idx++) {
@@ -742,7 +747,8 @@ alloc_seg_walk(const struct rte_memseg_list *msl, void *arg)
 			if (wa->ms)
 				memset(wa->ms, 0, sizeof(*wa->ms) * wa->n_segs);
 
-			close(dir_fd);
+			if (dir_fd >= 0)
+				close(dir_fd);
 			return -1;
 		}
 		if (wa->ms)
@@ -754,7 +760,8 @@ alloc_seg_walk(const struct rte_memseg_list *msl, void *arg)
 	wa->segs_allocated = i;
 	if (i > 0)
 		cur_msl->version++;
-	close(dir_fd);
+	if (dir_fd >= 0)
+		close(dir_fd);
 	return 1;
 }
 
@@ -769,7 +776,7 @@ free_seg_walk(const struct rte_memseg_list *msl, void *arg)
 	struct rte_memseg_list *found_msl;
 	struct free_walk_param *wa = arg;
 	uintptr_t start_addr, end_addr;
-	int msl_idx, seg_idx, ret, dir_fd;
+	int msl_idx, seg_idx, ret, dir_fd = -1;
 
 	start_addr = (uintptr_t) msl->base_va;
 	end_addr = start_addr + msl->memseg_arr.len * (size_t)msl->page_sz;
@@ -788,19 +795,24 @@ free_seg_walk(const struct rte_memseg_list *msl, void *arg)
 	 * because file creation and locking operations are not atomic,
 	 * and we might be the first or the last ones to use a particular page,
 	 * so we need to ensure atomicity of every operation.
+	 *
+	 * during init, we already hold a write lock, so don't try to take out
+	 * another one.
 	 */
-	dir_fd = open(wa->hi->hugedir, O_RDONLY);
-	if (dir_fd < 0) {
-		RTE_LOG(ERR, EAL, "%s(): Cannot open '%s': %s\n", __func__,
-			wa->hi->hugedir, strerror(errno));
-		return -1;
-	}
-	/* blocking writelock */
-	if (flock(dir_fd, LOCK_EX)) {
-		RTE_LOG(ERR, EAL, "%s(): Cannot lock '%s': %s\n", __func__,
-			wa->hi->hugedir, strerror(errno));
-		close(dir_fd);
-		return -1;
+	if (wa->hi->lock_descriptor == -1) {
+		dir_fd = open(wa->hi->hugedir, O_RDONLY);
+		if (dir_fd < 0) {
+			RTE_LOG(ERR, EAL, "%s(): Cannot open '%s': %s\n",
+				__func__, wa->hi->hugedir, strerror(errno));
+			return -1;
+		}
+		/* blocking writelock */
+		if (flock(dir_fd, LOCK_EX)) {
+			RTE_LOG(ERR, EAL, "%s(): Cannot lock '%s': %s\n",
+				__func__, wa->hi->hugedir, strerror(errno));
+			close(dir_fd);
+			return -1;
+		}
 	}
 
 	found_msl->version++;
@@ -809,7 +821,8 @@ free_seg_walk(const struct rte_memseg_list *msl, void *arg)
 
 	ret = free_seg(wa->ms, wa->hi, msl_idx, seg_idx);
 
-	close(dir_fd);
+	if (dir_fd >= 0)
+		close(dir_fd);
 
 	if (ret < 0)
 		return -1;
-- 
2.7.4

Re: [dpdk-dev] [PATCH] eal: check if hugedir write lock is already being held

[Shahaf Shuler]

Monday, April 30, 2018 1:38 PM, Anatoly Burakov:
> Cc: arybchenko@solarflare.com; anatoly.burakov@intel.com
> Subject: [dpdk-dev] [PATCH] eal: check if hugedir write lock is already being
> held
> 
> At hugepage info initialization, EAL takes out a write lock on hugetlbfs
> directories, and drops it after the memory init is finished. However, in non-
> legacy mode, if "-m" or "--socket-mem"
> switches are passed, this leads to a deadlock because EAL tries to allocate
> pages (and thus take out a write lock on hugedir) while still holding a
> separate hugedir write lock in EAL.
> 
> Fix it by checking if write lock in hugepage info is active, and not trying to lock
> the directory if the hugedir fd is valid.
> 
> Fixes: 1a7dc2252f28 ("mem: revert to using flock and add per-segment
> lockfiles")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_memalloc.c | 71 ++++++++++++++++++------
> ------
>  1 file changed, 42 insertions(+), 29 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> index 00d7886..360d8f7 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> @@ -666,7 +666,7 @@ alloc_seg_walk(const struct rte_memseg_list *msl,
> void *arg)
>  	struct alloc_walk_param *wa = arg;
>  	struct rte_memseg_list *cur_msl;
>  	size_t page_sz;
> -	int cur_idx, start_idx, j, dir_fd;
> +	int cur_idx, start_idx, j, dir_fd = -1;
>  	unsigned int msl_idx, need, i;
> 
>  	if (msl->page_sz != wa->page_sz)
> @@ -691,19 +691,24 @@ alloc_seg_walk(const struct rte_memseg_list *msl,
> void *arg)
>  	 * because file creation and locking operations are not atomic,
>  	 * and we might be the first or the last ones to use a particular page,
>  	 * so we need to ensure atomicity of every operation.
> +	 *
> +	 * during init, we already hold a write lock, so don't try to take out
> +	 * another one.
>  	 */
> -	dir_fd = open(wa->hi->hugedir, O_RDONLY);
> -	if (dir_fd < 0) {
> -		RTE_LOG(ERR, EAL, "%s(): Cannot open '%s': %s\n",
> __func__,
> -			wa->hi->hugedir, strerror(errno));
> -		return -1;
> -	}
> -	/* blocking writelock */
> -	if (flock(dir_fd, LOCK_EX)) {
> -		RTE_LOG(ERR, EAL, "%s(): Cannot lock '%s': %s\n", __func__,
> -			wa->hi->hugedir, strerror(errno));
> -		close(dir_fd);
> -		return -1;
> +	if (wa->hi->lock_descriptor == -1) {
> +		dir_fd = open(wa->hi->hugedir, O_RDONLY);
> +		if (dir_fd < 0) {
> +			RTE_LOG(ERR, EAL, "%s(): Cannot open '%s': %s\n",
> +				__func__, wa->hi->hugedir, strerror(errno));
> +			return -1;
> +		}
> +		/* blocking writelock */
> +		if (flock(dir_fd, LOCK_EX)) {
> +			RTE_LOG(ERR, EAL, "%s(): Cannot lock '%s': %s\n",
> +				__func__, wa->hi->hugedir, strerror(errno));
> +			close(dir_fd);
> +			return -1;
> +		}
>  	}
> 
>  	for (i = 0; i < need; i++, cur_idx++) { @@ -742,7 +747,8 @@
> alloc_seg_walk(const struct rte_memseg_list *msl, void *arg)
>  			if (wa->ms)
>  				memset(wa->ms, 0, sizeof(*wa->ms) * wa-
> >n_segs);
> 
> -			close(dir_fd);
> +			if (dir_fd >= 0)
> +				close(dir_fd);
>  			return -1;
>  		}
>  		if (wa->ms)
> @@ -754,7 +760,8 @@ alloc_seg_walk(const struct rte_memseg_list *msl,
> void *arg)
>  	wa->segs_allocated = i;
>  	if (i > 0)
>  		cur_msl->version++;
> -	close(dir_fd);
> +	if (dir_fd >= 0)
> +		close(dir_fd);
>  	return 1;
>  }
> 
> @@ -769,7 +776,7 @@ free_seg_walk(const struct rte_memseg_list *msl,
> void *arg)
>  	struct rte_memseg_list *found_msl;
>  	struct free_walk_param *wa = arg;
>  	uintptr_t start_addr, end_addr;
> -	int msl_idx, seg_idx, ret, dir_fd;
> +	int msl_idx, seg_idx, ret, dir_fd = -1;
> 
>  	start_addr = (uintptr_t) msl->base_va;
>  	end_addr = start_addr + msl->memseg_arr.len * (size_t)msl-
> >page_sz; @@ -788,19 +795,24 @@ free_seg_walk(const struct
> rte_memseg_list *msl, void *arg)
>  	 * because file creation and locking operations are not atomic,
>  	 * and we might be the first or the last ones to use a particular page,
>  	 * so we need to ensure atomicity of every operation.
> +	 *
> +	 * during init, we already hold a write lock, so don't try to take out
> +	 * another one.
>  	 */
> -	dir_fd = open(wa->hi->hugedir, O_RDONLY);
> -	if (dir_fd < 0) {
> -		RTE_LOG(ERR, EAL, "%s(): Cannot open '%s': %s\n",
> __func__,
> -			wa->hi->hugedir, strerror(errno));
> -		return -1;
> -	}
> -	/* blocking writelock */
> -	if (flock(dir_fd, LOCK_EX)) {
> -		RTE_LOG(ERR, EAL, "%s(): Cannot lock '%s': %s\n", __func__,
> -			wa->hi->hugedir, strerror(errno));
> -		close(dir_fd);
> -		return -1;
> +	if (wa->hi->lock_descriptor == -1) {
> +		dir_fd = open(wa->hi->hugedir, O_RDONLY);
> +		if (dir_fd < 0) {
> +			RTE_LOG(ERR, EAL, "%s(): Cannot open '%s': %s\n",
> +				__func__, wa->hi->hugedir, strerror(errno));
> +			return -1;
> +		}
> +		/* blocking writelock */
> +		if (flock(dir_fd, LOCK_EX)) {
> +			RTE_LOG(ERR, EAL, "%s(): Cannot lock '%s': %s\n",
> +				__func__, wa->hi->hugedir, strerror(errno));
> +			close(dir_fd);
> +			return -1;
> +		}
>  	}
> 
>  	found_msl->version++;
> @@ -809,7 +821,8 @@ free_seg_walk(const struct rte_memseg_list *msl,
> void *arg)
> 
>  	ret = free_seg(wa->ms, wa->hi, msl_idx, seg_idx);
> 
> -	close(dir_fd);
> +	if (dir_fd >= 0)
> +		close(dir_fd);
> 
>  	if (ret < 0)
>  		return -1;

Tested-By: Shahaf Shuler <shahafs@mellanox.com> 

> --
> 2.7.4

Re: [dpdk-dev] [PATCH] eal: check if hugedir write lock is already being held

[Andrew Rybchenko]

On 04/30/2018 01:38 PM, Anatoly Burakov wrote:
> At hugepage info initialization, EAL takes out a write lock on
> hugetlbfs directories, and drops it after the memory init is
> finished. However, in non-legacy mode, if "-m" or "--socket-mem"
> switches are passed, this leads to a deadlock because EAL tries
> to allocate pages (and thus take out a write lock on hugedir)
> while still holding a separate hugedir write lock in EAL.
>
> Fix it by checking if write lock in hugepage info is active, and
> not trying to lock the directory if the hugedir fd is valid.
>
> Fixes: 1a7dc2252f28 ("mem: revert to using flock and add per-segment lockfiles")
> Cc: anatoly.burakov@intel.com
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Tested-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-dev] [PATCH] eal: check if hugedir write lock is already being held

[Thomas Monjalon]

30/04/2018 15:07, Andrew Rybchenko:
> On 04/30/2018 01:38 PM, Anatoly Burakov wrote:
> > At hugepage info initialization, EAL takes out a write lock on
> > hugetlbfs directories, and drops it after the memory init is
> > finished. However, in non-legacy mode, if "-m" or "--socket-mem"
> > switches are passed, this leads to a deadlock because EAL tries
> > to allocate pages (and thus take out a write lock on hugedir)
> > while still holding a separate hugedir write lock in EAL.
> >
> > Fix it by checking if write lock in hugepage info is active, and
> > not trying to lock the directory if the hugedir fd is valid.
> >
> > Fixes: 1a7dc2252f28 ("mem: revert to using flock and add per-segment lockfiles")
> > Cc: anatoly.burakov@intel.com
> >
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Tested-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Tested-by: Shahaf Shuler <shahafs@mellanox.com>
> Tested-by: Andrew Rybchenko <arybchenko@solarflare.com>

Applied, thanks