[dpdk-dev] [PATCH] fbarray: fix attach deadlock

[dpdk-dev] [PATCH] fbarray: fix attach deadlock

[Darek Stojaczyk]

rte_fbarray_attach() currently locks its internal
spinlock, but never releases it. Secondary processes
won't even start if there is more than one fbarray
to be attached to - the second rte_fbarray_attach()
would be just stuck.

Fix it by releasing the lock at the end of
rte_fbarray_attach(). I believe this was the original
intention.

Fixes: 5b61c62cfd76 ("fbarray: add internal tailq for mapped areas")
Cc: anatoly.burakov@intel.com
Cc: thomas@monjalon.net

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
 lib/librte_eal/common/eal_common_fbarray.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
index 0e7366e5e..5ca8d6f0e 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -906,6 +906,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
 
 	/* we're done */
 
+	rte_spinlock_unlock(&mem_area_lock);
 	return 0;
 fail:
 	if (data)
@@ -913,6 +914,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
 	if (fd >= 0)
 		close(fd);
 	free(ma);
+	rte_spinlock_unlock(&mem_area_lock);
 	return -1;
 }
 
-- 
2.17.1

[dpdk-dev] [PATCH] fbarray: fix attach deadlock

[Darek Stojaczyk]

rte_fbarray_attach() currently locks its internal
spinlock, but never releases it. Secondary processes
won't even start if there is more than one fbarray
to be attached to - the second rte_fbarray_attach()
would be just stuck.

Fix it by releasing the lock at the end of
rte_fbarray_attach(). I believe this was the original
intention.

Fixes: 5b61c62cfd76 ("fbarray: add internal tailq for mapped areas")
Cc: anatoly.burakov@intel.com
Cc: thomas@monjalon.net

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
 lib/librte_eal/common/eal_common_fbarray.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
index 0e7366e5e..5ca8d6f0e 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -906,6 +906,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
 
 	/* we're done */
 
+	rte_spinlock_unlock(&mem_area_lock);
 	return 0;
 fail:
 	if (data)
@@ -913,6 +914,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
 	if (fd >= 0)
 		close(fd);
 	free(ma);
+	rte_spinlock_unlock(&mem_area_lock);
 	return -1;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH] fbarray: fix attach deadlock

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Darek Stojaczyk
> Sent: Friday, March 29, 2019 1:10 PM
> To: dev@dpdk.org
> Cc: james.r.harris@intel.com; changpeng.liu@intel.com; Darek Stojaczyk
> <dariusz.stojaczyk@intel.com>; anatoly.burakov@intel.com;
> thomas@monjalon.net
> Subject: [dpdk-dev] [PATCH] fbarray: fix attach deadlock
>
> rte_fbarray_attach() currently locks its internal
> spinlock, but never releases it. Secondary processes
> won't even start if there is more than one fbarray
> to be attached to - the second rte_fbarray_attach()
> would be just stuck.
>
> Fix it by releasing the lock at the end of
> rte_fbarray_attach(). I believe this was the original
> intention.
>
> Fixes: 5b61c62cfd76 ("fbarray: add internal tailq for mapped areas")
> Cc: anatoly.burakov@intel.com
> Cc: thomas@monjalon.net
>
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
>  lib/librte_eal/common/eal_common_fbarray.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/lib/librte_eal/common/eal_common_fbarray.c
> b/lib/librte_eal/common/eal_common_fbarray.c
> index 0e7366e5e..5ca8d6f0e 100644
> --- a/lib/librte_eal/common/eal_common_fbarray.c
> +++ b/lib/librte_eal/common/eal_common_fbarray.c
> @@ -906,6 +906,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
>
>  /* we're done */
>
> +rte_spinlock_unlock(&mem_area_lock);
>  return 0;
>  fail:
>  if (data)
> @@ -913,6 +914,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
>  if (fd >= 0)
>  close(fd);
>  free(ma);
> +rte_spinlock_unlock(&mem_area_lock);
>  return -1;
>  }

This is an obvious issue, good catch!
Reviewed-by: Gavin Hu <gavin.hu@arm.com>

