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

Groups > linux.kernel > #1508236 > unrolled thread

[PATCH v2 6/8] IB/hns: Replace counting semaphore event_sem with wait_event

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 6/8] IB/hns: Replace counting semaphore event_sem with wait_event Binoy Jayan <binoy.jayan@linaro.org> - 2016-10-25 14:10 +0200
    Re: [PATCH v2 6/8] IB/hns: Replace counting semaphore event_sem with wait_event Arnd Bergmann <arnd@arndb.de> - 2016-10-25 14:30 +0200
      Re: [PATCH v2 6/8] IB/hns: Replace counting semaphore event_sem with wait_event Binoy Jayan <binoy.jayan@linaro.org> - 2016-10-25 15:10 +0200
        Re: [PATCH v2 6/8] IB/hns: Replace counting semaphore event_sem with wait_event Arnd Bergmann <arnd@arndb.de> - 2016-10-25 15:30 +0200
          Re: [PATCH v2 6/8] IB/hns: Replace counting semaphore event_sem with wait_event Binoy Jayan <binoy.jayan@linaro.org> - 2016-10-25 15:40 +0200

#1508236 — [PATCH v2 6/8] IB/hns: Replace counting semaphore event_sem with wait_event

Counting semaphores are going away in the future, so replace the semaphore
mthca_cmd::event_sem with a conditional wait_event.

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

diff --git a/drivers/infiniband/hw/hns/hns_roce_cmd.c b/drivers/infiniband/hw/hns/hns_roce_cmd.c
index 51a0675..efc5c48 100644
--- a/drivers/infiniband/hw/hns/hns_roce_cmd.c
+++ b/drivers/infiniband/hw/hns/hns_roce_cmd.c
@@ -189,6 +189,26 @@ void hns_roce_cmd_event(struct hns_roce_dev *hr_dev, u16 token, u8 status,
 	complete(&context->done);
 }
 
+/* Similar to atomic_cmpxchg but with the complimentary condition. Returns
+ * index to a free node. It also sets cmd->free_head to 'new' so it ensures
+ * atomicity between a call to 'wait_event' and manipulating the free_head.
+ */
+
+static inline int atomic_free_node(struct hns_roce_cmdq *cmd, int new)
+{
+	int orig;
+
+	spin_lock(&cmd->context_lock);
+
+	orig = cmd->free_head;
+	if (likely(cmd->free_head != -1))
+		cmd->free_head = new;
+
+	spin_unlock(&cmd->context_lock);
+
+	return orig;
+}
+
 /* this should be called with "use_events" */
 static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
 				    u64 out_param, unsigned long in_modifier,
@@ -198,11 +218,12 @@ static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
 	struct hns_roce_cmdq *cmd = &hr_dev->cmd;
 	struct device *dev = &hr_dev->pdev->dev;
 	struct hns_roce_cmd_context *context;
-	int ret = 0;
+	int orig_free_head, ret = 0;
+
+	wait_event(cmd->wq, (orig_free_head = atomic_free_node(cmd, -1)) != -1);
 
 	spin_lock(&cmd->context_lock);
-	WARN_ON(cmd->free_head < 0);
-	context = &cmd->context[cmd->free_head];
+	context = &cmd->context[orig_free_head];
 	context->token += cmd->token_mask + 1;
 	cmd->free_head = context->next;
 	spin_unlock(&cmd->context_lock);
@@ -238,6 +259,7 @@ static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
 	context->next = cmd->free_head;
 	cmd->free_head = context - cmd->context;
 	spin_unlock(&cmd->context_lock);
+	wake_up(&cmd->wq);
 
 	return ret;
 }
@@ -248,10 +270,8 @@ 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);
 	ret = __hns_roce_cmd_mbox_wait(hr_dev, in_param, out_param,
 				       in_modifier, op_modifier, op, timeout);
-	up(&hr_dev->cmd.event_sem);
 
 	return ret;
 }
@@ -313,7 +333,7 @@ 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->wq);
 	spin_lock_init(&hr_cmd->context_lock);
 
 	hr_cmd->token_mask = CMD_TOKEN_MASK;
@@ -325,12 +345,9 @@ int hns_roce_cmd_use_events(struct hns_roce_dev *hr_dev)
 void hns_roce_cmd_use_polling(struct hns_roce_dev *hr_dev)
 {
 	struct hns_roce_cmdq *hr_cmd = &hr_dev->cmd;
-	int i;
 
 	hr_cmd->use_events = 0;
-
-	for (i = 0; i < hr_cmd->max_cmds; ++i)
-		down(&hr_cmd->event_sem);
+	hr_cmd->free_head = -1;
 
 	kfree(hr_cmd->context);
 }
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
index 2afe075..ac95f52 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -364,7 +364,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;
+	wait_queue_head_t       wq;
 	int			max_cmds;
 	spinlock_t		context_lock;
 	int			free_head;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1508257

On Tuesday, October 25, 2016 5:31:57 PM CEST Binoy Jayan wrote:
>  static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
>                                     u64 out_param, unsigned long in_modifier,
> @@ -198,11 +218,12 @@ static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
>         struct hns_roce_cmdq *cmd = &hr_dev->cmd;
>         struct device *dev = &hr_dev->pdev->dev;
>         struct hns_roce_cmd_context *context;
> -       int ret = 0;
> +       int orig_free_head, ret = 0;
> +
> +       wait_event(cmd->wq, (orig_free_head = atomic_free_node(cmd, -1)) != -1);
>  
>         spin_lock(&cmd->context_lock);
> -       WARN_ON(cmd->free_head < 0);
> -       context = &cmd->context[cmd->free_head];
> +       context = &cmd->context[orig_free_head];
>         context->token += cmd->token_mask + 1;
>         cmd->free_head = context->next;
>         spin_unlock(&cmd->context_lock);
> 

