Re: [PATCH 02/25] IB/mthca, net/mlx4: remove counting semaphores

[Sagi Grimberg <sagig-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

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