> --
> 2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [dpdk-dev] [PATCH] fbarray: fix attach deadlock

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Darek Stojaczyk
> Sent: Friday, March 29, 2019 1:10 PM
> To: dev@dpdk.org
> Cc: james.r.harris@intel.com; changpeng.liu@intel.com; Darek Stojaczyk
> <dariusz.stojaczyk@intel.com>; anatoly.burakov@intel.com;
> thomas@monjalon.net
> Subject: [dpdk-dev] [PATCH] fbarray: fix attach deadlock
>
> rte_fbarray_attach() currently locks its internal
> spinlock, but never releases it. Secondary processes
> won't even start if there is more than one fbarray
> to be attached to - the second rte_fbarray_attach()
> would be just stuck.
>
> Fix it by releasing the lock at the end of
> rte_fbarray_attach(). I believe this was the original
> intention.
>
> Fixes: 5b61c62cfd76 ("fbarray: add internal tailq for mapped areas")
> Cc: anatoly.burakov@intel.com
> Cc: thomas@monjalon.net
>
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
>  lib/librte_eal/common/eal_common_fbarray.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/lib/librte_eal/common/eal_common_fbarray.c
> b/lib/librte_eal/common/eal_common_fbarray.c
> index 0e7366e5e..5ca8d6f0e 100644
> --- a/lib/librte_eal/common/eal_common_fbarray.c
> +++ b/lib/librte_eal/common/eal_common_fbarray.c
> @@ -906,6 +906,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
>
>  /* we're done */
>
> +rte_spinlock_unlock(&mem_area_lock);
>  return 0;
>  fail:
>  if (data)
> @@ -913,6 +914,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
>  if (fd >= 0)
>  close(fd);
>  free(ma);
> +rte_spinlock_unlock(&mem_area_lock);
>  return -1;
>  }

This is an obvious issue, good catch!
Reviewed-by: Gavin Hu <gavin.hu@arm.com>

> --
> 2.17.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[dpdk-dev] [PATCH v2] fbarray: fix attach deadlock

[Darek Stojaczyk]

rte_fbarray_attach() currently locks its internal
spinlock, but never releases it. Secondary processes
won't even start if there is more than one fbarray
to be attached to - the second rte_fbarray_attach()
would be just stuck.

Fix it by releasing the lock at the end of
rte_fbarray_attach(). I believe this was the original
intention.

Fixes: 5b61c62cfd76 ("fbarray: add internal tailq for mapped areas")
Cc: anatoly.burakov@intel.com
Cc: thomas@monjalon.net

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
v2:
 - fixed one more case where we could unlock the spinlock
   before locking it


 lib/librte_eal/common/eal_common_fbarray.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
index 0e7366e5e..31cce80a8 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -859,8 +859,10 @@ rte_fbarray_attach(struct rte_fbarray *arr)
 	}
 
 	page_sz = sysconf(_SC_PAGESIZE);
-	if (page_sz == (size_t)-1)
-		goto fail;
+	if (page_sz == (size_t)-1) {
+		free(ma);
+		return -1;
+	}
 
 	mmap_len = calc_data_size(page_sz, arr->elt_sz, arr->len);
 
@@ -906,6 +908,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
 
 	/* we're done */
 
+	rte_spinlock_unlock(&mem_area_lock);
 	return 0;
 fail:
 	if (data)
@@ -913,6 +916,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
 	if (fd >= 0)
 		close(fd);
 	free(ma);
+	rte_spinlock_unlock(&mem_area_lock);
 	return -1;
 }
 
-- 
2.17.1

[dpdk-dev] [PATCH v2] fbarray: fix attach deadlock

[Darek Stojaczyk]

rte_fbarray_attach() currently locks its internal
spinlock, but never releases it. Secondary processes
won't even start if there is more than one fbarray
to be attached to - the second rte_fbarray_attach()
would be just stuck.

Fix it by releasing the lock at the end of
rte_fbarray_attach(). I believe this was the original
intention.

Fixes: 5b61c62cfd76 ("fbarray: add internal tailq for mapped areas")
Cc: anatoly.burakov@intel.com
Cc: thomas@monjalon.net

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
v2:
 - fixed one more case where we could unlock the spinlock
   before locking it


 lib/librte_eal/common/eal_common_fbarray.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
index 0e7366e5e..31cce80a8 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -859,8 +859,10 @@ rte_fbarray_attach(struct rte_fbarray *arr)
 	}
 
 	page_sz = sysconf(_SC_PAGESIZE);
-	if (page_sz == (size_t)-1)
-		goto fail;
+	if (page_sz == (size_t)-1) {
+		free(ma);
+		return -1;
+	}
 
 	mmap_len = calc_data_size(page_sz, arr->elt_sz, arr->len);
 
