ipc: fix locking while sending messages

Message ID 164824cc66c16755416bb8d3d6911385f52f8c1e.1530092380.git.anatoly.burakov@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series ipc: fix locking while sending messages |

Checks

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

Commit Message

Burakov, Anatoly June 27, 2018, 9:44 a.m. UTC
  Previously, we were putting an exclusive lock to prevent secondary
processes spinning up while we are sending our messages. However,
using exclusive locks had an effect of disallowing multiple
simultaenous unrelated messages/requests being sent, which was
not the intention behind locking.

Fix it to put a shared lock on the directory. That way, we still
prevent secondary process initializations while sending data over
IPC, but allow multiple unrelated transmissions to proceed.

Fixes: 89f1fe7e6d95 ("eal: lock IPC directory on init and send")
Cc: stable@dpdk.org

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

Notes:
    This patch is needed for multiprocess hotplug support [1], to avoid cases
    where multiple requests deadlock while in progress.
    
    [1] http://patches.dpdk.org/project/dpdk/list/?series=252

 lib/librte_eal/common/eal_common_proc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

Comments

Qi Zhang June 27, 2018, 12:25 p.m. UTC | #1
> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Wednesday, June 27, 2018 5:44 PM
> To: dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; stable@dpdk.org
> Subject: [PATCH] ipc: fix locking while sending messages
> 
> Previously, we were putting an exclusive lock to prevent secondary processes
> spinning up while we are sending our messages. However, using exclusive
> locks had an effect of disallowing multiple simultaenous unrelated
> messages/requests being sent, which was not the intention behind locking.
> 
> Fix it to put a shared lock on the directory. That way, we still prevent
> secondary process initializations while sending data over IPC, but allow
> multiple unrelated transmissions to proceed.
> 
> Fixes: 89f1fe7e6d95 ("eal: lock IPC directory on init and send")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>

Tested-by: Qi Zhang <qi.z.zhang@intel.com>

Thanks for the quick fix!
Qi

> ---
> 
> Notes:
>     This patch is needed for multiprocess hotplug support [1], to avoid cases
>     where multiple requests deadlock while in progress.
> 
>     [1] http://patches.dpdk.org/project/dpdk/list/?series=252
> 
>  lib/librte_eal/common/eal_common_proc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_proc.c
> b/lib/librte_eal/common/eal_common_proc.c
> index 707d8ab30..f010ef59e 100644
> --- a/lib/librte_eal/common/eal_common_proc.c
> +++ b/lib/librte_eal/common/eal_common_proc.c
> @@ -786,7 +786,7 @@ mp_send(struct rte_mp_msg *msg, const char *peer,
> int type)
> 
>  	dir_fd = dirfd(mp_dir);
>  	/* lock the directory to prevent processes spinning up while we send */
> -	if (flock(dir_fd, LOCK_EX)) {
> +	if (flock(dir_fd, LOCK_SH)) {
>  		RTE_LOG(ERR, EAL, "Unable to lock directory %s\n",
>  			mp_dir_path);
>  		rte_errno = errno;
> @@ -1020,7 +1020,7 @@ rte_mp_request_sync(struct rte_mp_msg *req,
> struct rte_mp_reply *reply,
> 
>  	dir_fd = dirfd(mp_dir);
>  	/* lock the directory to prevent processes spinning up while we send */
> -	if (flock(dir_fd, LOCK_EX)) {
> +	if (flock(dir_fd, LOCK_SH)) {
>  		RTE_LOG(ERR, EAL, "Unable to lock directory %s\n",
>  			mp_dir_path);
>  		closedir(mp_dir);
> @@ -1146,7 +1146,7 @@ rte_mp_request_async(struct rte_mp_msg *req,
> const struct timespec *ts,
>  	dir_fd = dirfd(mp_dir);
> 
>  	/* lock the directory to prevent processes spinning up while we send */
> -	if (flock(dir_fd, LOCK_EX)) {
> +	if (flock(dir_fd, LOCK_SH)) {
>  		RTE_LOG(ERR, EAL, "Unable to lock directory %s\n",
>  			mp_dir_path);
>  		rte_errno = errno;
> --
> 2.17.1
  
Thomas Monjalon June 29, 2018, 12:02 a.m. UTC | #2
27/06/2018 14:25, Zhang, Qi Z:
> From: Burakov, Anatoly
> > 
> > Previously, we were putting an exclusive lock to prevent secondary processes
> > spinning up while we are sending our messages. However, using exclusive
> > locks had an effect of disallowing multiple simultaenous unrelated
> > messages/requests being sent, which was not the intention behind locking.
> > 
> > Fix it to put a shared lock on the directory. That way, we still prevent
> > secondary process initializations while sending data over IPC, but allow
> > multiple unrelated transmissions to proceed.
> > 
> > Fixes: 89f1fe7e6d95 ("eal: lock IPC directory on init and send")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Tested-by: Qi Zhang <qi.z.zhang@intel.com>
> 
> Thanks for the quick fix!
> Qi

Applied, thanks
  

Patch

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index 707d8ab30..f010ef59e 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -786,7 +786,7 @@  mp_send(struct rte_mp_msg *msg, const char *peer, int type)
 
 	dir_fd = dirfd(mp_dir);
 	/* lock the directory to prevent processes spinning up while we send */
-	if (flock(dir_fd, LOCK_EX)) {
+	if (flock(dir_fd, LOCK_SH)) {
 		RTE_LOG(ERR, EAL, "Unable to lock directory %s\n",
 			mp_dir_path);
 		rte_errno = errno;
@@ -1020,7 +1020,7 @@  rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 
 	dir_fd = dirfd(mp_dir);
 	/* lock the directory to prevent processes spinning up while we send */
-	if (flock(dir_fd, LOCK_EX)) {
+	if (flock(dir_fd, LOCK_SH)) {
 		RTE_LOG(ERR, EAL, "Unable to lock directory %s\n",
 			mp_dir_path);
 		closedir(mp_dir);
@@ -1146,7 +1146,7 @@  rte_mp_request_async(struct rte_mp_msg *req, const struct timespec *ts,
 	dir_fd = dirfd(mp_dir);
 
 	/* lock the directory to prevent processes spinning up while we send */
-	if (flock(dir_fd, LOCK_EX)) {
+	if (flock(dir_fd, LOCK_SH)) {
 		RTE_LOG(ERR, EAL, "Unable to lock directory %s\n",
 			mp_dir_path);
 		rte_errno = errno;