[v2,2/8] IB/core: Replace semaphore sm_sem with completion

The semaphore 'sm_sem' is used as completion, so convert it to
struct completion. Semaphores are going away in the future. The initial
status of the completion variable is marked as completed by a call to
the function 'complete' immediately following the initialization.

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

---
 drivers/infiniband/core/user_mad.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

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

Hi Binoy,

snip
>

>         port->ib_dev   = device;

>         port->port_num = port_num;

> -       sema_init(&port->sm_sem, 1);

> +       init_completion(&port->sm_comp);

> +       complete(&port->sm_comp);


Why complete here?

>         mutex_init(&port->file_mutex);

>         INIT_LIST_HEAD(&port->file_list);

>

> --

KR,
Jinpu

On 25 October 2016 at 18:13, Jack Wang <xjtuwjp@gmail.com> wrote:
> Hi Binoy,

>

> snip

>>

>>         port->ib_dev   = device;

>>         port->port_num = port_num;

>> -       sema_init(&port->sm_sem, 1);

>> +       init_completion(&port->sm_comp);

>> +       complete(&port->sm_comp);

>

> Why complete here?

>

>>         mutex_init(&port->file_mutex);

>>         INIT_LIST_HEAD(&port->file_list);

>>

>> --

> KR,

> Jinpu



Hi Jack,

ib_umad_sm_open() calls wait_for_completion_interruptible() which comes before
ib_umad_sm_close() that calls complete(). In the initial open() there
will not be
anybody to signal the completion, so the complete is called to mark
the initial state.
I am not sure if this is the right way to do it, though.

-Binoy

Hi Binoy,
2016-10-25 17:08 GMT+02:00 Binoy Jayan <binoy.jayan@linaro.org>:
> On 25 October 2016 at 18:13, Jack Wang <xjtuwjp@gmail.com> wrote:

>> Hi Binoy,

>>

>> snip

>>>

>>>         port->ib_dev   = device;

>>>         port->port_num = port_num;

>>> -       sema_init(&port->sm_sem, 1);

>>> +       init_completion(&port->sm_comp);

>>> +       complete(&port->sm_comp);

>>

>> Why complete here?

>>

>>>         mutex_init(&port->file_mutex);

>>>         INIT_LIST_HEAD(&port->file_list);

>>>

>>> --

>> KR,

>> Jinpu

>

>

> Hi Jack,

>

> ib_umad_sm_open() calls wait_for_completion_interruptible() which comes before

> ib_umad_sm_close() that calls complete(). In the initial open() there

> will not be

> anybody to signal the completion, so the complete is called to mark

> the initial state.

> I am not sure if this is the right way to do it, though.

>

> -Binoy


From Documentation/scheduler/completion.txt ,
"
117 This is not implying any temporal order on wait_for_completion() and the
118 call to complete() - if the call to complete() happened before the call
119 to wait_for_completion() then the waiting side simply will continue
120 immediately as all dependencies are satisfied if not it will block until
121 completion is signaled by complete().
"
In this case here, if sm_open/sm_close are paired, it should work.

KR
Jack

On Tue, Oct 25, 2016 at 08:38:21PM +0530, Binoy Jayan wrote:
> On 25 October 2016 at 18:13, Jack Wang <xjtuwjp@gmail.com> wrote:

> > Hi Binoy,

> >

> > snip

> >>

> >>         port->ib_dev   = device;

> >>         port->port_num = port_num;

> >> -       sema_init(&port->sm_sem, 1);

> >> +       init_completion(&port->sm_comp);

> >> +       complete(&port->sm_comp);

> >

> > Why complete here?

> >

> >>         mutex_init(&port->file_mutex);

> >>         INIT_LIST_HEAD(&port->file_list);

> >>

> > KR,

> > Jinpu

> 

> 

> Hi Jack,

> 

> ib_umad_sm_open() calls wait_for_completion_interruptible() which

> comes before ib_umad_sm_close() that calls complete(). In the

> initial open() there will not be anybody to signal the completion,

> so the complete is called to mark the initial state.  I am not sure

> if this is the right way to do it, though.


Using a completion to model exclusive ownership seems convoluted to
me - is that a thing now? What about an atomic?

open:

while (true) {
   wait_event_interruptible(priv->queue,test_bit(CLAIMED_BIT, &priv->flags));
   if (!test_and_set_bit(CLAIMED_BIT, &priv->flags))
        break;
}

close():

   clear_bit(CLAIMED_BIT, &priv->flags)
   wake_up(&priv->queue);

??

Jason

On Tuesday, October 25, 2016 9:48:22 AM CEST Jason Gunthorpe wrote:
> 

> Using a completion to model exclusive ownership seems convoluted to

> me - is that a thing now? What about an atomic?


I also totally missed how this is used. I initially mentioned to Binoy
that almost all semaphores can either be converted to a mutex or a
completion unless they rely on the counting behavior of the semaphore.

This driver clearly falls into another category and none of the above
apply here.

> open:

> 

> while (true) {

>    wait_event_interruptible(priv->queue,test_bit(CLAIMED_BIT, &priv->flags));

>    if (!test_and_set_bit(CLAIMED_BIT, &priv->flags))

>         break;

> }


I'd fold the test_and_set_bit() into the wait_event() and get rid of the
loop here, otherwise this looks nice.

I'm generally a bit suspicious of open() functions that try to
protect you from having multiple file descriptors open, as dup()
or fork() will also result in extra file descriptors, but this
use here seems harmless, as the device itself only supports
open() and close() and the only thing it does is to keep track
of whether it is open or not.

It's also interesting that the ib_modify_port() function also
keeps track of the a flag that is set in open() and cleared
in close(), so in principle we should be able to unify this
with the semaphore, but the wait_event() approach is certainly
much simpler.

	Arnd