| Message ID | 1477551554-30349-6-git-send-email-binoy.jayan@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
> The semaphore 'sem' in isert_device is used as completion, so convert > it to struct completion. Semaphores are going away in the future. Umm, this is 100% *not* true. np->sem is designed as a counting to sync the iscsi login thread with the connect requests coming from the initiators. So this is actually a reliable bug insertion :( NAK from me... Also, I would appreciate if you include get_maintainer.pl in your patch submissions so I won't need to fish these in the Linux-rdma patch traffic.
Hi Sagi, On 31 October 2016 at 02:42, Sagi Grimberg <sagi@grimberg.me> wrote: >> The semaphore 'sem' in isert_device is used as completion, so convert >> it to struct completion. Semaphores are going away in the future. > > > Umm, this is 100% *not* true. np->sem is designed as a counting to > sync the iscsi login thread with the connect requests coming from the > initiators. So this is actually a reliable bug insertion :( > > NAK from me... Sorry for the late reply as I was held up in other activities. I converted this to a wait_event() implementation but as I was doing it, I was wondering how it would have been different if it was a completion and not a semaphore. File: drivers/infiniband/ulp/isert/ib_isert.c If isert_connected_handler() is called multiple times, adding an entry to the list, and if that happens while we use completion, 'done' (part of struct completion) would be incremented by 1 each time 'complete' is called from isert_connected_handler. After 'n' iterations, done will be equal to 'n'. If we call wait_for_completion now from isert_accept_np, it would just decrement 'done' by one and continue without blocking, consuming one node at a time from the list 'isert_np->pending'. Alternatively if "done" becomes zero, and the next time wait_for_completion is called, the API would add a node at the end of the wait queue 'wait' in 'struct completion' and block until "done" is nonzero. (Ref: do_wait_for_common) It exists the wait when a call to 'complete' turns 'done' back to 1. But if there are multiple waits called before calling complete, all the tasks calling the wait gets queued up and they will all would see "done" set to zero. When complete is called now, done turns 1 again and the first task in the queue is woken up as it is serialized as FIFO. Now the first wait returns and the done is decremented by 1 just before the return. Am I missing something here? Thanks, Binoy
On Friday, November 18, 2016 12:27:32 PM CET Binoy Jayan wrote: > Hi Sagi, > > On 31 October 2016 at 02:42, Sagi Grimberg <sagi@grimberg.me> wrote: > >> The semaphore 'sem' in isert_device is used as completion, so convert > >> it to struct completion. Semaphores are going away in the future. > > > > > > Umm, this is 100% *not* true. np->sem is designed as a counting to > > sync the iscsi login thread with the connect requests coming from the > > initiators. So this is actually a reliable bug insertion :( > > > > NAK from me... > > Sorry for the late reply as I was held up in other activities. > > I converted this to a wait_event() implementation but as I was doing it, > I was wondering how it would have been different if it was a completion > and not a semaphore. > > File: drivers/infiniband/ulp/isert/ib_isert.c > > If isert_connected_handler() is called multiple times, adding an entry to the > list, and if that happens while we use completion, 'done' (part of struct > completion) would be incremented by 1 each time 'complete' is called from > isert_connected_handler. After 'n' iterations, done will be equal to 'n'. If we > call wait_for_completion now from isert_accept_np, it would just decrement > 'done' by one and continue without blocking, consuming one node at a time > from the list 'isert_np->pending'. > > Alternatively if "done" becomes zero, and the next time wait_for_completion is > called, the API would add a node at the end of the wait queue 'wait' in 'struct > completion' and block until "done" is nonzero. (Ref: do_wait_for_common) > It exists the wait when a call to 'complete' turns 'done' back to 1. > But if there > are multiple waits called before calling complete, all the tasks > calling the wait > gets queued up and they will all would see "done" set to zero. When complete > is called now, done turns 1 again and the first task in the queue is woken up > as it is serialized as FIFO. Now the first wait returns and the done is > decremented by 1 just before the return. > > Am I missing something here? I think you are right. This is behavior is actuallly documented in Documentation/scheduler/completion.txt: If complete() is called multiple times then this will allow for that number of waiters to continue - each call to complete() will simply increment the done element. Calling complete_all() multiple times is a bug though. Both complete() and complete_all() can be called in hard-irq/atomic context safely. However, this is fairly unusual behavior and I wasn't immediately aware of it either when I read Sagi's reply. While your patch looks correct, it's probably a good idea to point out the counting behavior of this completion as explicitly as possible, in the changelog text of the patch as well as in a code comment and perhaps in the naming of the completion. Arnd
Hi Arnd, On 18 November 2016 at 14:28, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday, November 18, 2016 12:27:32 PM CET Binoy Jayan wrote: >> Hi Sagi, > I think you are right. This is behavior is actuallly documented in > Documentation/scheduler/completion.txt: Thanking for having a look. > However, this is fairly unusual behavior and I wasn't immediately aware > of it either when I read Sagi's reply. While your patch looks correct, > it's probably a good idea to point out the counting behavior of this > completion as explicitly as possible, in the changelog text of the patch > as well as in a code comment and perhaps in the naming of the completion. Will mention this and resend the patch series. Thanks, Binoy
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(&isert_np->mutex); isert_info("np %p: Allow accept_np to continue\n", isert_np); - up(&isert_np->sem); + complete(&isert_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(&isert_np->sem, 0); + init_completion(&isert_np->comp); mutex_init(&isert_np->mutex); INIT_LIST_HEAD(&isert_np->accepted); INIT_LIST_HEAD(&isert_np->pending); @@ -2427,7 +2427,7 @@ struct rdma_cm_id * int ret; accept_wait: - ret = down_interruptible(&isert_np->sem); + ret = wait_for_completion_interruptible(&isert_np->comp); if (ret) return -ENODEV; diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h index c02ada5..a1277c0 100644 --- a/drivers/infiniband/ulp/isert/ib_isert.h +++ b/drivers/infiniband/ulp/isert/ib_isert.h @@ -3,6 +3,7 @@ #include <linux/in6.h> #include <rdma/ib_verbs.h> #include <rdma/rdma_cm.h> +#include <linux/completion.h> #include <rdma/rw.h> #include <scsi/iser.h> @@ -190,7 +191,7 @@ struct isert_device { struct isert_np { struct iscsi_np *np; - struct semaphore sem; + struct completion comp; struct rdma_cm_id *cm_id; struct mutex mutex; struct list_head accepted;
The semaphore 'sem' in isert_device is used as completion, so convert it to struct completion. Semaphores are going away in the future. Signed-off-by: Binoy Jayan <binoy.jayan@linaro.org> --- drivers/infiniband/ulp/isert/ib_isert.c | 6 +++--- drivers/infiniband/ulp/isert/ib_isert.h | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project