You get the lock in atomic_free_node() and then again right after that.
Why not combine the two and only take the lock inside of that
function that returns a context?

	Arnd

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

#1508284

On 25 October 2016 at 17:58, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday, October 25, 2016 5:31:57 PM CEST Binoy Jayan wrote:
>>  static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
>>                                     u64 out_param, unsigned long in_modifier,
>> @@ -198,11 +218,12 @@ static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
>>         struct hns_roce_cmdq *cmd = &hr_dev->cmd;
>>         struct device *dev = &hr_dev->pdev->dev;
>>         struct hns_roce_cmd_context *context;
>> -       int ret = 0;
>> +       int orig_free_head, ret = 0;
>> +
>> +       wait_event(cmd->wq, (orig_free_head = atomic_free_node(cmd, -1)) != -1);
>>
>>         spin_lock(&cmd->context_lock);
>> -       WARN_ON(cmd->free_head < 0);
>> -       context = &cmd->context[cmd->free_head];
>> +       context = &cmd->context[orig_free_head];
>>         context->token += cmd->token_mask + 1;
>>         cmd->free_head = context->next;
>>         spin_unlock(&cmd->context_lock);
>>
>
> You get the lock in atomic_free_node() and then again right after that.
> Why not combine the two and only take the lock inside of that
> function that returns a context?


Hi Arnd,

I couldn't figure out a way to wait for a node to be free followed by
acquiring a lock
in an atomic fashion. If the lock is acquired after the wait_event,
there could be race
between the wait_event and acquiring the lock. If the lock is acquired
before the
wait_event, the process may goto sleep with the lock held which is not desired.
Could you suggest me of some way to circumvent this?

-Binoy

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

#1508293

On Tuesday, October 25, 2016 6:29:45 PM CEST Binoy Jayan wrote:
> On 25 October 2016 at 17:58, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday, October 25, 2016 5:31:57 PM CEST Binoy Jayan wrote:
> >>  static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
> >>                                     u64 out_param, unsigned long in_modifier,
> >> @@ -198,11 +218,12 @@ static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
> >>         struct hns_roce_cmdq *cmd = &hr_dev->cmd;
> >>         struct device *dev = &hr_dev->pdev->dev;
> >>         struct hns_roce_cmd_context *context;
> >> -       int ret = 0;
> >> +       int orig_free_head, ret = 0;
> >> +
> >> +       wait_event(cmd->wq, (orig_free_head = atomic_free_node(cmd, -1)) != -1);
> >>
> >>         spin_lock(&cmd->context_lock);
> >> -       WARN_ON(cmd->free_head < 0);
> >> -       context = &cmd->context[cmd->free_head];
> >> +       context = &cmd->context[orig_free_head];
> >>         context->token += cmd->token_mask + 1;
> >>         cmd->free_head = context->next;
> >>         spin_unlock(&cmd->context_lock);
> >>
> >
> > You get the lock in atomic_free_node() and then again right after that.
> > Why not combine the two and only take the lock inside of that
> > function that returns a context?
> 
> 
> Hi Arnd,
> 
> I couldn't figure out a way to wait for a node to be free followed by
> acquiring a lock
> in an atomic fashion. If the lock is acquired after the wait_event,
> there could be race
> between the wait_event and acquiring the lock. If the lock is acquired
> before the
> wait_event, the process may goto sleep with the lock held which is not desired.
> Could you suggest me of some way to circumvent this?

Something like

static struct hns_roce_cmd_context *hns_roce_try_get_context(struct hns_roce_cmdq *cmd)
{
	struct hns_roce_cmd_context *context = NULL;

	spin_lock(&cmd->context_lock);

	if (cmd->free_head < 0)
		goto out;

	context = &cmd->context[cmd->free_head];

	... /* update free_head */

out:
	spin_unlock(&cmd->context_lock);

	return context;
}
...

static struct hns_roce_cmd_context *hns_roce_get_context(struct hns_roce_cmdq *cmd)
{
	struct hns_roce_cmd_context *context;

	wait_event(cmd->wq, (context = hns_roce_try_get_context(cmd)));

	return context;
}

	Arnd

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

#1508298

On 25 October 2016 at 18:51, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday, October 25, 2016 6:29:45 PM CEST Binoy Jayan wrote:
>
> Something like
>
> static struct hns_roce_cmd_context *hns_roce_try_get_context(struct hns_roce_cmdq *cmd)
> {
>         struct hns_roce_cmd_context *context = NULL;
>
>         spin_lock(&cmd->context_lock);
>
>         if (cmd->free_head < 0)
>                 goto out;
>
>         context = &cmd->context[cmd->free_head];
>
>         ... /* update free_head */
>
> out:
>         spin_unlock(&cmd->context_lock);
>
>         return context;
> }
> ...
>
> static struct hns_roce_cmd_context *hns_roce_get_context(struct hns_roce_cmdq *cmd)
> {
>         struct hns_roce_cmd_context *context;
>
>         wait_event(cmd->wq, (context = hns_roce_try_get_context(cmd)));
>
>         return context;
> }

That looks more elegant. Didn't think of that, Thank you Arnd.:)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web