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
