| Message ID | 1617262341-37571-2-git-send-email-liweihang@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/hns: Updates of CMDQ | expand |
On Thu, Apr 01, 2021 at 03:32:19PM +0800, Weihang Li wrote: > From: Lang Cheng <chenglang@huawei.com> > > Fix error of cmd's context number calculation algorithm to enable all of > 32 cmd entries and support 32 concurrent accesses. > > Signed-off-by: Lang Cheng <chenglang@huawei.com> > Signed-off-by: Weihang Li <liweihang@huawei.com> > --- > drivers/infiniband/hw/hns/hns_roce_cmd.c | 62 ++++++++++++----------------- > drivers/infiniband/hw/hns/hns_roce_device.h | 6 +-- > 2 files changed, 27 insertions(+), 41 deletions(-) <...> > - WARN_ON(cmd->free_head < 0); > - context = &cmd->context[cmd->free_head]; > - context->token += cmd->token_mask + 1; > - cmd->free_head = context->next; > + > + do { > + context = &cmd->context[cmd->free_head]; > + cmd->free_head = context->next; > + } while (context->busy); > + > + context->busy = 1; This "busy" flag after do-while together with release in __hns_roce_cmd_mbox_wait() is interesting thing. Are you sure that it won't loop forever here? Thanks
On 2021/4/1 21:08, Leon Romanovsky wrote: > On Thu, Apr 01, 2021 at 03:32:19PM +0800, Weihang Li wrote: >> From: Lang Cheng <chenglang@huawei.com> >> >> Fix error of cmd's context number calculation algorithm to enable all of >> 32 cmd entries and support 32 concurrent accesses. >> >> Signed-off-by: Lang Cheng <chenglang@huawei.com> >> Signed-off-by: Weihang Li <liweihang@huawei.com> >> --- >> drivers/infiniband/hw/hns/hns_roce_cmd.c | 62 ++++++++++++----------------- >> drivers/infiniband/hw/hns/hns_roce_device.h | 6 +-- >> 2 files changed, 27 insertions(+), 41 deletions(-) > > <...> > >> - WARN_ON(cmd->free_head < 0); >> - context = &cmd->context[cmd->free_head]; >> - context->token += cmd->token_mask + 1; >> - cmd->free_head = context->next; >> + >> + do { >> + context = &cmd->context[cmd->free_head]; >> + cmd->free_head = context->next; >> + } while (context->busy); >> + >> + context->busy = 1; > > This "busy" flag after do-while together with release in __hns_roce_cmd_mbox_wait() > is interesting thing. Are you sure that it won't loop forever here? > When initializing resources in hns_roce_cmd_use_events(), ensure that the number of semaphores is consistent with the depth of context[]. int hns_roce_cmd_use_events( ) { hr_cmd->context = kcalloc(hr_cmd->max_cmds, ...); sema_init(&hr_cmd->event_sem, hr_cmd->max_cmds); } Then, when someone gets the event_sem in hns_roce_cmd_mbox_wait(), it means that there must be a not busy context. Thanks. > Thanks > . >
On Fri, Apr 02, 2021 at 09:26:38AM +0800, Lang Cheng wrote: > > > On 2021/4/1 21:08, Leon Romanovsky wrote: > > On Thu, Apr 01, 2021 at 03:32:19PM +0800, Weihang Li wrote: > > > From: Lang Cheng <chenglang@huawei.com> > > > > > > Fix error of cmd's context number calculation algorithm to enable all of > > > 32 cmd entries and support 32 concurrent accesses. > > > > > > Signed-off-by: Lang Cheng <chenglang@huawei.com> > > > Signed-off-by: Weihang Li <liweihang@huawei.com> > > > --- > > > drivers/infiniband/hw/hns/hns_roce_cmd.c | 62 ++++++++++++----------------- > > > drivers/infiniband/hw/hns/hns_roce_device.h | 6 +-- > > > 2 files changed, 27 insertions(+), 41 deletions(-) > > > > <...> > > > > > - WARN_ON(cmd->free_head < 0); > > > - context = &cmd->context[cmd->free_head]; > > > - context->token += cmd->token_mask + 1; > > > - cmd->free_head = context->next; > > > + > > > + do { > > > + context = &cmd->context[cmd->free_head]; > > > + cmd->free_head = context->next; > > > + } while (context->busy); > > > + > > > + context->busy = 1; > > > > This "busy" flag after do-while together with release in __hns_roce_cmd_mbox_wait() > > is interesting thing. Are you sure that it won't loop forever here? > > > > When initializing resources in hns_roce_cmd_use_events(), ensure that the > number of semaphores is consistent with the depth of context[]. > > int hns_roce_cmd_use_events( ) > { > hr_cmd->context = kcalloc(hr_cmd->max_cmds, ...); > sema_init(&hr_cmd->event_sem, hr_cmd->max_cmds); > } > > Then, when someone gets the event_sem in hns_roce_cmd_mbox_wait(), it means > that there must be a not busy context. OK, thanks > > Thanks. > > > Thanks > > . > > > null
diff --git a/drivers/infiniband/hw/hns/hns_roce_cmd.c b/drivers/infiniband/hw/hns/hns_roce_cmd.c index 339e3fd..46900d0 100644 --- a/drivers/infiniband/hw/hns/hns_roce_cmd.c +++ b/drivers/infiniband/hw/hns/hns_roce_cmd.c @@ -38,22 +38,14 @@ #define CMD_POLL_TOKEN 0xffff #define CMD_MAX_NUM 32 -#define CMD_TOKEN_MASK 0x1f static int hns_roce_cmd_mbox_post_hw(struct hns_roce_dev *hr_dev, u64 in_param, u64 out_param, u32 in_modifier, u8 op_modifier, u16 op, u16 token, int event) { - struct hns_roce_cmdq *cmd = &hr_dev->cmd; - int ret; - - mutex_lock(&cmd->hcr_mutex); - ret = hr_dev->hw->post_mbox(hr_dev, in_param, out_param, in_modifier, - op_modifier, op, token, event); - mutex_unlock(&cmd->hcr_mutex); - - return ret; + return hr_dev->hw->post_mbox(hr_dev, in_param, out_param, in_modifier, + op_modifier, op, token, event); } /* this should be called with "poll_sem" */ @@ -96,15 +88,18 @@ void hns_roce_cmd_event(struct hns_roce_dev *hr_dev, u16 token, u8 status, struct hns_roce_cmd_context *context = &hr_dev->cmd.context[token % hr_dev->cmd.max_cmds]; - if (token != context->token) + if (unlikely(token != context->token)) { + dev_err_ratelimited(hr_dev->dev, + "[cmd] invalid ae token %x,context token is %x!\n", + token, context->token); return; + } context->result = (status == HNS_ROCE_CMD_SUCCESS) ? 0 : (-EIO); context->out_param = out_param; complete(&context->done); } -/* 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, u8 op_modifier, u16 op, @@ -116,13 +111,18 @@ static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param, int ret; spin_lock(&cmd->context_lock); - WARN_ON(cmd->free_head < 0); - context = &cmd->context[cmd->free_head]; - context->token += cmd->token_mask + 1; - cmd->free_head = context->next; + + do { + context = &cmd->context[cmd->free_head]; + cmd->free_head = context->next; + } while (context->busy); + + context->busy = 1; + context->token += cmd->max_cmds; + spin_unlock(&cmd->context_lock); - init_completion(&context->done); + reinit_completion(&context->done); ret = hns_roce_cmd_mbox_post_hw(hr_dev, in_param, out_param, in_modifier, op_modifier, op, @@ -130,30 +130,19 @@ static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param, if (ret) goto out; - /* - * It is timeout when wait_for_completion_timeout return 0 - * The return value is the time limit set in advance - * how many seconds showing - */ if (!wait_for_completion_timeout(&context->done, msecs_to_jiffies(timeout))) { - dev_err(dev, "[cmd]wait_for_completion_timeout timeout\n"); + dev_err(dev, "[cmd] token %x timeout, drop.\n", context->token); ret = -EBUSY; goto out; } ret = context->result; - if (ret) { - dev_err(dev, "[cmd]event mod cmd process error!err=%d\n", ret); - goto out; - } + if (ret) + dev_err(dev, "[cmd] token %x error %d\n", context->token, ret); out: - spin_lock(&cmd->context_lock); - context->next = cmd->free_head; - cmd->free_head = context - cmd->context; - spin_unlock(&cmd->context_lock); - + context->busy = 0; return ret; } @@ -208,7 +197,6 @@ int hns_roce_cmd_init(struct hns_roce_dev *hr_dev) { struct device *dev = hr_dev->dev; - mutex_init(&hr_dev->cmd.hcr_mutex); sema_init(&hr_dev->cmd.poll_sem, 1); hr_dev->cmd.use_events = 0; hr_dev->cmd.max_cmds = CMD_MAX_NUM; @@ -239,16 +227,16 @@ int hns_roce_cmd_use_events(struct hns_roce_dev *hr_dev) for (i = 0; i < hr_cmd->max_cmds; ++i) { hr_cmd->context[i].token = i; hr_cmd->context[i].next = i + 1; + init_completion(&hr_cmd->context[i].done); } - - hr_cmd->context[hr_cmd->max_cmds - 1].next = -1; + hr_cmd->context[hr_cmd->max_cmds - 1].next = 0; hr_cmd->free_head = 0; sema_init(&hr_cmd->event_sem, hr_cmd->max_cmds); spin_lock_init(&hr_cmd->context_lock); - hr_cmd->token_mask = CMD_TOKEN_MASK; hr_cmd->use_events = 1; + down(&hr_cmd->poll_sem); return 0; } @@ -259,6 +247,8 @@ void hns_roce_cmd_use_polling(struct hns_roce_dev *hr_dev) kfree(hr_cmd->context); hr_cmd->use_events = 0; + + up(&hr_cmd->poll_sem); } 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 eb2ccb8..9c20c3a 100644 --- a/drivers/infiniband/hw/hns/hns_roce_device.h +++ b/drivers/infiniband/hw/hns/hns_roce_device.h @@ -556,6 +556,7 @@ struct hns_roce_cmd_context { int next; u64 out_param; u16 token; + u16 busy; }; struct hns_roce_cmdq { @@ -572,11 +573,6 @@ struct hns_roce_cmdq { int free_head; struct hns_roce_cmd_context *context; /* - * Result of get integer part - * which max_comds compute according a power of 2 - */ - u16 token_mask; - /* * Process whether use event mode, init default non-zero * After the event queue of cmd event ready, * can switch into event mode