diff mbox series

[v2] fbarray: fix attach deadlock

Message ID 20190329095239.9646-1-dariusz.stojaczyk@intel.com (mailing list archive)
State Accepted, archived
Headers
Series [v2] fbarray: fix attach deadlock |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Stojaczyk, Dariusz March 29, 2019, 9:52 a.m. UTC 
  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(-)

Comments

Anatoly Burakov March 29, 2019, 10:42 a.m. UTC | #1 
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).
Thomas Monjalon March 29, 2019, 11:15 a.m. UTC | #2 
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.
diff mbox series

Patch

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;
 }