Re: [PATCH v5 5/9] IB/isert: Replace semaphore sem with completion
On Monday, November 21, 2016 2:33:02 PM CET Sagi Grimberg wrote:
> >>> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
> >>> b/drivers/infiniband/ulp/isert/ib_isert.c
> >>> index 6dd43f6..de80f56 100644
> >>> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> >>> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> >>> @@ -619,7 +619,7 @@
> >>> mutex_unlock(_np->mutex);
> >>>
> >>> isert_info("np %p: Allow accept_np to continue\n", isert_np);
> >>> - up(_np->sem);
> >>> + complete(_np->comp);
> >>> }
> >>>
> >>> static void
> >>> @@ -2311,7 +2311,7 @@ struct rdma_cm_id *
> >>> isert_err("Unable to allocate struct isert_np\n");
> >>> return -ENOMEM;
> >>> }
> >>> - sema_init(_np->sem, 0);
> >>> + init_completion(_np->comp);
> >>
> >> This is still racy, a connect event can complete just before we
> >> init the completion and *will* get lost...
> >>
> >> This code started off with a waitqueue which exposes the same
> >> problem, see:
> >> 531b7bf4bd79 Target/iser: Fix iscsit_accept_np and rdma_cm racy flow
> >>
> >> So, still NAK from me...
> >
> > I don't see what you mean here. The code using a waitqueue had no
> > counter but the completion does.
>
> The problem here is that init_completion sets the counter to zero
> and between this thread wakes up and until it initializes comp->done
> we might have another connect event completing it and I fail to
> see how this is handled correctly.
But the sema_init()/init_completion() is done right after the
structure is allocated, so nothing should have a pointer to it
at this time.
Later when we do the down()/wait_for_completion(), that will
just decrement the counter rather than initialize, and converting
from one to the other doesn't change anything here.
Arnd
Re: [PATCH v5 5/9] IB/isert: Replace semaphore sem with completion
On Monday, November 21, 2016 2:33:02 PM CET Sagi Grimberg wrote:
> >>> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
> >>> b/drivers/infiniband/ulp/isert/ib_isert.c
> >>> index 6dd43f6..de80f56 100644
> >>> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> >>> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> >>> @@ -619,7 +619,7 @@
> >>> mutex_unlock(_np->mutex);
> >>>
> >>> isert_info("np %p: Allow accept_np to continue\n", isert_np);
> >>> - up(_np->sem);
> >>> + complete(_np->comp);
> >>> }
> >>>
> >>> static void
> >>> @@ -2311,7 +2311,7 @@ struct rdma_cm_id *
> >>> isert_err("Unable to allocate struct isert_np\n");
> >>> return -ENOMEM;
> >>> }
> >>> - sema_init(_np->sem, 0);
> >>> + init_completion(_np->comp);
> >>
> >> This is still racy, a connect event can complete just before we
> >> init the completion and *will* get lost...
> >>
> >> This code started off with a waitqueue which exposes the same
> >> problem, see:
> >> 531b7bf4bd79 Target/iser: Fix iscsit_accept_np and rdma_cm racy flow
> >>
> >> So, still NAK from me...
> >
> > I don't see what you mean here. The code using a waitqueue had no
> > counter but the completion does.
>
> The problem here is that init_completion sets the counter to zero
> and between this thread wakes up and until it initializes comp->done
> we might have another connect event completing it and I fail to
> see how this is handled correctly.
But the sema_init()/init_completion() is done right after the
structure is allocated, so nothing should have a pointer to it
at this time.
Later when we do the down()/wait_for_completion(), that will
just decrement the counter rather than initialize, and converting
from one to the other doesn't change anything here.
Arnd
Re: [PATCH v5 5/9] IB/isert: Replace semaphore sem with completion
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
b/drivers/infiniband/ulp/isert/ib_isert.c
index 6dd43f6..de80f56 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -619,7 +619,7 @@
mutex_unlock(_np->mutex);
isert_info("np %p: Allow accept_np to continue\n", isert_np);
- up(_np->sem);
+ complete(_np->comp);
}
static void
@@ -2311,7 +2311,7 @@ struct rdma_cm_id *
isert_err("Unable to allocate struct isert_np\n");
return -ENOMEM;
}
- sema_init(_np->sem, 0);
+ init_completion(_np->comp);
This is still racy, a connect event can complete just before we
init the completion and *will* get lost...
This code started off with a waitqueue which exposes the same
problem, see:
531b7bf4bd79 Target/iser: Fix iscsit_accept_np and rdma_cm racy flow
So, still NAK from me...
I don't see what you mean here. The code using a waitqueue had no
counter but the completion does.
The problem here is that init_completion sets the counter to zero
and between this thread wakes up and until it initializes comp->done
we might have another connect event completing it and I fail to
see how this is handled correctly.
Re: [PATCH v5 5/9] IB/isert: Replace semaphore sem with completion
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
b/drivers/infiniband/ulp/isert/ib_isert.c
index 6dd43f6..de80f56 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -619,7 +619,7 @@
mutex_unlock(_np->mutex);
isert_info("np %p: Allow accept_np to continue\n", isert_np);
- up(_np->sem);
+ complete(_np->comp);
}
static void
@@ -2311,7 +2311,7 @@ struct rdma_cm_id *
isert_err("Unable to allocate struct isert_np\n");
return -ENOMEM;
}
- sema_init(_np->sem, 0);
+ init_completion(_np->comp);
This is still racy, a connect event can complete just before we
init the completion and *will* get lost...
This code started off with a waitqueue which exposes the same
problem, see:
531b7bf4bd79 Target/iser: Fix iscsit_accept_np and rdma_cm racy flow
So, still NAK from me...
I don't see what you mean here. The code using a waitqueue had no
counter but the completion does.
The problem here is that init_completion sets the counter to zero
and between this thread wakes up and until it initializes comp->done
we might have another connect event completing it and I fail to
see how this is handled correctly.
Re: [PATCH v5 5/9] IB/isert: Replace semaphore sem with completion
On Monday, November 21, 2016 9:36:22 AM CET Sagi Grimberg wrote:
> > diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
> > b/drivers/infiniband/ulp/isert/ib_isert.c
> > index 6dd43f6..de80f56 100644
> > --- a/drivers/infiniband/ulp/isert/ib_isert.c
> > +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> > @@ -619,7 +619,7 @@
> > mutex_unlock(_np->mutex);
> >
> > isert_info("np %p: Allow accept_np to continue\n", isert_np);
> > - up(_np->sem);
> > + complete(_np->comp);
> > }
> >
> > static void
> > @@ -2311,7 +2311,7 @@ struct rdma_cm_id *
> > isert_err("Unable to allocate struct isert_np\n");
> > return -ENOMEM;
> > }
> > - sema_init(_np->sem, 0);
> > + init_completion(_np->comp);
>
> This is still racy, a connect event can complete just before we
> init the completion and *will* get lost...
>
> This code started off with a waitqueue which exposes the same
> problem, see:
> 531b7bf4bd79 Target/iser: Fix iscsit_accept_np and rdma_cm racy flow
>
> So, still NAK from me...
I don't see what you mean here. The code using a waitqueue had no
counter but the completion does. I had suggested that Binoy add
a comment into the code about this, as it is a rarely used
property of completions, but it does seem entirely valid to me.
Arnd
Re: [PATCH v5 5/9] IB/isert: Replace semaphore sem with completion
On Monday, November 21, 2016 9:36:22 AM CET Sagi Grimberg wrote:
> > diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
> > b/drivers/infiniband/ulp/isert/ib_isert.c
> > index 6dd43f6..de80f56 100644
> > --- a/drivers/infiniband/ulp/isert/ib_isert.c
> > +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> > @@ -619,7 +619,7 @@
> > mutex_unlock(_np->mutex);
> >
> > isert_info("np %p: Allow accept_np to continue\n", isert_np);
> > - up(_np->sem);
> > + complete(_np->comp);
> > }
> >
> > static void
> > @@ -2311,7 +2311,7 @@ struct rdma_cm_id *
> > isert_err("Unable to allocate struct isert_np\n");
> > return -ENOMEM;
> > }
> > - sema_init(_np->sem, 0);
> > + init_completion(_np->comp);
>
> This is still racy, a connect event can complete just before we
> init the completion and *will* get lost...
>
> This code started off with a waitqueue which exposes the same
> problem, see:
> 531b7bf4bd79 Target/iser: Fix iscsit_accept_np and rdma_cm racy flow
>
> So, still NAK from me...
I don't see what you mean here. The code using a waitqueue had no
counter but the completion does. I had suggested that Binoy add
a comment into the code about this, as it is a rarely used
property of completions, but it does seem entirely valid to me.
Arnd
Re: [PATCH v5 5/9] IB/isert: Replace semaphore sem with completion
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
b/drivers/infiniband/ulp/isert/ib_isert.c
index 6dd43f6..de80f56 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -619,7 +619,7 @@
mutex_unlock(_np->mutex);
isert_info("np %p: Allow accept_np to continue\n", isert_np);
- up(_np->sem);
+ complete(_np->comp);
}
static void
@@ -2311,7 +2311,7 @@ struct rdma_cm_id *
isert_err("Unable to allocate struct isert_np\n");
return -ENOMEM;
}
- sema_init(_np->sem, 0);
+ init_completion(_np->comp);
This is still racy, a connect event can complete just before we
init the completion and *will* get lost...
This code started off with a waitqueue which exposes the same
problem, see:
531b7bf4bd79 Target/iser: Fix iscsit_accept_np and rdma_cm racy flow
So, still NAK from me...
Re: [PATCH v5 5/9] IB/isert: Replace semaphore sem with completion
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
b/drivers/infiniband/ulp/isert/ib_isert.c
index 6dd43f6..de80f56 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -619,7 +619,7 @@
mutex_unlock(_np->mutex);
isert_info("np %p: Allow accept_np to continue\n", isert_np);
- up(_np->sem);
+ complete(_np->comp);
}
static void
@@ -2311,7 +2311,7 @@ struct rdma_cm_id *
isert_err("Unable to allocate struct isert_np\n");
return -ENOMEM;
}
- sema_init(_np->sem, 0);
+ init_completion(_np->comp);
This is still racy, a connect event can complete just before we
init the completion and *will* get lost...
This code started off with a waitqueue which exposes the same
problem, see:
531b7bf4bd79 Target/iser: Fix iscsit_accept_np and rdma_cm racy flow
So, still NAK from me...
