linux-kernel - Re: [PATCH v3 8/9] IB/mlx5: Add helper mlx5_ib_post_send

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <12933685.Se3eQHi9An@wuerfel>
Date:   Wed, 26 Oct 2016 10:48:26 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Binoy Jayan <binoy.jayan@...aro.org>
Cc:     Doug Ledford <dledford@...hat.com>,
        Sean Hefty <sean.hefty@...el.com>,
        Hal Rosenstock <hal.rosenstock@...il.com>,
        linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 8/9] IB/mlx5: Add helper mlx5_ib_post_send_wait

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