Re: [PATCH v3 8/9] IB/mlx5: Add helper mlx5_ib_post_send_wait

[Arnd Bergmann]

On Wednesday, October 26, 2016 1:31:13 PM CEST Binoy Jayan wrote:
> +static inline int mlx5_ib_post_send_wait(struct mlx5_ib_dev *dev,
> +                                        struct mlx5_umr_wr *umrwr)
> +{
> +       struct umr_common *umrc = &dev->umrc;
> +       struct ib_send_wr *bad;
> +       int err;
> +       struct mlx5_ib_umr_context umr_context;
> +
> +       mlx5_ib_init_umr_context(&umr_context);
> +       umrwr->wr.wr_cqe = &umr_context.cqe;
> +
> +       down(&umrc->sem);
> +       err = ib_post_send(umrc->qp, &umrwr->wr, &bad);
> +       if (err) {
> +               mlx5_ib_warn(dev, "UMR post send failed, err %d\n", err);
> +       } else {
> +               wait_for_completion(&umr_context.done);
> +               if (umr_context.status != IB_WC_SUCCESS) {
> +                       mlx5_ib_warn(dev, "reg umr failed (%u)\n",
> +                                    umr_context.status);
> +                       err = -EFAULT;
> +               }
> +       }
> +       up(&umrc->sem);
> +       return err;
> +}
> 

Looks nice!

Now that this has become the only use of the semaphore (and the
last one in infiniband I guess), I wonder if we can agree on a way
to get rid of that one too. How about

	/* limit number of concurrent ib_post_send() on qp */
	wait_event(&umrc->wq, atomic_add_unless(&umrc->users, 1, MAX_UMR_WR);
	...
	atomic_dec(&umrc->users);
	wake_up(&umrc->wq);

That would be a fairly simple conversion and document better
what we actually do here: the down() looks like a simple mutex,
which it isn't.

In terms of efficiency, the wait_event() is actually better
here for the common case that the semaphore is not contented
as it only needs a single atomic operation and a branch
instead of an external function call and a spinlock in down().
However, the wake_up() is slightly better than atomic_dec()+up()
as it avoids the additional atomic.

	Arnd