[6/8] IB/hns: Replace counting semaphore event_sem with wait condition

Counting semaphores are going away in the future, so replace the semaphore
hns_roce_cmdq::event_sem with an open-coded implementation.

Signed-off-by: Binoy Jayan <binoy.jayan@linaro.org>

---
 drivers/infiniband/hw/hns/hns_roce_cmd.c    | 16 ++++++++++++----
 drivers/infiniband/hw/hns/hns_roce_device.h |  3 ++-
 include/rdma/ib_sa.h                        |  5 +++++
 3 files changed, 19 insertions(+), 5 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

On Mon, Oct 17, 2016 at 10:01:00PM +0530, Binoy Jayan wrote:
> Counting semaphores are going away in the future, so replace the semaphore

> hns_roce_cmdq::event_sem with an open-coded implementation.


Sorry, but no.  Using a proper semaphore abstraction is always
better than open coding it.  While most semaphore users should move
to better primitives, those that actually are counting semaphore
should stick to the existing primitive.

On Monday, October 17, 2016 10:01:00 PM CEST Binoy Jayan wrote:
> --- a/drivers/infiniband/hw/hns/hns_roce_cmd.c

> +++ b/drivers/infiniband/hw/hns/hns_roce_cmd.c

> @@ -248,10 +248,14 @@ static int hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,

>  {

>         int ret = 0;

>  

> -       down(&hr_dev->cmd.event_sem);

> +       wait_event(hr_dev->cmd.event_sem.wq,

> +                  atomic_add_unless(&hr_dev->cmd.event_sem.count, -1, 0));

> +

>         ret = __hns_roce_cmd_mbox_wait(hr_dev, in_param, out_param,

>                                        in_modifier, op_modifier, op, timeout);

> -       up(&hr_dev->cmd.event_sem);

> +

> +       if (atomic_inc_return(&hr_dev->cmd.event_sem.count) == 1)

> +               wake_up(&hr_dev->cmd.event_sem.wq);

>  

>         return ret;

>  }


This is the only interesting use of the event_sem that cares about
the counting and it protects the __hns_roce_cmd_mbox_wait() from being
entered too often. The count here is the number of size of the
hr_dev->cmd.context[] array.

However, that function already use a spinlock to protect that array
and pick the correct context. I think changing the inner function
to handle the case of 'no context available' by using a waitqueue
without counting anything would be a reasonable transformation
away from the semaphore.

	Arnd

On 18 October 2016 at 01:59, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday, October 17, 2016 10:01:00 PM CEST Binoy Jayan wrote:

>> --- a/drivers/infiniband/hw/hns/hns_roce_cmd.c

>> +++ b/drivers/infiniband/hw/hns/hns_roce_cmd.c

>> @@ -248,10 +248,14 @@ static int hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,

>>  {

>>         int ret = 0;

>>

>> -       down(&hr_dev->cmd.event_sem);

>> +       wait_event(hr_dev->cmd.event_sem.wq,

>> +                  atomic_add_unless(&hr_dev->cmd.event_sem.count, -1, 0));

>> +

>>         ret = __hns_roce_cmd_mbox_wait(hr_dev, in_param, out_param,

>>                                        in_modifier, op_modifier, op, timeout);

>> -       up(&hr_dev->cmd.event_sem);

>> +

>> +       if (atomic_inc_return(&hr_dev->cmd.event_sem.count) == 1)

>> +               wake_up(&hr_dev->cmd.event_sem.wq);

>>

>>         return ret;

>>  }

>

> This is the only interesting use of the event_sem that cares about

> the counting and it protects the __hns_roce_cmd_mbox_wait() from being

> entered too often. The count here is the number of size of the

> hr_dev->cmd.context[] array.

>

> However, that function already use a spinlock to protect that array

> and pick the correct context. I think changing the inner function

> to handle the case of 'no context available' by using a waitqueue

> without counting anything would be a reasonable transformation

> away from the semaphore.

>

>         Arnd



Hi Arnd,

Thank you for replying for the questions. I''ll look for alternatives
for patches
6,7 and 8 and resend the series.

-Binoy

On Tuesday, October 18, 2016 10:46:45 AM CEST Binoy Jayan wrote:
> Thank you for replying for the questions. I''ll look for alternatives

> for patches 6,7 and 8 and resend the series.


Ok, thanks!

I also looked at patch 8 some more and noticed that those four
functions all do the exact same sequence:

- initialize a mlx5_ib_umr_context on the stack
- assign "umrwr.wr.wr_cqe = &umr_context.cqe"
- take the semaphore
- call ib_post_send with a single ib_send_wr
- wait for the mlx5_ib_umr_done() function to get called
- if we get back a failure, print a warning and return -EFAULT.
- release the semaphore

Moving all of these into a shared helper function would be
a good cleanup, and it leaves only a single function using
the semaphore, which can then be rewritten to use something
else.

The existing completion in there can be simplified to a
wait_event, since we are waiting for the return value to
be filled.

	Arnd