Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1502180 > unrolled thread

[PATCH 0/8] infiniband: Remove semaphores

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] infiniband: Remove semaphores Binoy Jayan <binoy.jayan@linaro.org> - 2016-10-17 18:40 +0200
    [PATCH 1/8] IB/core: iwpm_nlmsg_request: Replace semaphore with completion Binoy Jayan <binoy.jayan@linaro.org> - 2016-10-17 18:40 +0200
    [PATCH 6/8] IB/hns: Replace counting semaphore event_sem with wait condition Binoy Jayan <binoy.jayan@linaro.org> - 2016-10-17 18:40 +0200
      Re: [PATCH 6/8] IB/hns: Replace counting semaphore event_sem with wait condition Arnd Bergmann <arnd@arndb.de> - 2016-10-17 22:40 +0200
        Re: [PATCH 6/8] IB/hns: Replace counting semaphore event_sem with  wait condition Binoy Jayan <binoy.jayan@linaro.org> - 2016-10-18 07:20 +0200
          Re: [PATCH 6/8] IB/hns: Replace counting semaphore event_sem with wait condition Arnd Bergmann <arnd@arndb.de> - 2016-10-19 17:20 +0200
    [PATCH 4/8] IB/mthca: Replace semaphore poll_sem with mutex Binoy Jayan <binoy.jayan@linaro.org> - 2016-10-17 18:40 +0200
    [PATCH 3/8] IB/hns: Replace semaphore poll_sem with mutex Binoy Jayan <binoy.jayan@linaro.org> - 2016-10-17 18:40 +0200
    [PATCH 2/8] IB/core: Replace semaphore sm_sem with completion Binoy Jayan <binoy.jayan@linaro.org> - 2016-10-17 18:40 +0200
    [PATCH 5/8] IB/isert: Replace semaphore sem with completion Binoy Jayan <binoy.jayan@linaro.org> - 2016-10-17 18:40 +0200
    Re: [PATCH 0/8] infiniband: Remove semaphores Arnd Bergmann <arnd@arndb.de> - 2016-10-17 22:10 +0200
      Re: [PATCH 0/8] infiniband: Remove semaphores Arnd Bergmann <arnd@arndb.de> - 2016-10-17 22:40 +0200

#1502180 — [PATCH 0/8] infiniband: Remove semaphores

Hi,

These are a set of patches which removes semaphores from infiniband.
These are part of a bigger effort to eliminate all semaphores from the
linux kernel.

NB: A few semaphores which are counting ones are replaced with an open-coded
implementation by introducing a new type in 'include/rdma/ib_sa.h'. Need to
see if this can be programmed in a generic way using wait queues.

Thanks,
Binoy

Binoy Jayan (8):
  IB/core: iwpm_nlmsg_request: Replace semaphore with completion
  IB/core: Replace semaphore sm_sem with completion
  IB/hns: Replace semaphore poll_sem with mutex
  IB/mthca: Replace semaphore poll_sem with mutex
  IB/isert: Replace semaphore sem with completion
  IB/hns: Replace counting semaphore event_sem with wait condition
  IB/mthca: Replace counting semaphore event_sem with wait condition
  IB/mlx5: Replace counting semaphore sem with wait condition

 drivers/infiniband/core/iwpm_msg.c          |  8 ++++----
 drivers/infiniband/core/iwpm_util.c         |  7 +++----
 drivers/infiniband/core/iwpm_util.h         |  3 ++-
 drivers/infiniband/core/user_mad.c          | 14 ++++++++------
 drivers/infiniband/hw/hns/hns_roce_cmd.c    | 28 ++++++++++++++++++----------
 drivers/infiniband/hw/hns/hns_roce_device.h |  6 ++++--
 drivers/infiniband/hw/mlx5/main.c           |  3 ++-
 drivers/infiniband/hw/mlx5/mlx5_ib.h        |  3 ++-
 drivers/infiniband/hw/mlx5/mr.c             | 28 +++++++++++++++++++---------
 drivers/infiniband/hw/mthca/mthca_cmd.c     | 22 +++++++++++++---------
 drivers/infiniband/hw/mthca/mthca_cmd.h     |  1 +
 drivers/infiniband/hw/mthca/mthca_dev.h     |  5 +++--
 drivers/infiniband/ulp/isert/ib_isert.c     |  6 +++---
 drivers/infiniband/ulp/isert/ib_isert.h     |  3 ++-
 include/rdma/ib_sa.h                        |  5 +++++
 15 files changed, 89 insertions(+), 53 deletions(-)

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

