| Message ID | 1583129716-28382-3-git-send-email-xuzaibo@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: hisilicon - Improve SEC performance | expand |
On Mon, 2 Mar 2020 14:15:13 +0800 Zaibo Xu <xuzaibo@huawei.com> wrote: > From: yekai13 <yekai13@huawei.com> > > Allocate one workqueue for each QM instead of one for all QMs, > we found the throughput of SEC engine can be increased to > the hardware limit throughput during testing sec2 performance. > so we added this scheme. > > Signed-off-by: yekai13 <yekai13@huawei.com> > Signed-off-by: liulongfang <liulongfang@huawei.com> That first sign off needs fixing. Needs to be a real name. Also missing xuzaibo's sign offf. A question inline that might be worth a follow up patch. With signoffs fixed Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Thanks, Jonathan > --- > drivers/crypto/hisilicon/sec2/sec_main.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/drivers/crypto/hisilicon/sec2/sec_main.c b/drivers/crypto/hisilicon/sec2/sec_main.c > index 3767fdb..ebafc1c 100644 > --- a/drivers/crypto/hisilicon/sec2/sec_main.c > +++ b/drivers/crypto/hisilicon/sec2/sec_main.c > @@ -774,12 +774,24 @@ static void sec_qm_uninit(struct hisi_qm *qm) > > static int sec_probe_init(struct hisi_qm *qm, struct sec_dev *sec) > { > + int ret; > + > + qm->wq = alloc_workqueue("%s", WQ_HIGHPRI | WQ_CPU_INTENSIVE | > + WQ_MEM_RECLAIM | WQ_UNBOUND, num_online_cpus(), > + pci_name(qm->pdev)); I appreciate that you have the same parameters here as were originally in qm.c, but I would like to fully understand why some of these flags are set. Perhaps a comment for each of them? I'm not sure I'd consider the work to be done in this work queue CPU_INTENSIVE for example. This could be a follow up patch though as not actually related to this change. > + if (!qm->wq) { > + pci_err(qm->pdev, "fail to alloc workqueue\n"); > + return -ENOMEM; > + } > + > if (qm->fun_type == QM_HW_PF) { > qm->qp_base = SEC_PF_DEF_Q_BASE; > qm->qp_num = pf_q_num; > qm->debug.curr_qm_qp_num = pf_q_num; > > - return sec_pf_probe_init(sec); > + ret = sec_pf_probe_init(sec); > + if (ret) > + goto err_probe_uninit; > } else if (qm->fun_type == QM_HW_VF) { > /* > * have no way to get qm configure in VM in v1 hardware, > @@ -792,18 +804,26 @@ static int sec_probe_init(struct hisi_qm *qm, struct sec_dev *sec) > qm->qp_num = SEC_QUEUE_NUM_V1 - SEC_PF_DEF_Q_NUM; > } else if (qm->ver == QM_HW_V2) { > /* v2 starts to support get vft by mailbox */ > - return hisi_qm_get_vft(qm, &qm->qp_base, &qm->qp_num); > + ret = hisi_qm_get_vft(qm, &qm->qp_base, &qm->qp_num); > + if (ret) > + goto err_probe_uninit; > } > } else { > - return -ENODEV; > + ret = -ENODEV; > + goto err_probe_uninit; > } > > return 0; > +err_probe_uninit: > + destroy_workqueue(qm->wq); > + return ret; > } > > static void sec_probe_uninit(struct hisi_qm *qm) > { > hisi_qm_dev_err_uninit(qm); > + > + destroy_workqueue(qm->wq); > } > > static int sec_probe(struct pci_dev *pdev, const struct pci_device_id *id)
Hi, On 2020/3/2 19:51, Jonathan Cameron wrote: > On Mon, 2 Mar 2020 14:15:13 +0800 > Zaibo Xu <xuzaibo@huawei.com> wrote: > >> From: yekai13 <yekai13@huawei.com> >> >> Allocate one workqueue for each QM instead of one for all QMs, >> we found the throughput of SEC engine can be increased to >> the hardware limit throughput during testing sec2 performance. >> so we added this scheme. >> >> Signed-off-by: yekai13 <yekai13@huawei.com> >> Signed-off-by: liulongfang <liulongfang@huawei.com> > That first sign off needs fixing. Needs to be a real name. Okay, will fix it. > > Also missing xuzaibo's sign offf. > > A question inline that might be worth a follow up patch. > > With signoffs fixed > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Okay. > > Thanks, > > Jonathan > >> --- >> drivers/crypto/hisilicon/sec2/sec_main.c | 26 +++++++++++++++++++++++--- >> 1 file changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/crypto/hisilicon/sec2/sec_main.c b/drivers/crypto/hisilicon/sec2/sec_main.c >> index 3767fdb..ebafc1c 100644 >> --- a/drivers/crypto/hisilicon/sec2/sec_main.c >> +++ b/drivers/crypto/hisilicon/sec2/sec_main.c >> @@ -774,12 +774,24 @@ static void sec_qm_uninit(struct hisi_qm *qm) >> >> static int sec_probe_init(struct hisi_qm *qm, struct sec_dev *sec) >> { >> + int ret; >> + >> + qm->wq = alloc_workqueue("%s", WQ_HIGHPRI | WQ_CPU_INTENSIVE | >> + WQ_MEM_RECLAIM | WQ_UNBOUND, num_online_cpus(), >> + pci_name(qm->pdev)); > I appreciate that you have the same parameters here as were originally in > qm.c, but I would like to fully understand why some of these flags are set. > > Perhaps a comment for each of them? I'm not sure I'd consider the work > to be done in this work queue CPU_INTENSIVE for example. Okay. I thinks this is borrowed from the dm-crypto's workqueue flags :) > > This could be a follow up patch though as not actually related to this > change. This change is to improve the throughput as running multiple threads. As only one workqueue for all the QMs, the bottleneck is here. Thanks, Zaibo . > >> + if (!qm->wq) { >> + pci_err(qm->pdev, "fail to alloc workqueue\n"); >> + return -ENOMEM; >> + } >> + >> if (qm->fun_type == QM_HW_PF) { >> qm->qp_base = SEC_PF_DEF_Q_BASE; >> qm->qp_num = pf_q_num; >> qm->debug.curr_qm_qp_num = pf_q_num; >> >> - return sec_pf_probe_init(sec); >> + ret = sec_pf_probe_init(sec); >> + if (ret) >> + goto err_probe_uninit; >> } else if (qm->fun_type == QM_HW_VF) { >> /* >> * have no way to get qm configure in VM in v1 hardware, >> @@ -792,18 +804,26 @@ static int sec_probe_init(struct hisi_qm *qm, struct sec_dev *sec) >> qm->qp_num = SEC_QUEUE_NUM_V1 - SEC_PF_DEF_Q_NUM; >> } else if (qm->ver == QM_HW_V2) { >> /* v2 starts to support get vft by mailbox */ >> - return hisi_qm_get_vft(qm, &qm->qp_base, &qm->qp_num); >> + ret = hisi_qm_get_vft(qm, &qm->qp_base, &qm->qp_num); >> + if (ret) >> + goto err_probe_uninit; >> } >> } else { >> - return -ENODEV; >> + ret = -ENODEV; >> + goto err_probe_uninit; >> } >> >> return 0; >> +err_probe_uninit: >> + destroy_workqueue(qm->wq); >> + return ret; >> } >> >> static void sec_probe_uninit(struct hisi_qm *qm) >> { >> hisi_qm_dev_err_uninit(qm); >> + >> + destroy_workqueue(qm->wq); >> } >> >> static int sec_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > . >
diff --git a/drivers/crypto/hisilicon/sec2/sec_main.c b/drivers/crypto/hisilicon/sec2/sec_main.c index 3767fdb..ebafc1c 100644 --- a/drivers/crypto/hisilicon/sec2/sec_main.c +++ b/drivers/crypto/hisilicon/sec2/sec_main.c @@ -774,12 +774,24 @@ static void sec_qm_uninit(struct hisi_qm *qm) static int sec_probe_init(struct hisi_qm *qm, struct sec_dev *sec) { + int ret; + + qm->wq = alloc_workqueue("%s", WQ_HIGHPRI | WQ_CPU_INTENSIVE | + WQ_MEM_RECLAIM | WQ_UNBOUND, num_online_cpus(), + pci_name(qm->pdev)); + if (!qm->wq) { + pci_err(qm->pdev, "fail to alloc workqueue\n"); + return -ENOMEM; + } + if (qm->fun_type == QM_HW_PF) { qm->qp_base = SEC_PF_DEF_Q_BASE; qm->qp_num = pf_q_num; qm->debug.curr_qm_qp_num = pf_q_num; - return sec_pf_probe_init(sec); + ret = sec_pf_probe_init(sec); + if (ret) + goto err_probe_uninit; } else if (qm->fun_type == QM_HW_VF) { /* * have no way to get qm configure in VM in v1 hardware, @@ -792,18 +804,26 @@ static int sec_probe_init(struct hisi_qm *qm, struct sec_dev *sec) qm->qp_num = SEC_QUEUE_NUM_V1 - SEC_PF_DEF_Q_NUM; } else if (qm->ver == QM_HW_V2) { /* v2 starts to support get vft by mailbox */ - return hisi_qm_get_vft(qm, &qm->qp_base, &qm->qp_num); + ret = hisi_qm_get_vft(qm, &qm->qp_base, &qm->qp_num); + if (ret) + goto err_probe_uninit; } } else { - return -ENODEV; + ret = -ENODEV; + goto err_probe_uninit; } return 0; +err_probe_uninit: + destroy_workqueue(qm->wq); + return ret; } static void sec_probe_uninit(struct hisi_qm *qm) { hisi_qm_dev_err_uninit(qm); + + destroy_workqueue(qm->wq); } static int sec_probe(struct pci_dev *pdev, const struct pci_device_id *id)