@@ -906,6 +908,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
 
 	/* we're done */
 
+	rte_spinlock_unlock(&mem_area_lock);
 	return 0;
 fail:
 	if (data)
@@ -913,6 +916,7 @@ rte_fbarray_attach(struct rte_fbarray *arr)
 	if (fd >= 0)
 		close(fd);
 	free(ma);
+	rte_spinlock_unlock(&mem_area_lock);
 	return -1;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] fbarray: fix attach deadlock

[Burakov, Anatoly]

On 29-Mar-19 9:52 AM, Darek Stojaczyk wrote:
> rte_fbarray_attach() currently locks its internal
> spinlock, but never releases it. Secondary processes
> won't even start if there is more than one fbarray
> to be attached to - the second rte_fbarray_attach()
> would be just stuck.
> 
> Fix it by releasing the lock at the end of
> rte_fbarray_attach(). I believe this was the original
> intention.
> 
> Fixes: 5b61c62cfd76 ("fbarray: add internal tailq for mapped areas")
> Cc: anatoly.burakov@intel.com
> Cc: thomas@monjalon.net
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
> v2:
>   - fixed one more case where we could unlock the spinlock
>     before locking it

Thanks for catching this!

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

There is one more case where we do unlock on init without locking, i'll 
submit a patch separately (and will check other functions with a 
microscope just in case).

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] fbarray: fix attach deadlock

[Burakov, Anatoly]

On 29-Mar-19 9:52 AM, Darek Stojaczyk wrote:
> rte_fbarray_attach() currently locks its internal
> spinlock, but never releases it. Secondary processes
> won't even start if there is more than one fbarray
> to be attached to - the second rte_fbarray_attach()
> would be just stuck.
> 
> Fix it by releasing the lock at the end of
> rte_fbarray_attach(). I believe this was the original
> intention.
> 
> Fixes: 5b61c62cfd76 ("fbarray: add internal tailq for mapped areas")
> Cc: anatoly.burakov@intel.com
> Cc: thomas@monjalon.net
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> ---
> v2:
>   - fixed one more case where we could unlock the spinlock
>     before locking it

Thanks for catching this!

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

There is one more case where we do unlock on init without locking, i'll 
submit a patch separately (and will check other functions with a 
microscope just in case).

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] fbarray: fix attach deadlock

[Thomas Monjalon]

29/03/2019 11:42, Burakov, Anatoly:
> On 29-Mar-19 9:52 AM, Darek Stojaczyk wrote:
> > rte_fbarray_attach() currently locks its internal
> > spinlock, but never releases it. Secondary processes
> > won't even start if there is more than one fbarray
> > to be attached to - the second rte_fbarray_attach()
> > would be just stuck.
> > 
> > Fix it by releasing the lock at the end of
> > rte_fbarray_attach(). I believe this was the original
> > intention.
> > 
> > Fixes: 5b61c62cfd76 ("fbarray: add internal tailq for mapped areas")
> > Cc: anatoly.burakov@intel.com
> > Cc: thomas@monjalon.net
> > 
> > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > ---
> > v2:
> >   - fixed one more case where we could unlock the spinlock
> >     before locking it
> 
> Thanks for catching this!
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks


> There is one more case where we do unlock on init without locking, i'll 
> submit a patch separately (and will check other functions with a 
> microscope just in case).

We'll take this one too.

Re: [dpdk-dev] [PATCH v2] fbarray: fix attach deadlock

[Thomas Monjalon]

29/03/2019 11:42, Burakov, Anatoly:
> On 29-Mar-19 9:52 AM, Darek Stojaczyk wrote:
> > rte_fbarray_attach() currently locks its internal
> > spinlock, but never releases it. Secondary processes
> > won't even start if there is more than one fbarray
> > to be attached to - the second rte_fbarray_attach()
> > would be just stuck.
> > 
> > Fix it by releasing the lock at the end of
> > rte_fbarray_attach(). I believe this was the original
> > intention.
> > 
> > Fixes: 5b61c62cfd76 ("fbarray: add internal tailq for mapped areas")
> > Cc: anatoly.burakov@intel.com
> > Cc: thomas@monjalon.net
> > 
> > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > ---
> > v2:
> >   - fixed one more case where we could unlock the spinlock
> >     before locking it
> 
> Thanks for catching this!
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks


> There is one more case where we do unlock on init without locking, i'll 
> submit a patch separately (and will check other functions with a 
> microscope just in case).

We'll take this one too.