[toc] | [next] | [standalone]

#1502181 — [PATCH 1/8] IB/core: iwpm_nlmsg_request: Replace semaphore with completion

Semaphore sem in iwpm_nlmsg_request is used as completion, so
convert it to a struct completion type. Semaphores are going
away in the future.

Signed-off-by: Binoy Jayan <binoy.jayan@linaro.org>
---
 drivers/infiniband/core/iwpm_msg.c  | 8 ++++----
 drivers/infiniband/core/iwpm_util.c | 7 +++----
 drivers/infiniband/core/iwpm_util.h | 3 ++-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/core/iwpm_msg.c b/drivers/infiniband/core/iwpm_msg.c
index 1c41b95..761358f 100644
--- a/drivers/infiniband/core/iwpm_msg.c
+++ b/drivers/infiniband/core/iwpm_msg.c
@@ -394,7 +394,7 @@ int iwpm_register_pid_cb(struct sk_buff *skb, struct netlink_callback *cb)
 	/* always for found nlmsg_request */
 	kref_put(&nlmsg_request->kref, iwpm_free_nlmsg_request);
 	barrier();
-	up(&nlmsg_request->sem);
+	complete(&nlmsg_request->comp);
 	return 0;
 }
 EXPORT_SYMBOL(iwpm_register_pid_cb);
@@ -463,7 +463,7 @@ int iwpm_add_mapping_cb(struct sk_buff *skb, struct netlink_callback *cb)
 	/* always for found request */
 	kref_put(&nlmsg_request->kref, iwpm_free_nlmsg_request);
 	barrier();
-	up(&nlmsg_request->sem);
+	complete(&nlmsg_request->comp);
 	return 0;
 }
 EXPORT_SYMBOL(iwpm_add_mapping_cb);
@@ -555,7 +555,7 @@ int iwpm_add_and_query_mapping_cb(struct sk_buff *skb,
 	/* always for found request */
 	kref_put(&nlmsg_request->kref, iwpm_free_nlmsg_request);
 	barrier();
-	up(&nlmsg_request->sem);
+	complete(&nlmsg_request->comp);
 	return 0;
 }
 EXPORT_SYMBOL(iwpm_add_and_query_mapping_cb);
@@ -749,7 +749,7 @@ int iwpm_mapping_error_cb(struct sk_buff *skb, struct netlink_callback *cb)
 	/* always for found request */
 	kref_put(&nlmsg_request->kref, iwpm_free_nlmsg_request);
 	barrier();
-	up(&nlmsg_request->sem);
+	complete(&nlmsg_request->comp);
 	return 0;
 }
 EXPORT_SYMBOL(iwpm_mapping_error_cb);
diff --git a/drivers/infiniband/core/iwpm_util.c b/drivers/infiniband/core/iwpm_util.c
index ade71e7..08ddd2e 100644
--- a/drivers/infiniband/core/iwpm_util.c
+++ b/drivers/infiniband/core/iwpm_util.c
@@ -323,8 +323,7 @@ struct iwpm_nlmsg_request *iwpm_get_nlmsg_request(__u32 nlmsg_seq,
 	nlmsg_request->nl_client = nl_client;
 	nlmsg_request->request_done = 0;
 	nlmsg_request->err_code = 0;
-	sema_init(&nlmsg_request->sem, 1);
-	down(&nlmsg_request->sem);
+	init_completion(&nlmsg_request->comp);
 	return nlmsg_request;
 }
 
@@ -368,8 +367,8 @@ int iwpm_wait_complete_req(struct iwpm_nlmsg_request *nlmsg_request)
 {
 	int ret;
 
-	ret = down_timeout(&nlmsg_request->sem, IWPM_NL_TIMEOUT);
-	if (ret) {
+	ret = wait_for_completion_timeout(&nlmsg_request->comp, IWPM_NL_TIMEOUT);
+	if (!ret) {
 		ret = -EINVAL;
 		pr_info("%s: Timeout %d sec for netlink request (seq = %u)\n",
 			__func__, (IWPM_NL_TIMEOUT/HZ), nlmsg_request->nlmsg_seq);
diff --git a/drivers/infiniband/core/iwpm_util.h b/drivers/infiniband/core/iwpm_util.h
index af1fc14..ea6c299 100644
--- a/drivers/infiniband/core/iwpm_util.h
+++ b/drivers/infiniband/core/iwpm_util.h
@@ -43,6 +43,7 @@
 #include <linux/delay.h>
 #include <linux/workqueue.h>
 #include <linux/mutex.h>
+#include <linux/completion.h>
 #include <linux/jhash.h>
 #include <linux/kref.h>
 #include <net/netlink.h>
@@ -69,7 +70,7 @@ struct iwpm_nlmsg_request {
 	u8	            nl_client;
 	u8                  request_done;
 	u16                 err_code;
-	struct semaphore    sem;
+	struct completion   comp;
 	struct kref         kref;
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1502187 — [PATCH 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(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_cmd.c b/drivers/infiniband/hw/hns/hns_roce_cmd.c
index 1421fdb..3e76717 100644
--- 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;
 }
@@ -313,7 +317,9 @@ int hns_roce_cmd_use_events(struct hns_roce_dev *hr_dev)
 	hr_cmd->context[hr_cmd->max_cmds - 1].next = -1;
 	hr_cmd->free_head = 0;
 
-	sema_init(&hr_cmd->event_sem, hr_cmd->max_cmds);
+	init_waitqueue_head(&hr_cmd->event_sem.wq);
+	atomic_set(&hr_cmd->event_sem.count, hr_cmd->max_cmds);
+
 	spin_lock_init(&hr_cmd->context_lock);
 
 	hr_cmd->token_mask = CMD_TOKEN_MASK;
@@ -332,7 +338,9 @@ void hns_roce_cmd_use_polling(struct hns_roce_dev *hr_dev)
 	hr_cmd->use_events = 0;
 
 	for (i = 0; i < hr_cmd->max_cmds; ++i)
-		down(&hr_cmd->event_sem);
+		wait_event(hr_cmd->event_sem.wq,
+			   atomic_add_unless(
+			   &hr_dev->cmd.event_sem.count, -1, 0));
 
 	kfree(hr_cmd->context);
 	mutex_unlock(&hr_cmd->poll_mutex);
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
index 2afe075..6aed04a 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -34,6 +34,7 @@
 #define _HNS_ROCE_DEVICE_H
 
 #include <rdma/ib_verbs.h>
+#include <rdma/ib_sa.h>
 #include <linux/mutex.h>
 
 #define DRV_NAME "hns_roce"
@@ -364,7 +365,7 @@ struct hns_roce_cmdq {
 	* Event mode: cmd register mutex protection,
 	* ensure to not exceed max_cmds and user use limit region
 	*/
-	struct semaphore	event_sem;
+	struct ib_semaphore	event_sem;
 	int			max_cmds;
 	spinlock_t		context_lock;
 	int			free_head;
diff --git a/include/rdma/ib_sa.h b/include/rdma/ib_sa.h
index 5ee7aab..1901042 100644
--- a/include/rdma/ib_sa.h
+++ b/include/rdma/ib_sa.h
@@ -291,6 +291,11 @@ struct ib_sa_service_rec {
 #define IB_SA_GUIDINFO_REC_GID6		IB_SA_COMP_MASK(10)
 #define IB_SA_GUIDINFO_REC_GID7		IB_SA_COMP_MASK(11)
 
+struct ib_semaphore {
+	wait_queue_head_t wq;
+	atomic_t count;
+};
+
 struct ib_sa_guidinfo_rec {
 	__be16	lid;
 	u8	block_num;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1502438 — Re: [PATCH 6/8] IB/hns: Replace counting semaphore event_sem with wait condition

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

[toc] | [prev] | [next] | [standalone]

#1502696 — Re: [PATCH 6/8] IB/hns: Replace counting semaphore event_sem with wait condition

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

[toc] | [prev] | [next] | [standalone]

#1503860 — Re: [PATCH 6/8] IB/hns: Replace counting semaphore event_sem with wait condition

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

[toc] | [prev] | [next] | [standalone]

#1502190 — [PATCH 4/8] IB/mthca: Replace semaphore poll_sem with mutex

The semaphore 'poll_sem' is a simple mutex, so it should be written as one.
Semaphores are going away in the future.

Signed-off-by: Binoy Jayan <binoy.jayan@linaro.org>
---
 drivers/infiniband/hw/mthca/mthca_cmd.c | 10 +++++-----
 drivers/infiniband/hw/mthca/mthca_cmd.h |  1 +
 drivers/infiniband/hw/mthca/mthca_dev.h |  2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/hw/mthca/mthca_cmd.c b/drivers/infiniband/hw/mthca/mthca_cmd.c
index c7f49bb..0cb58ea 100644
--- a/drivers/infiniband/hw/mthca/mthca_cmd.c
+++ b/drivers/infiniband/hw/mthca/mthca_cmd.c
@@ -347,7 +347,7 @@ static int mthca_cmd_poll(struct mthca_dev *dev,
 	unsigned long end;
 	u8 status;
 
-	down(&dev->cmd.poll_sem);
+	mutex_lock(&dev->cmd.poll_mutex);
 
 	err = mthca_cmd_post(dev, in_param,
 			     out_param ? *out_param : 0,
@@ -382,7 +382,7 @@ static int mthca_cmd_poll(struct mthca_dev *dev,
 	}
 
 out:
-	up(&dev->cmd.poll_sem);
+	mutex_unlock(&dev->cmd.poll_mutex);
 	return err;
 }
 
@@ -520,7 +520,7 @@ static int mthca_cmd_imm(struct mthca_dev *dev,
 int mthca_cmd_init(struct mthca_dev *dev)
 {
 	mutex_init(&dev->cmd.hcr_mutex);
-	sema_init(&dev->cmd.poll_sem, 1);
+	mutex_init(&dev->cmd.poll_mutex);
 	dev->cmd.flags = 0;
 
 	dev->hcr = ioremap(pci_resource_start(dev->pdev, 0) + MTHCA_HCR_BASE,
@@ -582,7 +582,7 @@ int mthca_cmd_use_events(struct mthca_dev *dev)
 
 	dev->cmd.flags |= MTHCA_CMD_USE_EVENTS;
 
-	down(&dev->cmd.poll_sem);
+	mutex_lock(&dev->cmd.poll_mutex);
 
 	return 0;
 }
@@ -601,7 +601,7 @@ void mthca_cmd_use_polling(struct mthca_dev *dev)
 
 	kfree(dev->cmd.context);
 
-	up(&dev->cmd.poll_sem);
+	mutex_unlock(&dev->cmd.poll_mutex);
 }
 
 struct mthca_mailbox *mthca_alloc_mailbox(struct mthca_dev *dev,
diff --git a/drivers/infiniband/hw/mthca/mthca_cmd.h b/drivers/infiniband/hw/mthca/mthca_cmd.h
index d2e5b19..a7f197e 100644
--- a/drivers/infiniband/hw/mthca/mthca_cmd.h
+++ b/drivers/infiniband/hw/mthca/mthca_cmd.h
@@ -35,6 +35,7 @@
 #ifndef MTHCA_CMD_H
 #define MTHCA_CMD_H
 
+#include <linux/mutex.h>
 #include <rdma/ib_verbs.h>
 
 #define MTHCA_MAILBOX_SIZE 4096
diff --git a/drivers/infiniband/hw/mthca/mthca_dev.h b/drivers/infiniband/hw/mthca/mthca_dev.h
index 4393a02..87ab964 100644
--- a/drivers/infiniband/hw/mthca/mthca_dev.h
+++ b/drivers/infiniband/hw/mthca/mthca_dev.h
@@ -120,7 +120,7 @@ enum {
 struct mthca_cmd {
 	struct pci_pool          *pool;
 	struct mutex              hcr_mutex;
-	struct semaphore 	  poll_sem;
+	struct mutex		  poll_mutex;
 	struct semaphore 	  event_sem;
 	int              	  max_cmds;
 	spinlock_t                context_lock;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1502191 — [PATCH 3/8] IB/hns: Replace semaphore poll_sem with mutex

The semaphore 'poll_sem' is a simple mutex, so it should be written as one.
Semaphores are going away in the future.

Signed-off-by: Binoy Jayan <binoy.jayan@linaro.org>
---
 drivers/infiniband/hw/hns/hns_roce_cmd.c    | 12 ++++++------
 drivers/infiniband/hw/hns/hns_roce_device.h |  3 ++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_cmd.c b/drivers/infiniband/hw/hns/hns_roce_cmd.c
index 2a0b6c0..1421fdb 100644
--- a/drivers/infiniband/hw/hns/hns_roce_cmd.c
+++ b/drivers/infiniband/hw/hns/hns_roce_cmd.c
@@ -119,7 +119,7 @@ static int hns_roce_cmd_mbox_post_hw(struct hns_roce_dev *hr_dev, u64 in_param,
 	return ret;
 }
 
-/* this should be called with "poll_sem" */
+/* this should be called with "poll_mutex" */
 static int __hns_roce_cmd_mbox_poll(struct hns_roce_dev *hr_dev, u64 in_param,
 				    u64 out_param, unsigned long in_modifier,
 				    u8 op_modifier, u16 op,
@@ -167,10 +167,10 @@ static int hns_roce_cmd_mbox_poll(struct hns_roce_dev *hr_dev, u64 in_param,
 {
 	int ret;
 
-	down(&hr_dev->cmd.poll_sem);
+	mutex_lock(&hr_dev->cmd.poll_mutex);
 	ret = __hns_roce_cmd_mbox_poll(hr_dev, in_param, out_param, in_modifier,
 				       op_modifier, op, timeout);
-	up(&hr_dev->cmd.poll_sem);
+	mutex_unlock(&hr_dev->cmd.poll_mutex);
 
 	return ret;
 }
@@ -275,7 +275,7 @@ int hns_roce_cmd_init(struct hns_roce_dev *hr_dev)
 	struct device *dev = &hr_dev->pdev->dev;
 
 	mutex_init(&hr_dev->cmd.hcr_mutex);
-	sema_init(&hr_dev->cmd.poll_sem, 1);
+	mutex_init(&hr_dev->cmd.poll_mutex);
 	hr_dev->cmd.use_events = 0;
 	hr_dev->cmd.toggle = 1;
 	hr_dev->cmd.max_cmds = CMD_MAX_NUM;
@@ -319,7 +319,7 @@ int hns_roce_cmd_use_events(struct hns_roce_dev *hr_dev)
 	hr_cmd->token_mask = CMD_TOKEN_MASK;
 	hr_cmd->use_events = 1;
 
-	down(&hr_cmd->poll_sem);
+	mutex_lock(&hr_cmd->poll_mutex);
 
 	return 0;
 }
@@ -335,7 +335,7 @@ void hns_roce_cmd_use_polling(struct hns_roce_dev *hr_dev)
 		down(&hr_cmd->event_sem);
 
 	kfree(hr_cmd->context);
-	up(&hr_cmd->poll_sem);
+	mutex_unlock(&hr_cmd->poll_mutex);
 }
 
 struct hns_roce_cmd_mailbox
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
index 3417315..2afe075 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -34,6 +34,7 @@
 #define _HNS_ROCE_DEVICE_H
 
 #include <rdma/ib_verbs.h>
+#include <linux/mutex.h>
 
 #define DRV_NAME "hns_roce"
 
@@ -358,7 +359,7 @@ struct hns_roce_cmdq {
 	struct dma_pool		*pool;
 	u8 __iomem		*hcr;
 	struct mutex		hcr_mutex;
-	struct semaphore	poll_sem;
+	struct mutex		poll_mutex;
 	/*
 	* Event mode: cmd register mutex protection,
 	* ensure to not exceed max_cmds and user use limit region
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1502193 — [PATCH 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(-)

diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
index 415a318..df070cc 100644
--- a/drivers/infiniband/core/user_mad.c
+++ b/drivers/infiniband/core/user_mad.c
@@ -47,6 +47,7 @@
 #include <linux/kref.h>
 #include <linux/compat.h>
 #include <linux/sched.h>
+#include <linux/completion.h>
 #include <linux/semaphore.h>
 #include <linux/slab.h>
 
@@ -87,7 +88,7 @@ struct ib_umad_port {
 
 	struct cdev           sm_cdev;
 	struct device	      *sm_dev;
-	struct semaphore       sm_sem;
+	struct completion      sm_comp;
 
 	struct mutex	       file_mutex;
 	struct list_head       file_list;
@@ -1030,12 +1031,12 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	port = container_of(inode->i_cdev, struct ib_umad_port, sm_cdev);
 
 	if (filp->f_flags & O_NONBLOCK) {
-		if (down_trylock(&port->sm_sem)) {
+		if (!try_wait_for_completion(&port->sm_comp)) {
 			ret = -EAGAIN;
 			goto fail;
 		}
 	} else {
-		if (down_interruptible(&port->sm_sem)) {
+		if (wait_for_completion_interruptible(&port->sm_comp)) {
 			ret = -ERESTARTSYS;
 			goto fail;
 		}
@@ -1060,7 +1061,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	ib_modify_port(port->ib_dev, port->port_num, 0, &props);
 
 err_up_sem:
-	up(&port->sm_sem);
+	complete(&port->sm_comp);
 
 fail:
 	return ret;
@@ -1079,7 +1080,7 @@ static int ib_umad_sm_close(struct inode *inode, struct file *filp)
 		ret = ib_modify_port(port->ib_dev, port->port_num, 0, &props);
 	mutex_unlock(&port->file_mutex);
 
-	up(&port->sm_sem);
+	complete(&port->sm_comp);
 
 	kobject_put(&port->umad_dev->kobj);
 
@@ -1177,7 +1178,8 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
 
 	port->ib_dev   = device;
 	port->port_num = port_num;
-	sema_init(&port->sm_sem, 1);
+	init_completion(&port->sm_comp);
+	complete(&port->sm_comp);
 	mutex_init(&port->file_mutex);
 	INIT_LIST_HEAD(&port->file_list);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1502194 — [PATCH 5/8] IB/isert: Replace semaphore sem with completion

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(-)

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 Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1502416

On Monday, October 17, 2016 9:57:34 AM CEST Bart Van Assche wrote:
> On 10/17/2016 09:30 AM, Binoy Jayan wrote:
> > These are a set of patches which removes semaphores from infiniband.
> > These are part of a bigger effort to eliminate all semaphores from the
> > linux kernel.
> 
> Hello Binoy,
> 
> Why do you think it would be a good idea to eliminate all semaphores 
> from the Linux kernel? I don't know anyone who doesn't consider 
> semaphores a useful abstraction.

There are a several reasons why the semaphores as defined in the
kernel are bad and we should get rid of them:

- semaphores cannot be analysed using lockdep, since they don't
  always fit in the simpler 'mutex' semantics

- those that are basically mutexes should be converted to mutexes
  for efficiency and consistency anyway

- the semaphores that are not just used as mutexes are typically
  used as completions and should just be converted to completions
  for simplicity

- when running a preempt-rt kernel, semaphores suffer from priority
  inversion problems, while mutexes use use priority inheritance
  as a countermeasure

There are very few remaining semaphores in the kernel and generally
speaking we'd be better off removing them all so no new users
show up in the future. Most of them are trivial to replace with
mutexes or completions. For the ones that are not trivially replaced,
we have to look at each one and decide what to do about them,
there usually is some solution that actually improves the code.

Using an open-coded semaphore as a replacement is probably just
the last resort that we can consider once we are down to the
last handful of users. I haven't looked at drivers/infiniband/
yet for this, but I believe that drivers/acpi/ is a case for
which I see no better alternative (the AML bytecode requires
counting semaphore semantics).

	Arnd

[toc] | [prev] | [next] | [standalone]

#1502436

On Monday, October 17, 2016 1:28:01 PM CEST Bart Van Assche wrote:
> On 10/17/2016 01:06 PM, Arnd Bergmann wrote:
> > Using an open-coded semaphore as a replacement is probably just
> > the last resort that we can consider once we are down to the
> > last handful of users. I haven't looked at drivers/infiniband/
> > yet for this, but I believe that drivers/acpi/ is a case for
> > which I see no better alternative (the AML bytecode requires
> > counting semaphore semantics).
> 
> Hello Arnd,
> 
> Thanks for the detailed reply. However, I doubt that removing and 
> open-coding counting semaphores is the best alternative. Counting 
> semaphores are a useful abstraction. I think open-coding counting 
> semaphores everywhere counting semaphores are used would be a step back 
> instead of a step forward.

Absolutely agreed, that's why I said 'last resort' above. I don't
think that we need to go there for infiniband. See my reply
for patch 6 for one idea on how to handle hns and mthca. There
might be better ways.

These fall into the general category of using the counting semaphore
to count something that we already know in the code that uses
the semaphore, so we can remove the count and just need some other
waiting primitive.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web