From: Arnd Bergmann <arnd@arndb.de>
To: Binoy Jayan <binoy.jayan@linaro.org>
Cc: Doug Ledford <dledford@redhat.com>, Sean Hefty <sean.hefty@intel.com>,
        Hal Rosenstock <hal.rosenstock@gmail.com>, linux-rdma@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 8/9] IB/mlx5: Add helper mlx5_ib_post_send_wait
Date: Wed, 26 Oct 2016 10:48:26 +0200
Message-ID: <12933685.Se3eQHi9An@wuerfel>
User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; )
In-Reply-To: <1477468874-16328-9-git-send-email-binoy.jayan@linaro.org>
References: <1477468874-16328-1-git-send-email-binoy.jayan@linaro.org> <1477468874-16328-9-git-send-email-binoy.jayan@linaro.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1899
Lines: 53

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