* [PATCH 02/25] IB/mthca, net/mlx4: remove counting semaphores [not found] <1446000373-1823620-1-git-send-email-arnd@arndb.de> @ 2015-10-28 2:45 ` Arnd Bergmann [not found] ` <1446000373-1823620-3-git-send-email-arnd-r2nGTMty4D4@public.gmane.org> 2015-10-29 7:41 ` Jack Morgenstein 0 siblings, 2 replies; 3+ messages in thread From: Arnd Bergmann @ 2015-10-28 2:45 UTC (permalink / raw) To: tglx Cc: Arnd Bergmann, Roland Dreier, Eli Cohen, Yevgeny Petrilin, netdev, linux-rdma The mthca and mlx4 device drivers use the same method to switch between polling and event-driven command mode, abusing two semaphores to create a mutual exclusion between one polled command or multiple concurrent event driven commands. Since we want to make counting semaphores go away, this patch replaces the semaphore counting the event-driven commands with an open-coded wait-queue, which should be an equivalent transformation of the code, although it does not make it any nicer. As far as I can tell, there is a preexisting race condition regarding the cmd->use_events flag, which is not protected by any lock. When this flag is toggled while another command is being started, that command gets stuck until the mode is toggled back. A better solution that would solve the race condition and at the same time improve the code readability would create a new locking primitive that replaces both semaphores, like static int mlx4_use_events(struct mlx4_cmd *cmd) { int ret = -EAGAIN; spin_lock(&cmd->lock); if (cmd->use_events && cmd->commands < cmd->max_commands) { cmd->commands++; ret = 1; } else if (!cmd->use_events && cmd->commands == 0) { cmd->commands = 1; ret = 0; } spin_unlock(&cmd->lock); return ret; } static bool mlx4_use_events(struct mlx4_cmd *cmd) { int ret; wait_event(cmd->events_wq, ret = __mlx4_use_events(cmd) >= 0); return ret; } Cc: Roland Dreier <roland@purestorage.com> Cc: Eli Cohen <eli@mellanox.co.il> Cc: Yevgeny Petrilin <yevgenyp@mellanox.co.il> Cc: netdev@vger.kernel.org Cc: linux-rdma@vger.kernel.org Signed-off-by: Arnd Bergmann <arnd@arndb.de> Conflicts: drivers/net/mlx4/cmd.c drivers/net/mlx4/mlx4.h --- drivers/infiniband/hw/mthca/mthca_cmd.c | 12 ++++++++---- drivers/infiniband/hw/mthca/mthca_dev.h | 3 ++- drivers/net/ethernet/mellanox/mlx4/cmd.c | 12 ++++++++---- drivers/net/ethernet/mellanox/mlx4/mlx4.h | 3 ++- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/drivers/infiniband/hw/mthca/mthca_cmd.c b/drivers/infiniband/hw/mthca/mthca_cmd.c index 9d3e5c1ac60e..aad1852e8e10 100644 --- a/drivers/infiniband/hw/mthca/mthca_cmd.c +++ b/drivers/infiniband/hw/mthca/mthca_cmd.c @@ -417,7 +417,8 @@ static int mthca_cmd_wait(struct mthca_dev *dev, int err = 0; struct mthca_cmd_context *context; - down(&dev->cmd.event_sem); + wait_event(dev->cmd.event_wait, + atomic_add_unless(&dev->cmd.commands, -1, 0)); spin_lock(&dev->cmd.context_lock); BUG_ON(dev->cmd.free_head < 0); @@ -459,7 +460,8 @@ out: dev->cmd.free_head = context - dev->cmd.context; spin_unlock(&dev->cmd.context_lock); - up(&dev->cmd.event_sem); + atomic_inc(&dev->cmd.commands); + wake_up(&dev->cmd.event_wait); return err; } @@ -571,7 +573,8 @@ int mthca_cmd_use_events(struct mthca_dev *dev) dev->cmd.context[dev->cmd.max_cmds - 1].next = -1; dev->cmd.free_head = 0; - sema_init(&dev->cmd.event_sem, dev->cmd.max_cmds); + init_waitqueue_head(&dev->cmd.event_wait); + atomic_set(&dev->cmd.commands, dev->cmd.max_cmds); spin_lock_init(&dev->cmd.context_lock); for (dev->cmd.token_mask = 1; @@ -597,7 +600,8 @@ void mthca_cmd_use_polling(struct mthca_dev *dev) dev->cmd.flags &= ~MTHCA_CMD_USE_EVENTS; for (i = 0; i < dev->cmd.max_cmds; ++i) - down(&dev->cmd.event_sem); + wait_event(dev->cmd.event_wait, + atomic_add_unless(&dev->cmd.commands, -1, 0)); kfree(dev->cmd.context); diff --git a/drivers/infiniband/hw/mthca/mthca_dev.h b/drivers/infiniband/hw/mthca/mthca_dev.h index 7e6a6d64ad4e..3055f5c12ac8 100644 --- a/drivers/infiniband/hw/mthca/mthca_dev.h +++ b/drivers/infiniband/hw/mthca/mthca_dev.h @@ -121,7 +121,8 @@ struct mthca_cmd { struct pci_pool *pool; struct mutex hcr_mutex; struct semaphore poll_sem; - struct semaphore event_sem; + wait_queue_head_t event_wait; + atomic_t commands; int max_cmds; spinlock_t context_lock; int free_head; diff --git a/drivers/net/ethernet/mellanox/mlx4/cmd.c b/drivers/net/ethernet/mellanox/mlx4/cmd.c index 78f5a1a0b8c8..60134a4245ef 100644 --- a/drivers/net/ethernet/mellanox/mlx4/cmd.c +++ b/drivers/net/ethernet/mellanox/mlx4/cmd.c @@ -273,7 +273,8 @@ static int mlx4_cmd_wait(struct mlx4_dev *dev, u64 in_param, u64 *out_param, struct mlx4_cmd_context *context; int err = 0; - down(&cmd->event_sem); + wait_event(cmd->event_wait, + atomic_add_unless(&cmd->commands, -1, 0)); spin_lock(&cmd->context_lock); BUG_ON(cmd->free_head < 0); @@ -305,7 +306,8 @@ out: cmd->free_head = context - cmd->context; spin_unlock(&cmd->context_lock); - up(&cmd->event_sem); + atomic_inc(&cmd->commands); + wake_up(&cmd->event_wait); return err; } @@ -380,7 +382,8 @@ int mlx4_cmd_use_events(struct mlx4_dev *dev) priv->cmd.context[priv->cmd.max_cmds - 1].next = -1; priv->cmd.free_head = 0; - sema_init(&priv->cmd.event_sem, priv->cmd.max_cmds); + init_waitqueue_head(&priv->cmd.event_wait); + atomic_set(&priv->cmd.commands, priv->cmd.max_cmds); spin_lock_init(&priv->cmd.context_lock); for (priv->cmd.token_mask = 1; @@ -407,7 +410,8 @@ void mlx4_cmd_use_polling(struct mlx4_dev *dev) priv->cmd.use_events = 0; for (i = 0; i < priv->cmd.max_cmds; ++i) - down(&priv->cmd.event_sem); + wait_event(priv->cmd.event_wait, + atomic_add_unless(&priv->cmd.commands, -1, 0)); kfree(priv->cmd.context); diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4.h b/drivers/net/ethernet/mellanox/mlx4/mlx4.h index 5dfa68ffc11c..5a2c55d8ccfd 100644 --- a/drivers/net/ethernet/mellanox/mlx4/mlx4.h +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4.h @@ -189,7 +189,8 @@ struct mlx4_cmd { void __iomem *hcr; struct mutex hcr_mutex; struct semaphore poll_sem; - struct semaphore event_sem; + wait_queue_head_t event_wait; + atomic_t commands; int max_cmds; spinlock_t context_lock; int free_head; -- 2.1.0.rc2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
[parent not found: <1446000373-1823620-3-git-send-email-arnd-r2nGTMty4D4@public.gmane.org>]
* Re: [PATCH 02/25] IB/mthca, net/mlx4: remove counting semaphores [not found] ` <1446000373-1823620-3-git-send-email-arnd-r2nGTMty4D4@public.gmane.org> @ 2015-10-28 9:19 ` Sagi Grimberg 0 siblings, 0 replies; 3+ messages in thread From: Sagi Grimberg @ 2015-10-28 9:19 UTC (permalink / raw) To: Arnd Bergmann, tglx-hfZtesqFncYOwBW4kG4KsQ Cc: Roland Dreier, Eli Cohen, Yevgeny Petrilin, netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA, Nicholas A. Bellinger Hi Arnd, > Since we want to make counting semaphores go away, Why do we want to make counting semaphores go away? completely? or just for binary use cases? I have a use case in iser target code where a counting semaphore is the best suited synchronizing mechanism. I have a single thread handling connect requests (one at a time) while connect requests are event driven and come asynchronously. This is why I use a queue and a counting semaphore to handle this situation. I'd need to rethink of a new strategy to handle this without counting semaphores and I'm not entirely sure it would be simpler. > this patch replaces the semaphore counting the event-driven > commands with an open-coded wait-queue, which should > be an equivalent transformation of the code, although > it does not make it any nicer. > > As far as I can tell, there is a preexisting race condition > regarding the cmd->use_events flag, which is not protected > by any lock. When this flag is toggled while another command > is being started, that command gets stuck until the mode is > toggled back. > > A better solution that would solve the race condition and > at the same time improve the code readability would create > a new locking primitive that replaces both semaphores, like > > static int mlx4_use_events(struct mlx4_cmd *cmd) > { > int ret = -EAGAIN; > spin_lock(&cmd->lock); > if (cmd->use_events && cmd->commands < cmd->max_commands) { > cmd->commands++; > ret = 1; > } else if (!cmd->use_events && cmd->commands == 0) { > cmd->commands = 1; > ret = 0; > } > spin_unlock(&cmd->lock); > return ret; > } > > static bool mlx4_use_events(struct mlx4_cmd *cmd) > { > int ret; > wait_event(cmd->events_wq, ret = __mlx4_use_events(cmd) >= 0); > return ret; > } > > Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org> > Cc: Eli Cohen <eli-VPRAkNaXOzVS1MOuV/RT9w@public.gmane.org> > Cc: Yevgeny Petrilin <yevgenyp-VPRAkNaXOzVS1MOuV/RT9w@public.gmane.org> > Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> > > Conflicts: > > drivers/net/mlx4/cmd.c > drivers/net/mlx4/mlx4.h > --- > drivers/infiniband/hw/mthca/mthca_cmd.c | 12 ++++++++---- > drivers/infiniband/hw/mthca/mthca_dev.h | 3 ++- > drivers/net/ethernet/mellanox/mlx4/cmd.c | 12 ++++++++---- > drivers/net/ethernet/mellanox/mlx4/mlx4.h | 3 ++- > 4 files changed, 20 insertions(+), 10 deletions(-) > > diff --git a/drivers/infiniband/hw/mthca/mthca_cmd.c b/drivers/infiniband/hw/mthca/mthca_cmd.c > index 9d3e5c1ac60e..aad1852e8e10 100644 > --- a/drivers/infiniband/hw/mthca/mthca_cmd.c > +++ b/drivers/infiniband/hw/mthca/mthca_cmd.c > @@ -417,7 +417,8 @@ static int mthca_cmd_wait(struct mthca_dev *dev, > int err = 0; > struct mthca_cmd_context *context; > > - down(&dev->cmd.event_sem); > + wait_event(dev->cmd.event_wait, > + atomic_add_unless(&dev->cmd.commands, -1, 0)); > > spin_lock(&dev->cmd.context_lock); > BUG_ON(dev->cmd.free_head < 0); > @@ -459,7 +460,8 @@ out: > dev->cmd.free_head = context - dev->cmd.context; > spin_unlock(&dev->cmd.context_lock); > > - up(&dev->cmd.event_sem); > + atomic_inc(&dev->cmd.commands); > + wake_up(&dev->cmd.event_wait); > return err; > } > > @@ -571,7 +573,8 @@ int mthca_cmd_use_events(struct mthca_dev *dev) > dev->cmd.context[dev->cmd.max_cmds - 1].next = -1; > dev->cmd.free_head = 0; > > - sema_init(&dev->cmd.event_sem, dev->cmd.max_cmds); > + init_waitqueue_head(&dev->cmd.event_wait); > + atomic_set(&dev->cmd.commands, dev->cmd.max_cmds); > spin_lock_init(&dev->cmd.context_lock); > > for (dev->cmd.token_mask = 1; > @@ -597,7 +600,8 @@ void mthca_cmd_use_polling(struct mthca_dev *dev) > dev->cmd.flags &= ~MTHCA_CMD_USE_EVENTS; > > for (i = 0; i < dev->cmd.max_cmds; ++i) > - down(&dev->cmd.event_sem); > + wait_event(dev->cmd.event_wait, > + atomic_add_unless(&dev->cmd.commands, -1, 0)); > > kfree(dev->cmd.context); > > diff --git a/drivers/infiniband/hw/mthca/mthca_dev.h b/drivers/infiniband/hw/mthca/mthca_dev.h > index 7e6a6d64ad4e..3055f5c12ac8 100644 > --- a/drivers/infiniband/hw/mthca/mthca_dev.h > +++ b/drivers/infiniband/hw/mthca/mthca_dev.h > @@ -121,7 +121,8 @@ struct mthca_cmd { > struct pci_pool *pool; > struct mutex hcr_mutex; > struct semaphore poll_sem; > - struct semaphore event_sem; > + wait_queue_head_t event_wait; > + atomic_t commands; > int max_cmds; > spinlock_t context_lock; > int free_head; > diff --git a/drivers/net/ethernet/mellanox/mlx4/cmd.c b/drivers/net/ethernet/mellanox/mlx4/cmd.c > index 78f5a1a0b8c8..60134a4245ef 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/cmd.c > +++ b/drivers/net/ethernet/mellanox/mlx4/cmd.c > @@ -273,7 +273,8 @@ static int mlx4_cmd_wait(struct mlx4_dev *dev, u64 in_param, u64 *out_param, > struct mlx4_cmd_context *context; > int err = 0; > > - down(&cmd->event_sem); > + wait_event(cmd->event_wait, > + atomic_add_unless(&cmd->commands, -1, 0)); > > spin_lock(&cmd->context_lock); > BUG_ON(cmd->free_head < 0); > @@ -305,7 +306,8 @@ out: > cmd->free_head = context - cmd->context; > spin_unlock(&cmd->context_lock); > > - up(&cmd->event_sem); > + atomic_inc(&cmd->commands); > + wake_up(&cmd->event_wait); > return err; > } > > @@ -380,7 +382,8 @@ int mlx4_cmd_use_events(struct mlx4_dev *dev) > priv->cmd.context[priv->cmd.max_cmds - 1].next = -1; > priv->cmd.free_head = 0; > > - sema_init(&priv->cmd.event_sem, priv->cmd.max_cmds); > + init_waitqueue_head(&priv->cmd.event_wait); > + atomic_set(&priv->cmd.commands, priv->cmd.max_cmds); > spin_lock_init(&priv->cmd.context_lock); > > for (priv->cmd.token_mask = 1; > @@ -407,7 +410,8 @@ void mlx4_cmd_use_polling(struct mlx4_dev *dev) > priv->cmd.use_events = 0; > > for (i = 0; i < priv->cmd.max_cmds; ++i) > - down(&priv->cmd.event_sem); > + wait_event(priv->cmd.event_wait, > + atomic_add_unless(&priv->cmd.commands, -1, 0)); > > kfree(priv->cmd.context); > > diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4.h b/drivers/net/ethernet/mellanox/mlx4/mlx4.h > index 5dfa68ffc11c..5a2c55d8ccfd 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/mlx4.h > +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4.h > @@ -189,7 +189,8 @@ struct mlx4_cmd { > void __iomem *hcr; > struct mutex hcr_mutex; > struct semaphore poll_sem; > - struct semaphore event_sem; > + wait_queue_head_t event_wait; > + atomic_t commands; > int max_cmds; > spinlock_t context_lock; > int free_head; > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 02/25] IB/mthca, net/mlx4: remove counting semaphores 2015-10-28 2:45 ` [PATCH 02/25] IB/mthca, net/mlx4: remove counting semaphores Arnd Bergmann [not found] ` <1446000373-1823620-3-git-send-email-arnd-r2nGTMty4D4@public.gmane.org> @ 2015-10-29 7:41 ` Jack Morgenstein 1 sibling, 0 replies; 3+ messages in thread From: Jack Morgenstein @ 2015-10-29 7:41 UTC (permalink / raw) To: Arnd Bergmann Cc: tglx, Roland Dreier, Eli Cohen, Yevgeny Petrilin, netdev, linux-rdma, matanb, yishaih, ogerlitz, eranbe, talal On Wed, 28 Oct 2015 03:45:50 +0100 Arnd Bergmann <arnd@arndb.de> wrote: > As far as I can tell, there is a preexisting race condition > regarding the cmd->use_events flag, which is not protected > by any lock. When this flag is toggled while another command > is being started, that command gets stuck until the mode is > toggled back. We fixed this issue in mellanox ofed in a manner that allowed keeping the same counting mechanism. IMHO, this is preferable, rather than totally changing the mechanism. We will submit a similar patch to the upstream kernel shortly. -Jack net/mlx4: Switching between sending commands via polling and events may results in hung tasks When switching between those methonds of sending commands, it's possbile that a task will keep waiting for the polling sempahore, but may never be able to acquire it. This is due to mlx4_cmd_use_events which "down"s the sempahore back to 0. Reproducing it involves in sending commands while chaning between mlx4_cmd_use_polling and mlx4_cmd_use_events. Solving it by adding a read-write semaphore when switching between modes. issue: 402565 Change-Id: I19f0d40dbb327c49b39a9abbcb2bb002b0279b0b Signed-off-by: Matan Barak <matanb@mellanox.com> --- drivers/net/ethernet/mellanox/mlx4/cmd.c | 23 +++++++++++++++++------ drivers/net/ethernet/mellanox/mlx4/mlx4.h | 2 ++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx4/cmd.c b/drivers/net/ethernet/mellanox/mlx4/cmd.c index def1338..f94a960 100644 --- a/drivers/net/ethernet/mellanox/mlx4/cmd.c +++ b/drivers/net/ethernet/mellanox/mlx4/cmd.c @@ -766,17 +766,23 @@ int __mlx4_cmd(struct mlx4_dev *dev, u64 in_param, u64 *out_param, return mlx4_cmd_reset_flow(dev, op, op_modifier, -EIO); if (!mlx4_is_mfunc(dev) || (native && mlx4_is_master(dev))) { + int ret; + if (dev->persist->state & MLX4_DEVICE_STATE_INTERNAL_ERROR) return mlx4_internal_err_ret_value(dev, op, op_modifier); + down_read(&mlx4_priv(dev)->cmd.switch_sem); if (mlx4_priv(dev)->cmd.use_events) - return mlx4_cmd_wait(dev, in_param, out_param, - out_is_imm, in_modifier, - op_modifier, op, timeout); + ret = mlx4_cmd_wait(dev, in_param, out_param, + out_is_imm, in_modifier, + op_modifier, op, timeout); else - return mlx4_cmd_poll(dev, in_param, out_param, - out_is_imm, in_modifier, - op_modifier, op, timeout); + ret = mlx4_cmd_poll(dev, in_param, out_param, + out_is_imm, in_modifier, + op_modifier, op, timeout); + + up_read(&mlx4_priv(dev)->cmd.switch_sem); + return ret; } return mlx4_slave_cmd(dev, in_param, out_param, out_is_imm, in_modifier, op_modifier, op, timeout); @@ -2437,6 +2443,7 @@ int mlx4_cmd_init(struct mlx4_dev *dev) int flags = 0; if (!priv->cmd.initialized) { + init_rwsem(&priv->cmd.switch_sem); mutex_init(&priv->cmd.slave_cmd_mutex); sema_init(&priv->cmd.poll_sem, 1); priv->cmd.use_events = 0; @@ -2566,6 +2573,7 @@ int mlx4_cmd_use_events(struct mlx4_dev *dev) if (!priv->cmd.context) return -ENOMEM; + down_write(&priv->cmd.switch_sem); for (i = 0; i < priv->cmd.max_cmds; ++i) { priv->cmd.context[i].token = i; priv->cmd.context[i].next = i + 1; @@ -2590,6 +2598,7 @@ int mlx4_cmd_use_events(struct mlx4_dev *dev) down(&priv->cmd.poll_sem); priv->cmd.use_events = 1; + up_write(&priv->cmd.switch_sem); return err; } @@ -2602,6 +2611,7 @@ void mlx4_cmd_use_polling(struct mlx4_dev *dev) struct mlx4_priv *priv = mlx4_priv(dev); int i; + down_write(&priv->cmd.switch_sem); priv->cmd.use_events = 0; for (i = 0; i < priv->cmd.max_cmds; ++i) @@ -2610,6 +2620,7 @@ void mlx4_cmd_use_polling(struct mlx4_dev *dev) kfree(priv->cmd.context); up(&priv->cmd.poll_sem); + up_write(&priv->cmd.switch_sem); } struct mlx4_cmd_mailbox *mlx4_alloc_cmd_mailbox(struct mlx4_dev *dev) diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4.h b/drivers/net/ethernet/mellanox/mlx4/mlx4.h index 6c58021..2f03e6e 100644 --- a/drivers/net/ethernet/mellanox/mlx4/mlx4.h +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4.h @@ -45,6 +45,7 @@ #include <linux/workqueue.h> #include <linux/interrupt.h> #include <linux/spinlock.h> +#include <linux/rwsem.h> #include <linux/mlx4/device.h> #include <linux/mlx4/driver.h> @@ -626,6 +627,7 @@ struct mlx4_cmd { struct mutex slave_cmd_mutex; struct semaphore poll_sem; struct semaphore event_sem; + struct rw_semaphore switch_sem; int max_cmds; spinlock_t context_lock; int free_head; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-10-29 7:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1446000373-1823620-1-git-send-email-arnd@arndb.de>
2015-10-28 2:45 ` [PATCH 02/25] IB/mthca, net/mlx4: remove counting semaphores Arnd Bergmann
[not found] ` <1446000373-1823620-3-git-send-email-arnd-r2nGTMty4D4@public.gmane.org>
2015-10-28 9:19 ` Sagi Grimberg
2015-10-29 7:41 ` Jack Morgenstein
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox