RE: [PATCH v5 3/5] virtio-crypto: wait ctrl queue instead of busy polling

From: Gonglei (Arei)
Date: Fri May 06 2022 - 05:26:35 EST

Next message: yunfei.dong@xxxxxxxxxxxx: "Re: [DKIM] [PATCH v10, 00/15] media: mtk-vcodec: support for M8192 decoder"
Previous message: yunfei.dong@xxxxxxxxxxxx: "Re: [PATCH v10, 16/17] media: mediatek: vcodec: support stateless VP9 decoding"
In reply to: zhenwei pi: "[PATCH v5 3/5] virtio-crypto: wait ctrl queue instead of busy polling"
Next in thread: zhenwei pi: "[PATCH v5 2/5] virtio-crypto: use private buffer for control request"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: zhenwei pi [mailto:pizhenwei@xxxxxxxxxxxxx]
> Sent: Thursday, May 5, 2022 5:24 PM
> To: Gonglei (Arei) <arei.gonglei@xxxxxxxxxx>; mst@xxxxxxxxxx
> Cc: jasowang@xxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx;
> linux-crypto@xxxxxxxxxxxxxxx; helei.sig11@xxxxxxxxxxxxx;
> pizhenwei@xxxxxxxxxxxxx; davem@xxxxxxxxxxxxx
> Subject: [PATCH v5 3/5] virtio-crypto: wait ctrl queue instead of busy polling
>
> Originally, after submitting request into virtio crypto control queue, the guest
> side polls the result from the virt queue. This works like following:
> CPU0 CPU1 ... CPUx CPUy
> | | | |
> \ \ / /
> \--------spin_lock(&vcrypto->ctrl_lock)-------/
> |
> virtqueue add & kick
> |
> busy poll virtqueue
> |
> spin_unlock(&vcrypto->ctrl_lock)
> ...
>
> There are two problems:
> 1, The queue depth is always 1, the performance of a virtio crypto
> device gets limited. Multi user processes share a single control
> queue, and hit spin lock race from control queue. Test on Intel
> Platinum 8260, a single worker gets ~35K/s create/close session
> operations, and 8 workers get ~40K/s operations with 800% CPU
> utilization.
> 2, The control request is supposed to get handled immediately, but
> in the current implementation of QEMU(v6.2), the vCPU thread kicks
> another thread to do this work, the latency also gets unstable.
> Tracking latency of virtio_crypto_alg_akcipher_close_session in 5s:
> usecs : count distribution
> 0 -> 1 : 0 | |
> 2 -> 3 : 7 | |
> 4 -> 7 : 72 | |
> 8 -> 15 : 186485 |************************|
> 16 -> 31 : 687 | |
> 32 -> 63 : 5 | |
> 64 -> 127 : 3 | |
> 128 -> 255 : 1 | |
> 256 -> 511 : 0 | |
> 512 -> 1023 : 0 | |
> 1024 -> 2047 : 0 | |
> 2048 -> 4095 : 0 | |
> 4096 -> 8191 : 0 | |
> 8192 -> 16383 : 2 | |
> This means that a CPU may hold vcrypto->ctrl_lock as long as 8192~16383us.
>
> To improve the performance of control queue, a request on control queue waits
> completion instead of busy polling to reduce lock racing, and gets completed by
> control queue callback.
> CPU0 CPU1 ... CPUx CPUy
> | | | |
> \ \ / /
> \--------spin_lock(&vcrypto->ctrl_lock)-------/
> |
> virtqueue add & kick
> |
> ---------spin_unlock(&vcrypto->ctrl_lock)------
> / / \ \
> | | | |
> wait wait wait wait
>
> Test this patch, the guest side get ~200K/s operations with 300% CPU
> utilization.
>
> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Cc: Jason Wang <jasowang@xxxxxxxxxx>
> Cc: Gonglei <arei.gonglei@xxxxxxxxxx>
> Signed-off-by: zhenwei pi <pizhenwei@xxxxxxxxxxxxx>
> ---
> .../virtio/virtio_crypto_akcipher_algs.c | 29 ++---------
> drivers/crypto/virtio/virtio_crypto_common.h | 4 ++
> drivers/crypto/virtio/virtio_crypto_core.c | 52 ++++++++++++++++++-
> .../virtio/virtio_crypto_skcipher_algs.c | 34 ++----------
> 4 files changed, 64 insertions(+), 55 deletions(-)
>

Reviewed-by: Gonglei <arei.gonglei@xxxxxxxxxx>

Regards,
-Gonglei

> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> index 698ea57e2649..382ccec9ab12 100644
> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -103,7 +103,6 @@ static int
> virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
> struct scatterlist outhdr_sg, key_sg, inhdr_sg, *sgs[3];
> struct virtio_crypto *vcrypto = ctx->vcrypto;
> uint8_t *pkey;
> - unsigned int inlen;
> int err;
> unsigned int num_out = 0, num_in = 0;
> struct virtio_crypto_op_ctrl_req *ctrl; @@ -135,18 +134,9 @@ static int
> virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
> sg_init_one(&inhdr_sg, input, sizeof(*input));
> sgs[num_out + num_in++] = &inhdr_sg;
>
> - spin_lock(&vcrypto->ctrl_lock);
> - err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto,
> GFP_ATOMIC);
> - if (err < 0) {
> - spin_unlock(&vcrypto->ctrl_lock);
> + err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, num_in,
> vc_ctrl_req);
> + if (err < 0)
> goto out;
> - }
> -
> - virtqueue_kick(vcrypto->ctrl_vq);
> - while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) &&
> - !virtqueue_is_broken(vcrypto->ctrl_vq))
> - cpu_relax();
> - spin_unlock(&vcrypto->ctrl_lock);
>
> if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
> pr_err("virtio_crypto: Create session failed status: %u\n", @@
> -171,7 +161,7 @@ static int virtio_crypto_alg_akcipher_close_session(struct
> virtio_crypto_akciphe
> struct scatterlist outhdr_sg, inhdr_sg, *sgs[2];
> struct virtio_crypto_destroy_session_req *destroy_session;
> struct virtio_crypto *vcrypto = ctx->vcrypto;
> - unsigned int num_out = 0, num_in = 0, inlen;
> + unsigned int num_out = 0, num_in = 0;
> int err;
> struct virtio_crypto_op_ctrl_req *ctrl;
> struct virtio_crypto_inhdr *ctrl_status; @@ -199,18 +189,9 @@ static int
> virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
> sg_init_one(&inhdr_sg, &ctrl_status->status, sizeof(ctrl_status->status));
> sgs[num_out + num_in++] = &inhdr_sg;
>
> - spin_lock(&vcrypto->ctrl_lock);
> - err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto,
> GFP_ATOMIC);
> - if (err < 0) {
> - spin_unlock(&vcrypto->ctrl_lock);
> + err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, num_in,
> vc_ctrl_req);
> + if (err < 0)
> goto out;
> - }
> -
> - virtqueue_kick(vcrypto->ctrl_vq);
> - while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) &&
> - !virtqueue_is_broken(vcrypto->ctrl_vq))
> - cpu_relax();
> - spin_unlock(&vcrypto->ctrl_lock);
>
> if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
> pr_err("virtio_crypto: Close session failed status: %u, session_id:
> 0x%llx\n", diff --git a/drivers/crypto/virtio/virtio_crypto_common.h
> b/drivers/crypto/virtio/virtio_crypto_common.h
> index 2422237ec4e6..59a4c0259456 100644
> --- a/drivers/crypto/virtio/virtio_crypto_common.h
> +++ b/drivers/crypto/virtio/virtio_crypto_common.h
> @@ -90,6 +90,7 @@ struct virtio_crypto_ctrl_request {
> struct virtio_crypto_op_ctrl_req ctrl;
> struct virtio_crypto_session_input input;
> struct virtio_crypto_inhdr ctrl_status;
> + struct completion compl;
> };
>
> struct virtio_crypto_request;
> @@ -141,5 +142,8 @@ int virtio_crypto_skcipher_algs_register(struct
> virtio_crypto *vcrypto); void virtio_crypto_skcipher_algs_unregister(struct
> virtio_crypto *vcrypto); int virtio_crypto_akcipher_algs_register(struct
> virtio_crypto *vcrypto); void virtio_crypto_akcipher_algs_unregister(struct
> virtio_crypto *vcrypto);
> +int virtio_crypto_ctrl_vq_request(struct virtio_crypto *vcrypto, struct
> scatterlist *sgs[],
> + unsigned int out_sgs, unsigned int in_sgs,
> + struct virtio_crypto_ctrl_request *vc_ctrl_req);
>
> #endif /* _VIRTIO_CRYPTO_COMMON_H */
> diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
> b/drivers/crypto/virtio/virtio_crypto_core.c
> index c6f482db0bc0..60490ffa3df1 100644
> --- a/drivers/crypto/virtio/virtio_crypto_core.c
> +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> @@ -22,6 +22,56 @@ virtcrypto_clear_request(struct virtio_crypto_request
> *vc_req)
> }
> }
>
> +static void virtio_crypto_ctrlq_callback(struct
> +virtio_crypto_ctrl_request *vc_ctrl_req) {
> + complete(&vc_ctrl_req->compl);
> +}
> +
> +static void virtcrypto_ctrlq_callback(struct virtqueue *vq) {
> + struct virtio_crypto *vcrypto = vq->vdev->priv;
> + struct virtio_crypto_ctrl_request *vc_ctrl_req;
> + unsigned long flags;
> + unsigned int len;
> +
> + spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
> + do {
> + virtqueue_disable_cb(vq);
> + while ((vc_ctrl_req = virtqueue_get_buf(vq, &len)) != NULL) {
> + spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
> + virtio_crypto_ctrlq_callback(vc_ctrl_req);
> + spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
> + }
> + if (unlikely(virtqueue_is_broken(vq)))
> + break;
> + } while (!virtqueue_enable_cb(vq));
> + spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags); }
> +
> +int virtio_crypto_ctrl_vq_request(struct virtio_crypto *vcrypto, struct
> scatterlist *sgs[],
> + unsigned int out_sgs, unsigned int in_sgs,
> + struct virtio_crypto_ctrl_request *vc_ctrl_req) {
> + int err;
> + unsigned long flags;
> +
> + init_completion(&vc_ctrl_req->compl);
> +
> + spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
> + err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, out_sgs, in_sgs,
> vc_ctrl_req, GFP_ATOMIC);
> + if (err < 0) {
> + spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
> + return err;
> + }
> +
> + virtqueue_kick(vcrypto->ctrl_vq);
> + spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
> +
> + wait_for_completion(&vc_ctrl_req->compl);
> +
> + return 0;
> +}
> +
> static void virtcrypto_dataq_callback(struct virtqueue *vq) {
> struct virtio_crypto *vcrypto = vq->vdev->priv; @@ -73,7 +123,7 @@
> static int virtcrypto_find_vqs(struct virtio_crypto *vi)
> goto err_names;
>
> /* Parameters for control virtqueue */
> - callbacks[total_vqs - 1] = NULL;
> + callbacks[total_vqs - 1] = virtcrypto_ctrlq_callback;
> names[total_vqs - 1] = "controlq";
>
> /* Allocate/initialize parameters for data virtqueues */ diff --git
> a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> index 6aaf0869b211..e553ccadbcbc 100644
> --- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> @@ -118,7 +118,6 @@ static int virtio_crypto_alg_skcipher_init_session(
> int encrypt)
> {
> struct scatterlist outhdr, key_sg, inhdr, *sgs[3];
> - unsigned int tmp;
> struct virtio_crypto *vcrypto = ctx->vcrypto;
> int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT :
> VIRTIO_CRYPTO_OP_DECRYPT;
> int err;
> @@ -170,23 +169,9 @@ static int virtio_crypto_alg_skcipher_init_session(
> sg_init_one(&inhdr, input, sizeof(*input));
> sgs[num_out + num_in++] = &inhdr;
>
> - spin_lock(&vcrypto->ctrl_lock);
> - err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
> - num_in, vcrypto, GFP_ATOMIC);
> - if (err < 0) {
> - spin_unlock(&vcrypto->ctrl_lock);
> + err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, num_in,
> vc_ctrl_req);
> + if (err < 0)
> goto out;
> - }
> - virtqueue_kick(vcrypto->ctrl_vq);
> -
> - /*
> - * Trapping into the hypervisor, so the request should be
> - * handled immediately.
> - */
> - while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) &&
> - !virtqueue_is_broken(vcrypto->ctrl_vq))
> - cpu_relax();
> - spin_unlock(&vcrypto->ctrl_lock);
>
> if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
> pr_err("virtio_crypto: Create session failed status: %u\n", @@
> -212,7 +197,6 @@ static int virtio_crypto_alg_skcipher_close_session(
> int encrypt)
> {
> struct scatterlist outhdr, status_sg, *sgs[2];
> - unsigned int tmp;
> struct virtio_crypto_destroy_session_req *destroy_session;
> struct virtio_crypto *vcrypto = ctx->vcrypto;
> int err;
> @@ -247,19 +231,9 @@ static int virtio_crypto_alg_skcipher_close_session(
> sg_init_one(&status_sg, &ctrl_status->status,
> sizeof(ctrl_status->status));
> sgs[num_out + num_in++] = &status_sg;
>
> - spin_lock(&vcrypto->ctrl_lock);
> - err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
> - num_in, vcrypto, GFP_ATOMIC);
> - if (err < 0) {
> - spin_unlock(&vcrypto->ctrl_lock);
> + err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, num_in,
> vc_ctrl_req);
> + if (err < 0)
> goto out;
> - }
> - virtqueue_kick(vcrypto->ctrl_vq);
> -
> - while (!virtqueue_get_buf(vcrypto->ctrl_vq, &tmp) &&
> - !virtqueue_is_broken(vcrypto->ctrl_vq))
> - cpu_relax();
> - spin_unlock(&vcrypto->ctrl_lock);
>
> if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
> pr_err("virtio_crypto: Close session failed status: %u, session_id:
> 0x%llx\n",
> --
> 2.20.1

Next message: yunfei.dong@xxxxxxxxxxxx: "Re: [DKIM] [PATCH v10, 00/15] media: mtk-vcodec: support for M8192 decoder"
Previous message: yunfei.dong@xxxxxxxxxxxx: "Re: [PATCH v10, 16/17] media: mediatek: vcodec: support stateless VP9 decoding"
In reply to: zhenwei pi: "[PATCH v5 3/5] virtio-crypto: wait ctrl queue instead of busy polling"
Next in thread: zhenwei pi: "[PATCH v5 2/5] virtio-crypto: use private buffer for control request"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]