From: yekai13 <[email protected]>
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 <[email protected]>
Signed-off-by: liulongfang <[email protected]>
---
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));
+ 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)
--
2.8.1
On Mon, 2 Mar 2020 14:15:13 +0800
Zaibo Xu <[email protected]> wrote:
> From: yekai13 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: liulongfang <[email protected]>
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 <[email protected]>
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 <[email protected]> wrote:
>
>> From: yekai13 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: liulongfang <[email protected]>
> 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 <[email protected]>
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)
>
> .
>