[PATCH v4 0/5] virtio-crypto: Improve performance

[PATCH v4 0/5] virtio-crypto: Improve performance

[zhenwei pi]

Hi, Lei
I'd like to move helper and callback functions(Eg, virtcrypto_clear_request
 and virtcrypto_ctrlq_callback) from xx_core.c to xx_common.c,
then the xx_core.c supports:
  - probe/remove/irq affinity seting for a virtio device
  - basic virtio related operations

xx_common.c supports:
  - common helpers/functions for algos

Do you have any suggestion about this?

v3 -> v4:
 - Don't create new file virtio_common.c, the new functions are added
   into virtio_crypto_core.c
 - Split the first patch into two parts:
     1, change code style,
     2, use private buffer instead of shared buffer
 - Remove relevant change.
 - Other minor changes.

v2 -> v3:
 - Jason suggested that spliting the first patch into two part:
     1, using private buffer
     2, remove the busy polling
   Rework as Jason's suggestion, this makes the smaller change in
   each one and clear.

v1 -> v2:
 - Use kfree instead of kfree_sensitive for insensitive buffer.
 - Several coding style fix.
 - Use memory from current node, instead of memory close to device
 - Add more message in commit, also explain why removing per-device
   request buffer.
 - Add necessary comment in code to explain why using kzalloc to
   allocate struct virtio_crypto_ctrl_request.

v1:
The main point of this series is to improve the performance for
virtio crypto:
- Use wait mechanism instead of busy polling for ctrl queue, this
  reduces CPU and lock racing, it's possiable to create/destroy session
  parallelly, QPS increases from ~40K/s to ~200K/s.
- Enable retry on crypto engine to improve performance for data queue,
  this allows the larger depth instead of 1.
- Fix dst data length in akcipher service.
- Other style fix.

lei he (2):
  virtio-crypto: adjust dst_len at ops callback
  virtio-crypto: enable retry for virtio-crypto-dev

zhenwei pi (3):
  virtio-crypto: change code style
  virtio-crypto: use private buffer for control request
  virtio-crypto: wait ctrl queue instead of busy polling

 .../virtio/virtio_crypto_akcipher_algs.c      |  83 ++++++-----
 drivers/crypto/virtio/virtio_crypto_common.h  |  21 ++-
 drivers/crypto/virtio/virtio_crypto_core.c    |  55 ++++++-
 .../virtio/virtio_crypto_skcipher_algs.c      | 140 ++++++++----------
 4 files changed, 180 insertions(+), 119 deletions(-)

-- 
2.20.1

[PATCH v4 1/5] virtio-crypto: change code style

[zhenwei pi]

Use temporary variable to make code easy to read and maintain.
	/* Pad cipher's parameters */
        vcrypto->ctrl.u.sym_create_session.op_type =
                cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
        vcrypto->ctrl.u.sym_create_session.u.cipher.para.algo =
                vcrypto->ctrl.header.algo;
        vcrypto->ctrl.u.sym_create_session.u.cipher.para.keylen =
                cpu_to_le32(keylen);
        vcrypto->ctrl.u.sym_create_session.u.cipher.para.op =
                cpu_to_le32(op);
-->
	sym_create_session = &ctrl->u.sym_create_session;
	sym_create_session->op_type = cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
	sym_create_session->u.cipher.para.algo = ctrl->header.algo;
	sym_create_session->u.cipher.para.keylen = cpu_to_le32(keylen);
	sym_create_session->u.cipher.para.op = cpu_to_le32(op);

The new style shows more obviously:
- the variable we want to operate.
- an assignment statement in a single line.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 .../virtio/virtio_crypto_akcipher_algs.c      | 40 ++++++-----
 .../virtio/virtio_crypto_skcipher_algs.c      | 72 +++++++++----------
 2 files changed, 59 insertions(+), 53 deletions(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index f3ec9420215e..20901a263fc8 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -106,23 +106,27 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
 	unsigned int inlen;
 	int err;
 	unsigned int num_out = 0, num_in = 0;
+	struct virtio_crypto_op_ctrl_req *ctrl;
+	struct virtio_crypto_session_input *input;
 
 	pkey = kmemdup(key, keylen, GFP_ATOMIC);
 	if (!pkey)
 		return -ENOMEM;
 
 	spin_lock(&vcrypto->ctrl_lock);
-	memcpy(&vcrypto->ctrl.header, header, sizeof(vcrypto->ctrl.header));
-	memcpy(&vcrypto->ctrl.u, para, sizeof(vcrypto->ctrl.u));
-	vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
+	ctrl = &vcrypto->ctrl;
+	memcpy(&ctrl->header, header, sizeof(ctrl->header));
+	memcpy(&ctrl->u, para, sizeof(ctrl->u));
+	input = &vcrypto->input;
+	input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
 
-	sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
+	sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
 	sgs[num_out++] = &outhdr_sg;
 
 	sg_init_one(&key_sg, pkey, keylen);
 	sgs[num_out++] = &key_sg;
 
-	sg_init_one(&inhdr_sg, &vcrypto->input, sizeof(vcrypto->input));
+	sg_init_one(&inhdr_sg, input, sizeof(*input));
 	sgs[num_out + num_in++] = &inhdr_sg;
 
 	err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto, GFP_ATOMIC);
@@ -134,12 +138,12 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
 	       !virtqueue_is_broken(vcrypto->ctrl_vq))
 		cpu_relax();
 
-	if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) {
+	if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
 		err = -EINVAL;
 		goto out;
 	}
 
-	ctx->session_id = le64_to_cpu(vcrypto->input.session_id);
+	ctx->session_id = le64_to_cpu(input->session_id);
 	ctx->session_valid = true;
 	err = 0;
 
@@ -149,7 +153,7 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
 
 	if (err < 0)
 		pr_err("virtio_crypto: Create session failed status: %u\n",
-			le32_to_cpu(vcrypto->input.status));
+			le32_to_cpu(input->status));
 
 	return err;
 }
@@ -161,23 +165,27 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
 	struct virtio_crypto *vcrypto = ctx->vcrypto;
 	unsigned int num_out = 0, num_in = 0, inlen;
 	int err;
+	struct virtio_crypto_op_ctrl_req *ctrl;
+	struct virtio_crypto_inhdr *ctrl_status;
 
 	spin_lock(&vcrypto->ctrl_lock);
 	if (!ctx->session_valid) {
 		err = 0;
 		goto out;
 	}
-	vcrypto->ctrl_status.status = VIRTIO_CRYPTO_ERR;
-	vcrypto->ctrl.header.opcode = cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
-	vcrypto->ctrl.header.queue_id = 0;
+	ctrl_status = &vcrypto->ctrl_status;
+	ctrl_status->status = VIRTIO_CRYPTO_ERR;
+	ctrl = &vcrypto->ctrl;
+	ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
+	ctrl->header.queue_id = 0;
 
-	destroy_session = &vcrypto->ctrl.u.destroy_session;
+	destroy_session = &ctrl->u.destroy_session;
 	destroy_session->session_id = cpu_to_le64(ctx->session_id);
 
-	sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
+	sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
 	sgs[num_out++] = &outhdr_sg;
 
-	sg_init_one(&inhdr_sg, &vcrypto->ctrl_status.status, sizeof(vcrypto->ctrl_status.status));
+	sg_init_one(&inhdr_sg, &ctrl_status->status, sizeof(ctrl_status->status));
 	sgs[num_out + num_in++] = &inhdr_sg;
 
 	err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto, GFP_ATOMIC);
@@ -189,7 +197,7 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
 	       !virtqueue_is_broken(vcrypto->ctrl_vq))
 		cpu_relax();
 
-	if (vcrypto->ctrl_status.status != VIRTIO_CRYPTO_OK) {
+	if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
 		err = -EINVAL;
 		goto out;
 	}
@@ -201,7 +209,7 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
 	spin_unlock(&vcrypto->ctrl_lock);
 	if (err < 0) {
 		pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
-			vcrypto->ctrl_status.status, destroy_session->session_id);
+			ctrl_status->status, destroy_session->session_id);
 	}
 
 	return err;
diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
index a618c46a52b8..e3c5bc8d6112 100644
--- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
@@ -123,6 +123,9 @@ static int virtio_crypto_alg_skcipher_init_session(
 	int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT : VIRTIO_CRYPTO_OP_DECRYPT;
 	int err;
 	unsigned int num_out = 0, num_in = 0;
+	struct virtio_crypto_op_ctrl_req *ctrl;
+	struct virtio_crypto_session_input *input;
+	struct virtio_crypto_sym_create_session_req *sym_create_session;
 
 	/*
 	 * Avoid to do DMA from the stack, switch to using
@@ -135,24 +138,22 @@ static int virtio_crypto_alg_skcipher_init_session(
 
 	spin_lock(&vcrypto->ctrl_lock);
 	/* Pad ctrl header */
-	vcrypto->ctrl.header.opcode =
-		cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION);
-	vcrypto->ctrl.header.algo = cpu_to_le32(alg);
+	ctrl = &vcrypto->ctrl;
+	ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION);
+	ctrl->header.algo = cpu_to_le32(alg);
 	/* Set the default dataqueue id to 0 */
-	vcrypto->ctrl.header.queue_id = 0;
+	ctrl->header.queue_id = 0;
 
-	vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
+	input = &vcrypto->input;
+	input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
 	/* Pad cipher's parameters */
-	vcrypto->ctrl.u.sym_create_session.op_type =
-		cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
-	vcrypto->ctrl.u.sym_create_session.u.cipher.para.algo =
-		vcrypto->ctrl.header.algo;
-	vcrypto->ctrl.u.sym_create_session.u.cipher.para.keylen =
-		cpu_to_le32(keylen);
-	vcrypto->ctrl.u.sym_create_session.u.cipher.para.op =
-		cpu_to_le32(op);
-
-	sg_init_one(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
+	sym_create_session = &ctrl->u.sym_create_session;
+	sym_create_session->op_type = cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
+	sym_create_session->u.cipher.para.algo = ctrl->header.algo;
+	sym_create_session->u.cipher.para.keylen = cpu_to_le32(keylen);
+	sym_create_session->u.cipher.para.op = cpu_to_le32(op);
+
+	sg_init_one(&outhdr, ctrl, sizeof(*ctrl));
 	sgs[num_out++] = &outhdr;
 
 	/* Set key */
@@ -160,7 +161,7 @@ static int virtio_crypto_alg_skcipher_init_session(
 	sgs[num_out++] = &key_sg;
 
 	/* Return status and session id back */
-	sg_init_one(&inhdr, &vcrypto->input, sizeof(vcrypto->input));
+	sg_init_one(&inhdr, input, sizeof(*input));
 	sgs[num_out + num_in++] = &inhdr;
 
 	err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
@@ -180,20 +181,18 @@ static int virtio_crypto_alg_skcipher_init_session(
 	       !virtqueue_is_broken(vcrypto->ctrl_vq))
 		cpu_relax();
 
-	if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) {
+	if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
 		spin_unlock(&vcrypto->ctrl_lock);
 		pr_err("virtio_crypto: Create session failed status: %u\n",
-			le32_to_cpu(vcrypto->input.status));
+			le32_to_cpu(input->status));
 		kfree_sensitive(cipher_key);
 		return -EINVAL;
 	}
 
 	if (encrypt)
-		ctx->enc_sess_info.session_id =
-			le64_to_cpu(vcrypto->input.session_id);
+		ctx->enc_sess_info.session_id = le64_to_cpu(input->session_id);
 	else
-		ctx->dec_sess_info.session_id =
-			le64_to_cpu(vcrypto->input.session_id);
+		ctx->dec_sess_info.session_id = le64_to_cpu(input->session_id);
 
 	spin_unlock(&vcrypto->ctrl_lock);
 
@@ -211,30 +210,30 @@ static int virtio_crypto_alg_skcipher_close_session(
 	struct virtio_crypto *vcrypto = ctx->vcrypto;
 	int err;
 	unsigned int num_out = 0, num_in = 0;
+	struct virtio_crypto_op_ctrl_req *ctrl;
+	struct virtio_crypto_inhdr *ctrl_status;
 
 	spin_lock(&vcrypto->ctrl_lock);
-	vcrypto->ctrl_status.status = VIRTIO_CRYPTO_ERR;
+	ctrl_status = &vcrypto->ctrl_status;
+	ctrl_status->status = VIRTIO_CRYPTO_ERR;
 	/* Pad ctrl header */
-	vcrypto->ctrl.header.opcode =
-		cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION);
+	ctrl = &vcrypto->ctrl;
+	ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION);
 	/* Set the default virtqueue id to 0 */
-	vcrypto->ctrl.header.queue_id = 0;
+	ctrl->header.queue_id = 0;
 
-	destroy_session = &vcrypto->ctrl.u.destroy_session;
+	destroy_session = &ctrl->u.destroy_session;
 
 	if (encrypt)
-		destroy_session->session_id =
-			cpu_to_le64(ctx->enc_sess_info.session_id);
+		destroy_session->session_id = cpu_to_le64(ctx->enc_sess_info.session_id);
 	else
-		destroy_session->session_id =
-			cpu_to_le64(ctx->dec_sess_info.session_id);
+		destroy_session->session_id = cpu_to_le64(ctx->dec_sess_info.session_id);
 
-	sg_init_one(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
+	sg_init_one(&outhdr, ctrl, sizeof(*ctrl));
 	sgs[num_out++] = &outhdr;
 
 	/* Return status and session id back */
-	sg_init_one(&status_sg, &vcrypto->ctrl_status.status,
-		sizeof(vcrypto->ctrl_status.status));
+	sg_init_one(&status_sg, &ctrl_status->status, sizeof(ctrl_status->status));
 	sgs[num_out + num_in++] = &status_sg;
 
 	err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
@@ -249,11 +248,10 @@ static int virtio_crypto_alg_skcipher_close_session(
 	       !virtqueue_is_broken(vcrypto->ctrl_vq))
 		cpu_relax();
 
-	if (vcrypto->ctrl_status.status != VIRTIO_CRYPTO_OK) {
+	if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
 		spin_unlock(&vcrypto->ctrl_lock);
 		pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
-			vcrypto->ctrl_status.status,
-			destroy_session->session_id);
+			ctrl_status->status, destroy_session->session_id);
 
 		return -EINVAL;
 	}
-- 
2.20.1

Re: [PATCH v4 1/5] virtio-crypto: change code style

[Jason Wang]

On Sun, Apr 24, 2022 at 6:45 PM zhenwei pi <pizhenwei@bytedance.com> wrote:
>
> Use temporary variable to make code easy to read and maintain.
>         /* Pad cipher's parameters */
>         vcrypto->ctrl.u.sym_create_session.op_type =
>                 cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
>         vcrypto->ctrl.u.sym_create_session.u.cipher.para.algo =
>                 vcrypto->ctrl.header.algo;
>         vcrypto->ctrl.u.sym_create_session.u.cipher.para.keylen =
>                 cpu_to_le32(keylen);
>         vcrypto->ctrl.u.sym_create_session.u.cipher.para.op =
>                 cpu_to_le32(op);
> -->
>         sym_create_session = &ctrl->u.sym_create_session;
>         sym_create_session->op_type = cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
>         sym_create_session->u.cipher.para.algo = ctrl->header.algo;
>         sym_create_session->u.cipher.para.keylen = cpu_to_le32(keylen);
>         sym_create_session->u.cipher.para.op = cpu_to_le32(op);
>
> The new style shows more obviously:
> - the variable we want to operate.
> - an assignment statement in a single line.

Still hundreds of lines of changes, I'd leave this change to other
mainters to dedice.

Thanks

>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  .../virtio/virtio_crypto_akcipher_algs.c      | 40 ++++++-----
>  .../virtio/virtio_crypto_skcipher_algs.c      | 72 +++++++++----------
>  2 files changed, 59 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> index f3ec9420215e..20901a263fc8 100644
> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -106,23 +106,27 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
>         unsigned int inlen;
>         int err;
>         unsigned int num_out = 0, num_in = 0;
> +       struct virtio_crypto_op_ctrl_req *ctrl;
> +       struct virtio_crypto_session_input *input;
>
>         pkey = kmemdup(key, keylen, GFP_ATOMIC);
>         if (!pkey)
>                 return -ENOMEM;
>
>         spin_lock(&vcrypto->ctrl_lock);
> -       memcpy(&vcrypto->ctrl.header, header, sizeof(vcrypto->ctrl.header));
> -       memcpy(&vcrypto->ctrl.u, para, sizeof(vcrypto->ctrl.u));
> -       vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
> +       ctrl = &vcrypto->ctrl;
> +       memcpy(&ctrl->header, header, sizeof(ctrl->header));
> +       memcpy(&ctrl->u, para, sizeof(ctrl->u));
> +       input = &vcrypto->input;
> +       input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
>
> -       sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
> +       sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
>         sgs[num_out++] = &outhdr_sg;
>
>         sg_init_one(&key_sg, pkey, keylen);
>         sgs[num_out++] = &key_sg;
>
> -       sg_init_one(&inhdr_sg, &vcrypto->input, sizeof(vcrypto->input));
> +       sg_init_one(&inhdr_sg, input, sizeof(*input));
>         sgs[num_out + num_in++] = &inhdr_sg;
>
>         err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto, GFP_ATOMIC);
> @@ -134,12 +138,12 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
>                !virtqueue_is_broken(vcrypto->ctrl_vq))
>                 cpu_relax();
>
> -       if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) {
> +       if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
>                 err = -EINVAL;
>                 goto out;
>         }
>
> -       ctx->session_id = le64_to_cpu(vcrypto->input.session_id);
> +       ctx->session_id = le64_to_cpu(input->session_id);
>         ctx->session_valid = true;
>         err = 0;
>
> @@ -149,7 +153,7 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
>
>         if (err < 0)
>                 pr_err("virtio_crypto: Create session failed status: %u\n",
> -                       le32_to_cpu(vcrypto->input.status));
> +                       le32_to_cpu(input->status));
>
>         return err;
>  }
> @@ -161,23 +165,27 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
>         struct virtio_crypto *vcrypto = ctx->vcrypto;
>         unsigned int num_out = 0, num_in = 0, inlen;
>         int err;
> +       struct virtio_crypto_op_ctrl_req *ctrl;
> +       struct virtio_crypto_inhdr *ctrl_status;
>
>         spin_lock(&vcrypto->ctrl_lock);
>         if (!ctx->session_valid) {
>                 err = 0;
>                 goto out;
>         }
> -       vcrypto->ctrl_status.status = VIRTIO_CRYPTO_ERR;
> -       vcrypto->ctrl.header.opcode = cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
> -       vcrypto->ctrl.header.queue_id = 0;
> +       ctrl_status = &vcrypto->ctrl_status;
> +       ctrl_status->status = VIRTIO_CRYPTO_ERR;
> +       ctrl = &vcrypto->ctrl;
> +       ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
> +       ctrl->header.queue_id = 0;
>
> -       destroy_session = &vcrypto->ctrl.u.destroy_session;
> +       destroy_session = &ctrl->u.destroy_session;
>         destroy_session->session_id = cpu_to_le64(ctx->session_id);
>
> -       sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
> +       sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
>         sgs[num_out++] = &outhdr_sg;
>
> -       sg_init_one(&inhdr_sg, &vcrypto->ctrl_status.status, sizeof(vcrypto->ctrl_status.status));
> +       sg_init_one(&inhdr_sg, &ctrl_status->status, sizeof(ctrl_status->status));
>         sgs[num_out + num_in++] = &inhdr_sg;
>
>         err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto, GFP_ATOMIC);
> @@ -189,7 +197,7 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
>                !virtqueue_is_broken(vcrypto->ctrl_vq))
>                 cpu_relax();
>
> -       if (vcrypto->ctrl_status.status != VIRTIO_CRYPTO_OK) {
> +       if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
>                 err = -EINVAL;
>                 goto out;
>         }
> @@ -201,7 +209,7 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
>         spin_unlock(&vcrypto->ctrl_lock);
>         if (err < 0) {
>                 pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
> -                       vcrypto->ctrl_status.status, destroy_session->session_id);
> +                       ctrl_status->status, destroy_session->session_id);
>         }
>
>         return err;
> diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> index a618c46a52b8..e3c5bc8d6112 100644
> --- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> @@ -123,6 +123,9 @@ static int virtio_crypto_alg_skcipher_init_session(
>         int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT : VIRTIO_CRYPTO_OP_DECRYPT;
>         int err;
>         unsigned int num_out = 0, num_in = 0;
> +       struct virtio_crypto_op_ctrl_req *ctrl;
> +       struct virtio_crypto_session_input *input;
> +       struct virtio_crypto_sym_create_session_req *sym_create_session;
>
>         /*
>          * Avoid to do DMA from the stack, switch to using
> @@ -135,24 +138,22 @@ static int virtio_crypto_alg_skcipher_init_session(
>
>         spin_lock(&vcrypto->ctrl_lock);
>         /* Pad ctrl header */
> -       vcrypto->ctrl.header.opcode =
> -               cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION);
> -       vcrypto->ctrl.header.algo = cpu_to_le32(alg);
> +       ctrl = &vcrypto->ctrl;
> +       ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION);
> +       ctrl->header.algo = cpu_to_le32(alg);
>         /* Set the default dataqueue id to 0 */
> -       vcrypto->ctrl.header.queue_id = 0;
> +       ctrl->header.queue_id = 0;
>
> -       vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
> +       input = &vcrypto->input;
> +       input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
>         /* Pad cipher's parameters */
> -       vcrypto->ctrl.u.sym_create_session.op_type =
> -               cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
> -       vcrypto->ctrl.u.sym_create_session.u.cipher.para.algo =
> -               vcrypto->ctrl.header.algo;
> -       vcrypto->ctrl.u.sym_create_session.u.cipher.para.keylen =
> -               cpu_to_le32(keylen);
> -       vcrypto->ctrl.u.sym_create_session.u.cipher.para.op =
> -               cpu_to_le32(op);
> -
> -       sg_init_one(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
> +       sym_create_session = &ctrl->u.sym_create_session;
> +       sym_create_session->op_type = cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
> +       sym_create_session->u.cipher.para.algo = ctrl->header.algo;
> +       sym_create_session->u.cipher.para.keylen = cpu_to_le32(keylen);
> +       sym_create_session->u.cipher.para.op = cpu_to_le32(op);
> +
> +       sg_init_one(&outhdr, ctrl, sizeof(*ctrl));
>         sgs[num_out++] = &outhdr;
>
>         /* Set key */
> @@ -160,7 +161,7 @@ static int virtio_crypto_alg_skcipher_init_session(
>         sgs[num_out++] = &key_sg;
>
>         /* Return status and session id back */
> -       sg_init_one(&inhdr, &vcrypto->input, sizeof(vcrypto->input));
> +       sg_init_one(&inhdr, input, sizeof(*input));
>         sgs[num_out + num_in++] = &inhdr;
>
>         err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
> @@ -180,20 +181,18 @@ static int virtio_crypto_alg_skcipher_init_session(
>                !virtqueue_is_broken(vcrypto->ctrl_vq))
>                 cpu_relax();
>
> -       if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) {
> +       if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
>                 spin_unlock(&vcrypto->ctrl_lock);
>                 pr_err("virtio_crypto: Create session failed status: %u\n",
> -                       le32_to_cpu(vcrypto->input.status));
> +                       le32_to_cpu(input->status));
>                 kfree_sensitive(cipher_key);
>                 return -EINVAL;
>         }
>
>         if (encrypt)
> -               ctx->enc_sess_info.session_id =
> -                       le64_to_cpu(vcrypto->input.session_id);
> +               ctx->enc_sess_info.session_id = le64_to_cpu(input->session_id);
>         else
> -               ctx->dec_sess_info.session_id =
> -                       le64_to_cpu(vcrypto->input.session_id);
> +               ctx->dec_sess_info.session_id = le64_to_cpu(input->session_id);
>
>         spin_unlock(&vcrypto->ctrl_lock);
>
> @@ -211,30 +210,30 @@ static int virtio_crypto_alg_skcipher_close_session(
>         struct virtio_crypto *vcrypto = ctx->vcrypto;
>         int err;
>         unsigned int num_out = 0, num_in = 0;
> +       struct virtio_crypto_op_ctrl_req *ctrl;
> +       struct virtio_crypto_inhdr *ctrl_status;
>
>         spin_lock(&vcrypto->ctrl_lock);
> -       vcrypto->ctrl_status.status = VIRTIO_CRYPTO_ERR;
> +       ctrl_status = &vcrypto->ctrl_status;
> +       ctrl_status->status = VIRTIO_CRYPTO_ERR;
>         /* Pad ctrl header */
> -       vcrypto->ctrl.header.opcode =
> -               cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION);
> +       ctrl = &vcrypto->ctrl;
> +       ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION);
>         /* Set the default virtqueue id to 0 */
> -       vcrypto->ctrl.header.queue_id = 0;
> +       ctrl->header.queue_id = 0;
>
> -       destroy_session = &vcrypto->ctrl.u.destroy_session;
> +       destroy_session = &ctrl->u.destroy_session;
>
>         if (encrypt)
> -               destroy_session->session_id =
> -                       cpu_to_le64(ctx->enc_sess_info.session_id);
> +               destroy_session->session_id = cpu_to_le64(ctx->enc_sess_info.session_id);
>         else
> -               destroy_session->session_id =
> -                       cpu_to_le64(ctx->dec_sess_info.session_id);
> +               destroy_session->session_id = cpu_to_le64(ctx->dec_sess_info.session_id);
>
> -       sg_init_one(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
> +       sg_init_one(&outhdr, ctrl, sizeof(*ctrl));
>         sgs[num_out++] = &outhdr;
>
>         /* Return status and session id back */
> -       sg_init_one(&status_sg, &vcrypto->ctrl_status.status,
> -               sizeof(vcrypto->ctrl_status.status));
> +       sg_init_one(&status_sg, &ctrl_status->status, sizeof(ctrl_status->status));
>         sgs[num_out + num_in++] = &status_sg;
>
>         err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
> @@ -249,11 +248,10 @@ static int virtio_crypto_alg_skcipher_close_session(
>                !virtqueue_is_broken(vcrypto->ctrl_vq))
>                 cpu_relax();
>
> -       if (vcrypto->ctrl_status.status != VIRTIO_CRYPTO_OK) {
> +       if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
>                 spin_unlock(&vcrypto->ctrl_lock);
>                 pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
> -                       vcrypto->ctrl_status.status,
> -                       destroy_session->session_id);
> +                       ctrl_status->status, destroy_session->session_id);
>
>                 return -EINVAL;
>         }
> --
> 2.20.1
>

Re: Re: [PATCH v4 1/5] virtio-crypto: change code style

[zhenwei pi]

On 4/26/22 14:12, Jason Wang wrote:
> On Sun, Apr 24, 2022 at 6:45 PM zhenwei pi <pizhenwei@bytedance.com> wrote:
>>
>> Use temporary variable to make code easy to read and maintain.
>>          /* Pad cipher's parameters */
>>          vcrypto->ctrl.u.sym_create_session.op_type =
>>                  cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
>>          vcrypto->ctrl.u.sym_create_session.u.cipher.para.algo =
>>                  vcrypto->ctrl.header.algo;
>>          vcrypto->ctrl.u.sym_create_session.u.cipher.para.keylen =
>>                  cpu_to_le32(keylen);
>>          vcrypto->ctrl.u.sym_create_session.u.cipher.para.op =
>>                  cpu_to_le32(op);
>> -->
>>          sym_create_session = &ctrl->u.sym_create_session;
>>          sym_create_session->op_type = cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
>>          sym_create_session->u.cipher.para.algo = ctrl->header.algo;
>>          sym_create_session->u.cipher.para.keylen = cpu_to_le32(keylen);
>>          sym_create_session->u.cipher.para.op = cpu_to_le32(op);
>>
>> The new style shows more obviously:
>> - the variable we want to operate.
>> - an assignment statement in a single line.
> 
> Still hundreds of lines of changes, I'd leave this change to other
> mainters to dedice.
> 
> Thanks
> 

Thanks to Jason!

Hi, Lei

What's your opinion?

>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Gonglei <arei.gonglei@huawei.com>
>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>> ---
>>   .../virtio/virtio_crypto_akcipher_algs.c      | 40 ++++++-----
>>   .../virtio/virtio_crypto_skcipher_algs.c      | 72 +++++++++----------
>>   2 files changed, 59 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
>> index f3ec9420215e..20901a263fc8 100644
>> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
>> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
>> @@ -106,23 +106,27 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
>>          unsigned int inlen;
>>          int err;
>>          unsigned int num_out = 0, num_in = 0;
>> +       struct virtio_crypto_op_ctrl_req *ctrl;
>> +       struct virtio_crypto_session_input *input;
>>
>>          pkey = kmemdup(key, keylen, GFP_ATOMIC);
>>          if (!pkey)
>>                  return -ENOMEM;
>>
>>          spin_lock(&vcrypto->ctrl_lock);
>> -       memcpy(&vcrypto->ctrl.header, header, sizeof(vcrypto->ctrl.header));
>> -       memcpy(&vcrypto->ctrl.u, para, sizeof(vcrypto->ctrl.u));
>> -       vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
>> +       ctrl = &vcrypto->ctrl;
>> +       memcpy(&ctrl->header, header, sizeof(ctrl->header));
>> +       memcpy(&ctrl->u, para, sizeof(ctrl->u));
>> +       input = &vcrypto->input;
>> +       input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
>>
>> -       sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
>> +       sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
>>          sgs[num_out++] = &outhdr_sg;
>>
>>          sg_init_one(&key_sg, pkey, keylen);
>>          sgs[num_out++] = &key_sg;
>>
>> -       sg_init_one(&inhdr_sg, &vcrypto->input, sizeof(vcrypto->input));
>> +       sg_init_one(&inhdr_sg, input, sizeof(*input));
>>          sgs[num_out + num_in++] = &inhdr_sg;
>>
>>          err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto, GFP_ATOMIC);
>> @@ -134,12 +138,12 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
>>                 !virtqueue_is_broken(vcrypto->ctrl_vq))
>>                  cpu_relax();
>>
>> -       if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) {
>> +       if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
>>                  err = -EINVAL;
>>                  goto out;
>>          }
>>
>> -       ctx->session_id = le64_to_cpu(vcrypto->input.session_id);
>> +       ctx->session_id = le64_to_cpu(input->session_id);
>>          ctx->session_valid = true;
>>          err = 0;
>>
>> @@ -149,7 +153,7 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
>>
>>          if (err < 0)
>>                  pr_err("virtio_crypto: Create session failed status: %u\n",
>> -                       le32_to_cpu(vcrypto->input.status));
>> +                       le32_to_cpu(input->status));
>>
>>          return err;
>>   }
>> @@ -161,23 +165,27 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
>>          struct virtio_crypto *vcrypto = ctx->vcrypto;
>>          unsigned int num_out = 0, num_in = 0, inlen;
>>          int err;
>> +       struct virtio_crypto_op_ctrl_req *ctrl;
>> +       struct virtio_crypto_inhdr *ctrl_status;
>>
>>          spin_lock(&vcrypto->ctrl_lock);
>>          if (!ctx->session_valid) {
>>                  err = 0;
>>                  goto out;
>>          }
>> -       vcrypto->ctrl_status.status = VIRTIO_CRYPTO_ERR;
>> -       vcrypto->ctrl.header.opcode = cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
>> -       vcrypto->ctrl.header.queue_id = 0;
>> +       ctrl_status = &vcrypto->ctrl_status;
>> +       ctrl_status->status = VIRTIO_CRYPTO_ERR;
>> +       ctrl = &vcrypto->ctrl;
>> +       ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
>> +       ctrl->header.queue_id = 0;
>>
>> -       destroy_session = &vcrypto->ctrl.u.destroy_session;
>> +       destroy_session = &ctrl->u.destroy_session;
>>          destroy_session->session_id = cpu_to_le64(ctx->session_id);
>>
>> -       sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
>> +       sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
>>          sgs[num_out++] = &outhdr_sg;
>>
>> -       sg_init_one(&inhdr_sg, &vcrypto->ctrl_status.status, sizeof(vcrypto->ctrl_status.status));
>> +       sg_init_one(&inhdr_sg, &ctrl_status->status, sizeof(ctrl_status->status));
>>          sgs[num_out + num_in++] = &inhdr_sg;
>>
>>          err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto, GFP_ATOMIC);
>> @@ -189,7 +197,7 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
>>                 !virtqueue_is_broken(vcrypto->ctrl_vq))
>>                  cpu_relax();
>>
>> -       if (vcrypto->ctrl_status.status != VIRTIO_CRYPTO_OK) {
>> +       if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
>>                  err = -EINVAL;
>>                  goto out;
>>          }
>> @@ -201,7 +209,7 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
>>          spin_unlock(&vcrypto->ctrl_lock);
>>          if (err < 0) {
>>                  pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
>> -                       vcrypto->ctrl_status.status, destroy_session->session_id);
>> +                       ctrl_status->status, destroy_session->session_id);
>>          }
>>
>>          return err;
>> diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
>> index a618c46a52b8..e3c5bc8d6112 100644
>> --- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
>> +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
>> @@ -123,6 +123,9 @@ static int virtio_crypto_alg_skcipher_init_session(
>>          int op = encrypt ? VIRTIO_CRYPTO_OP_ENCRYPT : VIRTIO_CRYPTO_OP_DECRYPT;
>>          int err;
>>          unsigned int num_out = 0, num_in = 0;
>> +       struct virtio_crypto_op_ctrl_req *ctrl;
>> +       struct virtio_crypto_session_input *input;
>> +       struct virtio_crypto_sym_create_session_req *sym_create_session;
>>
>>          /*
>>           * Avoid to do DMA from the stack, switch to using
>> @@ -135,24 +138,22 @@ static int virtio_crypto_alg_skcipher_init_session(
>>
>>          spin_lock(&vcrypto->ctrl_lock);
>>          /* Pad ctrl header */
>> -       vcrypto->ctrl.header.opcode =
>> -               cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION);
>> -       vcrypto->ctrl.header.algo = cpu_to_le32(alg);
>> +       ctrl = &vcrypto->ctrl;
>> +       ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION);
>> +       ctrl->header.algo = cpu_to_le32(alg);
>>          /* Set the default dataqueue id to 0 */
>> -       vcrypto->ctrl.header.queue_id = 0;
>> +       ctrl->header.queue_id = 0;
>>
>> -       vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
>> +       input = &vcrypto->input;
>> +       input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
>>          /* Pad cipher's parameters */
>> -       vcrypto->ctrl.u.sym_create_session.op_type =
>> -               cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
>> -       vcrypto->ctrl.u.sym_create_session.u.cipher.para.algo =
>> -               vcrypto->ctrl.header.algo;
>> -       vcrypto->ctrl.u.sym_create_session.u.cipher.para.keylen =
>> -               cpu_to_le32(keylen);
>> -       vcrypto->ctrl.u.sym_create_session.u.cipher.para.op =
>> -               cpu_to_le32(op);
>> -
>> -       sg_init_one(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
>> +       sym_create_session = &ctrl->u.sym_create_session;
>> +       sym_create_session->op_type = cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
>> +       sym_create_session->u.cipher.para.algo = ctrl->header.algo;
>> +       sym_create_session->u.cipher.para.keylen = cpu_to_le32(keylen);
>> +       sym_create_session->u.cipher.para.op = cpu_to_le32(op);
>> +
>> +       sg_init_one(&outhdr, ctrl, sizeof(*ctrl));
>>          sgs[num_out++] = &outhdr;
>>
>>          /* Set key */
>> @@ -160,7 +161,7 @@ static int virtio_crypto_alg_skcipher_init_session(
>>          sgs[num_out++] = &key_sg;
>>
>>          /* Return status and session id back */
>> -       sg_init_one(&inhdr, &vcrypto->input, sizeof(vcrypto->input));
>> +       sg_init_one(&inhdr, input, sizeof(*input));
>>          sgs[num_out + num_in++] = &inhdr;
>>
>>          err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
>> @@ -180,20 +181,18 @@ static int virtio_crypto_alg_skcipher_init_session(
>>                 !virtqueue_is_broken(vcrypto->ctrl_vq))
>>                  cpu_relax();
>>
>> -       if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) {
>> +       if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
>>                  spin_unlock(&vcrypto->ctrl_lock);
>>                  pr_err("virtio_crypto: Create session failed status: %u\n",
>> -                       le32_to_cpu(vcrypto->input.status));
>> +                       le32_to_cpu(input->status));
>>                  kfree_sensitive(cipher_key);
>>                  return -EINVAL;
>>          }
>>
>>          if (encrypt)
>> -               ctx->enc_sess_info.session_id =
>> -                       le64_to_cpu(vcrypto->input.session_id);
>> +               ctx->enc_sess_info.session_id = le64_to_cpu(input->session_id);
>>          else
>> -               ctx->dec_sess_info.session_id =
>> -                       le64_to_cpu(vcrypto->input.session_id);
>> +               ctx->dec_sess_info.session_id = le64_to_cpu(input->session_id);
>>
>>          spin_unlock(&vcrypto->ctrl_lock);
>>
>> @@ -211,30 +210,30 @@ static int virtio_crypto_alg_skcipher_close_session(
>>          struct virtio_crypto *vcrypto = ctx->vcrypto;
>>          int err;
>>          unsigned int num_out = 0, num_in = 0;
>> +       struct virtio_crypto_op_ctrl_req *ctrl;
>> +       struct virtio_crypto_inhdr *ctrl_status;
>>
>>          spin_lock(&vcrypto->ctrl_lock);
>> -       vcrypto->ctrl_status.status = VIRTIO_CRYPTO_ERR;
>> +       ctrl_status = &vcrypto->ctrl_status;
>> +       ctrl_status->status = VIRTIO_CRYPTO_ERR;
>>          /* Pad ctrl header */
>> -       vcrypto->ctrl.header.opcode =
>> -               cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION);
>> +       ctrl = &vcrypto->ctrl;
>> +       ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION);
>>          /* Set the default virtqueue id to 0 */
>> -       vcrypto->ctrl.header.queue_id = 0;
>> +       ctrl->header.queue_id = 0;
>>
>> -       destroy_session = &vcrypto->ctrl.u.destroy_session;
>> +       destroy_session = &ctrl->u.destroy_session;
>>
>>          if (encrypt)
>> -               destroy_session->session_id =
>> -                       cpu_to_le64(ctx->enc_sess_info.session_id);
>> +               destroy_session->session_id = cpu_to_le64(ctx->enc_sess_info.session_id);
>>          else
>> -               destroy_session->session_id =
>> -                       cpu_to_le64(ctx->dec_sess_info.session_id);
>> +               destroy_session->session_id = cpu_to_le64(ctx->dec_sess_info.session_id);
>>
>> -       sg_init_one(&outhdr, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
>> +       sg_init_one(&outhdr, ctrl, sizeof(*ctrl));
>>          sgs[num_out++] = &outhdr;
>>
>>          /* Return status and session id back */
>> -       sg_init_one(&status_sg, &vcrypto->ctrl_status.status,
>> -               sizeof(vcrypto->ctrl_status.status));
>> +       sg_init_one(&status_sg, &ctrl_status->status, sizeof(ctrl_status->status));
>>          sgs[num_out + num_in++] = &status_sg;
>>
>>          err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out,
>> @@ -249,11 +248,10 @@ static int virtio_crypto_alg_skcipher_close_session(
>>                 !virtqueue_is_broken(vcrypto->ctrl_vq))
>>                  cpu_relax();
>>
>> -       if (vcrypto->ctrl_status.status != VIRTIO_CRYPTO_OK) {
>> +       if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
>>                  spin_unlock(&vcrypto->ctrl_lock);
>>                  pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
>> -                       vcrypto->ctrl_status.status,
>> -                       destroy_session->session_id);
>> +                       ctrl_status->status, destroy_session->session_id);
>>
>>                  return -EINVAL;
>>          }
>> --
>> 2.20.1
>>
> 

-- 
zhenwei pi

[PATCH v4 2/5] virtio-crypto: use private buffer for control request

[zhenwei pi]

Originally, all of the control requests share a single buffer(
ctrl & input & ctrl_status fields in struct virtio_crypto), this
allows queue depth 1 only, the performance of control queue gets
limited by this design.

In this patch, each request allocates request buffer dynamically, and
free buffer after request, so the scope protected by ctrl_lock also
get optimized here.
It's possible to optimize control queue depth in the next step.

A necessary comment is already in code, still describe it again:
/*
 * Note: there are padding fields in request, clear them to zero before
 * sending to host to avoid to divulge any information.
 * Ex, virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48]
 */
So use kzalloc to allocate buffer of struct virtio_crypto_ctrl_request.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 .../virtio/virtio_crypto_akcipher_algs.c      | 41 +++++++++++----
 drivers/crypto/virtio/virtio_crypto_common.h  | 17 +++++--
 .../virtio/virtio_crypto_skcipher_algs.c      | 50 ++++++++++++-------
 3 files changed, 75 insertions(+), 33 deletions(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 20901a263fc8..509884e8b201 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -108,16 +108,22 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
 	unsigned int num_out = 0, num_in = 0;
 	struct virtio_crypto_op_ctrl_req *ctrl;
 	struct virtio_crypto_session_input *input;
+	struct virtio_crypto_ctrl_request *vc_ctrl_req;
 
 	pkey = kmemdup(key, keylen, GFP_ATOMIC);
 	if (!pkey)
 		return -ENOMEM;
 
-	spin_lock(&vcrypto->ctrl_lock);
-	ctrl = &vcrypto->ctrl;
+	vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
+	if (!vc_ctrl_req) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	ctrl = &vc_ctrl_req->ctrl;
 	memcpy(&ctrl->header, header, sizeof(ctrl->header));
 	memcpy(&ctrl->u, para, sizeof(ctrl->u));
-	input = &vcrypto->input;
+	input = &vc_ctrl_req->input;
 	input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
 
 	sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
@@ -129,14 +135,18 @@ 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)
+	if (err < 0) {
+		spin_unlock(&vcrypto->ctrl_lock);
 		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) {
 		err = -EINVAL;
@@ -148,7 +158,7 @@ static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher
 	err = 0;
 
 out:
-	spin_unlock(&vcrypto->ctrl_lock);
+	kfree(vc_ctrl_req);
 	kfree_sensitive(pkey);
 
 	if (err < 0)
@@ -167,15 +177,22 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
 	int err;
 	struct virtio_crypto_op_ctrl_req *ctrl;
 	struct virtio_crypto_inhdr *ctrl_status;
+	struct virtio_crypto_ctrl_request *vc_ctrl_req;
 
-	spin_lock(&vcrypto->ctrl_lock);
 	if (!ctx->session_valid) {
 		err = 0;
 		goto out;
 	}
-	ctrl_status = &vcrypto->ctrl_status;
+
+	vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
+	if (!vc_ctrl_req) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	ctrl_status = &vc_ctrl_req->ctrl_status;
 	ctrl_status->status = VIRTIO_CRYPTO_ERR;
-	ctrl = &vcrypto->ctrl;
+	ctrl = &vc_ctrl_req->ctrl;
 	ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
 	ctrl->header.queue_id = 0;
 
@@ -188,14 +205,18 @@ 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)
+	if (err < 0) {
+		spin_unlock(&vcrypto->ctrl_lock);
 		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) {
 		err = -EINVAL;
@@ -206,7 +227,7 @@ static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
 	ctx->session_valid = false;
 
 out:
-	spin_unlock(&vcrypto->ctrl_lock);
+	kfree(vc_ctrl_req);
 	if (err < 0) {
 		pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
 			ctrl_status->status, destroy_session->session_id);
diff --git a/drivers/crypto/virtio/virtio_crypto_common.h b/drivers/crypto/virtio/virtio_crypto_common.h
index e693d4ee83a6..2422237ec4e6 100644
--- a/drivers/crypto/virtio/virtio_crypto_common.h
+++ b/drivers/crypto/virtio/virtio_crypto_common.h
@@ -13,6 +13,7 @@
 #include <crypto/aead.h>
 #include <crypto/aes.h>
 #include <crypto/engine.h>
+#include <uapi/linux/virtio_crypto.h>
 
 
 /* Internal representation of a data virtqueue */
@@ -65,11 +66,6 @@ struct virtio_crypto {
 	/* Maximum size of per request */
 	u64 max_size;
 
-	/* Control VQ buffers: protected by the ctrl_lock */
-	struct virtio_crypto_op_ctrl_req ctrl;
-	struct virtio_crypto_session_input input;
-	struct virtio_crypto_inhdr ctrl_status;
-
 	unsigned long status;
 	atomic_t ref_count;
 	struct list_head list;
@@ -85,6 +81,17 @@ struct virtio_crypto_sym_session_info {
 	__u64 session_id;
 };
 
+/*
+ * Note: there are padding fields in request, clear them to zero before
+ *       sending to host to avoid to divulge any information.
+ * Ex, virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48]
+ */
+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 virtio_crypto_request;
 typedef void (*virtio_crypto_data_callback)
 		(struct virtio_crypto_request *vc_req, int len);
diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
index e3c5bc8d6112..6aaf0869b211 100644
--- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
@@ -126,6 +126,7 @@ static int virtio_crypto_alg_skcipher_init_session(
 	struct virtio_crypto_op_ctrl_req *ctrl;
 	struct virtio_crypto_session_input *input;
 	struct virtio_crypto_sym_create_session_req *sym_create_session;
+	struct virtio_crypto_ctrl_request *vc_ctrl_req;
 
 	/*
 	 * Avoid to do DMA from the stack, switch to using
@@ -136,15 +137,20 @@ static int virtio_crypto_alg_skcipher_init_session(
 	if (!cipher_key)
 		return -ENOMEM;
 
-	spin_lock(&vcrypto->ctrl_lock);
+	vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
+	if (!vc_ctrl_req) {
+		err = -ENOMEM;
+		goto out;
+	}
+
 	/* Pad ctrl header */
-	ctrl = &vcrypto->ctrl;
+	ctrl = &vc_ctrl_req->ctrl;
 	ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_CREATE_SESSION);
 	ctrl->header.algo = cpu_to_le32(alg);
 	/* Set the default dataqueue id to 0 */
 	ctrl->header.queue_id = 0;
 
-	input = &vcrypto->input;
+	input = &vc_ctrl_req->input;
 	input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
 	/* Pad cipher's parameters */
 	sym_create_session = &ctrl->u.sym_create_session;
@@ -164,12 +170,12 @@ 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);
-		kfree_sensitive(cipher_key);
-		return err;
+		goto out;
 	}
 	virtqueue_kick(vcrypto->ctrl_vq);
 
@@ -180,13 +186,13 @@ static int virtio_crypto_alg_skcipher_init_session(
 	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) {
-		spin_unlock(&vcrypto->ctrl_lock);
 		pr_err("virtio_crypto: Create session failed status: %u\n",
 			le32_to_cpu(input->status));
-		kfree_sensitive(cipher_key);
-		return -EINVAL;
+		err = -EINVAL;
+		goto out;
 	}
 
 	if (encrypt)
@@ -194,10 +200,11 @@ static int virtio_crypto_alg_skcipher_init_session(
 	else
 		ctx->dec_sess_info.session_id = le64_to_cpu(input->session_id);
 
-	spin_unlock(&vcrypto->ctrl_lock);
-
+	err = 0;
+out:
+	kfree(vc_ctrl_req);
 	kfree_sensitive(cipher_key);
-	return 0;
+	return err;
 }
 
 static int virtio_crypto_alg_skcipher_close_session(
@@ -212,12 +219,16 @@ static int virtio_crypto_alg_skcipher_close_session(
 	unsigned int num_out = 0, num_in = 0;
 	struct virtio_crypto_op_ctrl_req *ctrl;
 	struct virtio_crypto_inhdr *ctrl_status;
+	struct virtio_crypto_ctrl_request *vc_ctrl_req;
 
-	spin_lock(&vcrypto->ctrl_lock);
-	ctrl_status = &vcrypto->ctrl_status;
+	vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
+	if (!vc_ctrl_req)
+		return -ENOMEM;
+
+	ctrl_status = &vc_ctrl_req->ctrl_status;
 	ctrl_status->status = VIRTIO_CRYPTO_ERR;
 	/* Pad ctrl header */
-	ctrl = &vcrypto->ctrl;
+	ctrl = &vc_ctrl_req->ctrl;
 	ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION);
 	/* Set the default virtqueue id to 0 */
 	ctrl->header.queue_id = 0;
@@ -236,28 +247,31 @@ 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);
-		return err;
+		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) {
-		spin_unlock(&vcrypto->ctrl_lock);
 		pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
 			ctrl_status->status, destroy_session->session_id);
 
 		return -EINVAL;
 	}
-	spin_unlock(&vcrypto->ctrl_lock);
 
-	return 0;
+	err = 0;
+out:
+	kfree(vc_ctrl_req);
+	return err;
 }
 
 static int virtio_crypto_alg_skcipher_init_sessions(
-- 
2.20.1

Re: [PATCH v4 2/5] virtio-crypto: use private buffer for control request

[Dan Carpenter]

Hi zhenwei,

url:    https://github.com/intel-lab-lkp/linux/commits/zhenwei-pi/virtio-crypto-Improve-performance/20220424-184732
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20220424/202204242344.JepUMdzP-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:165 virtio_crypto_alg_akcipher_init_session() error: potentially dereferencing uninitialized 'input'.
drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:230 virtio_crypto_alg_akcipher_close_session() error: uninitialized symbol 'vc_ctrl_req'.
drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:232 virtio_crypto_alg_akcipher_close_session() error: potentially dereferencing uninitialized 'ctrl_status'.
drivers/crypto/virtio/virtio_crypto_akcipher_algs.c:232 virtio_crypto_alg_akcipher_close_session() error: potentially dereferencing uninitialized 'destroy_session'.

vim +/input +165 drivers/crypto/virtio/virtio_crypto_akcipher_algs.c

59ca6c93387d32 zhenwei pi 2022-03-02   99  static int virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher_ctx *ctx,
59ca6c93387d32 zhenwei pi 2022-03-02  100  		struct virtio_crypto_ctrl_header *header, void *para,
59ca6c93387d32 zhenwei pi 2022-03-02  101  		const uint8_t *key, unsigned int keylen)
59ca6c93387d32 zhenwei pi 2022-03-02  102  {
59ca6c93387d32 zhenwei pi 2022-03-02  103  	struct scatterlist outhdr_sg, key_sg, inhdr_sg, *sgs[3];
59ca6c93387d32 zhenwei pi 2022-03-02  104  	struct virtio_crypto *vcrypto = ctx->vcrypto;
59ca6c93387d32 zhenwei pi 2022-03-02  105  	uint8_t *pkey;
59ca6c93387d32 zhenwei pi 2022-03-02  106  	unsigned int inlen;
59ca6c93387d32 zhenwei pi 2022-03-02  107  	int err;
59ca6c93387d32 zhenwei pi 2022-03-02  108  	unsigned int num_out = 0, num_in = 0;
bb26cab9a7c25d zhenwei pi 2022-04-24  109  	struct virtio_crypto_op_ctrl_req *ctrl;
bb26cab9a7c25d zhenwei pi 2022-04-24  110  	struct virtio_crypto_session_input *input;
286da9ed04239c zhenwei pi 2022-04-24  111  	struct virtio_crypto_ctrl_request *vc_ctrl_req;
59ca6c93387d32 zhenwei pi 2022-03-02  112  
59ca6c93387d32 zhenwei pi 2022-03-02  113  	pkey = kmemdup(key, keylen, GFP_ATOMIC);
59ca6c93387d32 zhenwei pi 2022-03-02  114  	if (!pkey)
59ca6c93387d32 zhenwei pi 2022-03-02  115  		return -ENOMEM;
59ca6c93387d32 zhenwei pi 2022-03-02  116  
286da9ed04239c zhenwei pi 2022-04-24  117  	vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
286da9ed04239c zhenwei pi 2022-04-24  118  	if (!vc_ctrl_req) {
286da9ed04239c zhenwei pi 2022-04-24  119  		err = -ENOMEM;
286da9ed04239c zhenwei pi 2022-04-24  120  		goto out;
286da9ed04239c zhenwei pi 2022-04-24  121  	}
286da9ed04239c zhenwei pi 2022-04-24  122  
286da9ed04239c zhenwei pi 2022-04-24  123  	ctrl = &vc_ctrl_req->ctrl;
bb26cab9a7c25d zhenwei pi 2022-04-24  124  	memcpy(&ctrl->header, header, sizeof(ctrl->header));
bb26cab9a7c25d zhenwei pi 2022-04-24  125  	memcpy(&ctrl->u, para, sizeof(ctrl->u));
286da9ed04239c zhenwei pi 2022-04-24  126  	input = &vc_ctrl_req->input;
bb26cab9a7c25d zhenwei pi 2022-04-24  127  	input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
59ca6c93387d32 zhenwei pi 2022-03-02  128  
bb26cab9a7c25d zhenwei pi 2022-04-24  129  	sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
59ca6c93387d32 zhenwei pi 2022-03-02  130  	sgs[num_out++] = &outhdr_sg;
59ca6c93387d32 zhenwei pi 2022-03-02  131  
59ca6c93387d32 zhenwei pi 2022-03-02  132  	sg_init_one(&key_sg, pkey, keylen);
59ca6c93387d32 zhenwei pi 2022-03-02  133  	sgs[num_out++] = &key_sg;
59ca6c93387d32 zhenwei pi 2022-03-02  134  
bb26cab9a7c25d zhenwei pi 2022-04-24  135  	sg_init_one(&inhdr_sg, input, sizeof(*input));
59ca6c93387d32 zhenwei pi 2022-03-02  136  	sgs[num_out + num_in++] = &inhdr_sg;
59ca6c93387d32 zhenwei pi 2022-03-02  137  
286da9ed04239c zhenwei pi 2022-04-24  138  	spin_lock(&vcrypto->ctrl_lock);
59ca6c93387d32 zhenwei pi 2022-03-02  139  	err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto, GFP_ATOMIC);
286da9ed04239c zhenwei pi 2022-04-24  140  	if (err < 0) {
286da9ed04239c zhenwei pi 2022-04-24  141  		spin_unlock(&vcrypto->ctrl_lock);
59ca6c93387d32 zhenwei pi 2022-03-02  142  		goto out;
286da9ed04239c zhenwei pi 2022-04-24  143  	}
59ca6c93387d32 zhenwei pi 2022-03-02  144  
59ca6c93387d32 zhenwei pi 2022-03-02  145  	virtqueue_kick(vcrypto->ctrl_vq);
59ca6c93387d32 zhenwei pi 2022-03-02  146  	while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) &&
59ca6c93387d32 zhenwei pi 2022-03-02  147  	       !virtqueue_is_broken(vcrypto->ctrl_vq))
59ca6c93387d32 zhenwei pi 2022-03-02  148  		cpu_relax();
286da9ed04239c zhenwei pi 2022-04-24  149  	spin_unlock(&vcrypto->ctrl_lock);
59ca6c93387d32 zhenwei pi 2022-03-02  150  
bb26cab9a7c25d zhenwei pi 2022-04-24  151  	if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
59ca6c93387d32 zhenwei pi 2022-03-02  152  		err = -EINVAL;
59ca6c93387d32 zhenwei pi 2022-03-02  153  		goto out;
59ca6c93387d32 zhenwei pi 2022-03-02  154  	}
59ca6c93387d32 zhenwei pi 2022-03-02  155  
bb26cab9a7c25d zhenwei pi 2022-04-24  156  	ctx->session_id = le64_to_cpu(input->session_id);
59ca6c93387d32 zhenwei pi 2022-03-02  157  	ctx->session_valid = true;
59ca6c93387d32 zhenwei pi 2022-03-02  158  	err = 0;
59ca6c93387d32 zhenwei pi 2022-03-02  159  
59ca6c93387d32 zhenwei pi 2022-03-02  160  out:
286da9ed04239c zhenwei pi 2022-04-24  161  	kfree(vc_ctrl_req);
59ca6c93387d32 zhenwei pi 2022-03-02  162  	kfree_sensitive(pkey);
59ca6c93387d32 zhenwei pi 2022-03-02  163  
59ca6c93387d32 zhenwei pi 2022-03-02  164  	if (err < 0)
59ca6c93387d32 zhenwei pi 2022-03-02 @165  		pr_err("virtio_crypto: Create session failed status: %u\n",
bb26cab9a7c25d zhenwei pi 2022-04-24  166  			le32_to_cpu(input->status));

goto out is always suspicious.  "input" is not initialized.

59ca6c93387d32 zhenwei pi 2022-03-02  167  
59ca6c93387d32 zhenwei pi 2022-03-02  168  	return err;
59ca6c93387d32 zhenwei pi 2022-03-02  169  }
59ca6c93387d32 zhenwei pi 2022-03-02  170  
59ca6c93387d32 zhenwei pi 2022-03-02  171  static int virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akcipher_ctx *ctx)
59ca6c93387d32 zhenwei pi 2022-03-02  172  {
59ca6c93387d32 zhenwei pi 2022-03-02  173  	struct scatterlist outhdr_sg, inhdr_sg, *sgs[2];
59ca6c93387d32 zhenwei pi 2022-03-02  174  	struct virtio_crypto_destroy_session_req *destroy_session;
59ca6c93387d32 zhenwei pi 2022-03-02  175  	struct virtio_crypto *vcrypto = ctx->vcrypto;
59ca6c93387d32 zhenwei pi 2022-03-02  176  	unsigned int num_out = 0, num_in = 0, inlen;
59ca6c93387d32 zhenwei pi 2022-03-02  177  	int err;
bb26cab9a7c25d zhenwei pi 2022-04-24  178  	struct virtio_crypto_op_ctrl_req *ctrl;
bb26cab9a7c25d zhenwei pi 2022-04-24  179  	struct virtio_crypto_inhdr *ctrl_status;
286da9ed04239c zhenwei pi 2022-04-24  180  	struct virtio_crypto_ctrl_request *vc_ctrl_req;
59ca6c93387d32 zhenwei pi 2022-03-02  181  
59ca6c93387d32 zhenwei pi 2022-03-02  182  	if (!ctx->session_valid) {
59ca6c93387d32 zhenwei pi 2022-03-02  183  		err = 0;
59ca6c93387d32 zhenwei pi 2022-03-02  184  		goto out;
59ca6c93387d32 zhenwei pi 2022-03-02  185  	}
286da9ed04239c zhenwei pi 2022-04-24  186  
286da9ed04239c zhenwei pi 2022-04-24  187  	vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
286da9ed04239c zhenwei pi 2022-04-24  188  	if (!vc_ctrl_req) {
286da9ed04239c zhenwei pi 2022-04-24  189  		err = -ENOMEM;
286da9ed04239c zhenwei pi 2022-04-24  190  		goto out;
286da9ed04239c zhenwei pi 2022-04-24  191  	}
286da9ed04239c zhenwei pi 2022-04-24  192  
286da9ed04239c zhenwei pi 2022-04-24  193  	ctrl_status = &vc_ctrl_req->ctrl_status;
bb26cab9a7c25d zhenwei pi 2022-04-24  194  	ctrl_status->status = VIRTIO_CRYPTO_ERR;
286da9ed04239c zhenwei pi 2022-04-24  195  	ctrl = &vc_ctrl_req->ctrl;
bb26cab9a7c25d zhenwei pi 2022-04-24  196  	ctrl->header.opcode = cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
bb26cab9a7c25d zhenwei pi 2022-04-24  197  	ctrl->header.queue_id = 0;
59ca6c93387d32 zhenwei pi 2022-03-02  198  
bb26cab9a7c25d zhenwei pi 2022-04-24  199  	destroy_session = &ctrl->u.destroy_session;
59ca6c93387d32 zhenwei pi 2022-03-02  200  	destroy_session->session_id = cpu_to_le64(ctx->session_id);
59ca6c93387d32 zhenwei pi 2022-03-02  201  
bb26cab9a7c25d zhenwei pi 2022-04-24  202  	sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
59ca6c93387d32 zhenwei pi 2022-03-02  203  	sgs[num_out++] = &outhdr_sg;
59ca6c93387d32 zhenwei pi 2022-03-02  204  
bb26cab9a7c25d zhenwei pi 2022-04-24  205  	sg_init_one(&inhdr_sg, &ctrl_status->status, sizeof(ctrl_status->status));
59ca6c93387d32 zhenwei pi 2022-03-02  206  	sgs[num_out + num_in++] = &inhdr_sg;
59ca6c93387d32 zhenwei pi 2022-03-02  207  
286da9ed04239c zhenwei pi 2022-04-24  208  	spin_lock(&vcrypto->ctrl_lock);
59ca6c93387d32 zhenwei pi 2022-03-02  209  	err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in, vcrypto, GFP_ATOMIC);
286da9ed04239c zhenwei pi 2022-04-24  210  	if (err < 0) {
286da9ed04239c zhenwei pi 2022-04-24  211  		spin_unlock(&vcrypto->ctrl_lock);
59ca6c93387d32 zhenwei pi 2022-03-02  212  		goto out;
286da9ed04239c zhenwei pi 2022-04-24  213  	}
59ca6c93387d32 zhenwei pi 2022-03-02  214  
59ca6c93387d32 zhenwei pi 2022-03-02  215  	virtqueue_kick(vcrypto->ctrl_vq);
59ca6c93387d32 zhenwei pi 2022-03-02  216  	while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) &&
59ca6c93387d32 zhenwei pi 2022-03-02  217  	       !virtqueue_is_broken(vcrypto->ctrl_vq))
59ca6c93387d32 zhenwei pi 2022-03-02  218  		cpu_relax();
286da9ed04239c zhenwei pi 2022-04-24  219  	spin_unlock(&vcrypto->ctrl_lock);
59ca6c93387d32 zhenwei pi 2022-03-02  220  
bb26cab9a7c25d zhenwei pi 2022-04-24  221  	if (ctrl_status->status != VIRTIO_CRYPTO_OK) {
59ca6c93387d32 zhenwei pi 2022-03-02  222  		err = -EINVAL;
59ca6c93387d32 zhenwei pi 2022-03-02  223  		goto out;
59ca6c93387d32 zhenwei pi 2022-03-02  224  	}
59ca6c93387d32 zhenwei pi 2022-03-02  225  
59ca6c93387d32 zhenwei pi 2022-03-02  226  	err = 0;
59ca6c93387d32 zhenwei pi 2022-03-02  227  	ctx->session_valid = false;
59ca6c93387d32 zhenwei pi 2022-03-02  228  
59ca6c93387d32 zhenwei pi 2022-03-02  229  out:
286da9ed04239c zhenwei pi 2022-04-24 @230  	kfree(vc_ctrl_req);
59ca6c93387d32 zhenwei pi 2022-03-02  231  	if (err < 0) {
59ca6c93387d32 zhenwei pi 2022-03-02 @232  		pr_err("virtio_crypto: Close session failed status: %u, session_id: 0x%llx\n",
bb26cab9a7c25d zhenwei pi 2022-04-24  233  			ctrl_status->status, destroy_session->session_id);

More canonical goto out bugs.

59ca6c93387d32 zhenwei pi 2022-03-02  234  	}
59ca6c93387d32 zhenwei pi 2022-03-02  235  
59ca6c93387d32 zhenwei pi 2022-03-02  236  	return err;
59ca6c93387d32 zhenwei pi 2022-03-02  237  }

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[zhenwei pi]

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@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 .../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(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 509884e8b201..1e98502830cf 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) {
 		err = -EINVAL;
@@ -173,7 +163,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;
@@ -205,18 +195,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) {
 		err = -EINVAL;
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

[PATCH v4 4/5] virtio-crypto: adjust dst_len at ops callback

[zhenwei pi]

From: lei he <helei.sig11@bytedance.com>

For some akcipher operations(eg, decryption of pkcs1pad(rsa)),
the length of returned result maybe less than akcipher_req->dst_len,
we need to recalculate the actual dst_len through the virt-queue
protocol.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: lei he <helei.sig11@bytedance.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 1e98502830cf..1892901d2a71 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -90,9 +90,12 @@ static void virtio_crypto_dataq_akcipher_callback(struct virtio_crypto_request *
 	}
 
 	akcipher_req = vc_akcipher_req->akcipher_req;
-	if (vc_akcipher_req->opcode != VIRTIO_CRYPTO_AKCIPHER_VERIFY)
+	if (vc_akcipher_req->opcode != VIRTIO_CRYPTO_AKCIPHER_VERIFY) {
+		/* actuall length maybe less than dst buffer */
+		akcipher_req->dst_len = len - sizeof(vc_req->status);
 		sg_copy_from_buffer(akcipher_req->dst, sg_nents(akcipher_req->dst),
 				    vc_akcipher_req->dst_buf, akcipher_req->dst_len);
+	}
 	virtio_crypto_akcipher_finalize_req(vc_akcipher_req, akcipher_req, error);
 }
 
-- 
2.20.1

[PATCH v4 5/5] virtio-crypto: enable retry for virtio-crypto-dev

[zhenwei pi]

From: lei he <helei.sig11@bytedance.com>

Enable retry for virtio-crypto-dev, so that crypto-engine
can process cipher-requests parallelly.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: lei he <helei.sig11@bytedance.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 drivers/crypto/virtio/virtio_crypto_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/virtio/virtio_crypto_core.c b/drivers/crypto/virtio/virtio_crypto_core.c
index 60490ffa3df1..f67e0d4c1b0c 100644
--- a/drivers/crypto/virtio/virtio_crypto_core.c
+++ b/drivers/crypto/virtio/virtio_crypto_core.c
@@ -144,7 +144,8 @@ static int virtcrypto_find_vqs(struct virtio_crypto *vi)
 		spin_lock_init(&vi->data_vq[i].lock);
 		vi->data_vq[i].vq = vqs[i];
 		/* Initialize crypto engine */
-		vi->data_vq[i].engine = crypto_engine_alloc_init(dev, 1);
+		vi->data_vq[i].engine = crypto_engine_alloc_init_and_set(dev, true, NULL, 1,
+						virtqueue_get_vring_size(vqs[i]));
 		if (!vi->data_vq[i].engine) {
 			ret = -ENOMEM;
 			goto err_engine;
-- 
2.20.1

PING: [PATCH v4 0/5] virtio-crypto: Improve performance

[zhenwei pi]

Hi, Lei

Jason replied in another patch:
Still hundreds of lines of changes, I'd leave this change to other
maintainers to decide.

Quite frankly, the virtio crypto driver changed only a few in the past, 
and the performance of control queue is not good enough. I am in doubt 
about that this driver is not used widely. So I'd like to rework a lot, 
it would be best to complete this work in 5.18 window.

This gets different point with Jason. I would appreciate it if you could 
give me any hint.

On 4/24/22 18:41, zhenwei pi wrote:
> Hi, Lei
> I'd like to move helper and callback functions(Eg, virtcrypto_clear_request
>   and virtcrypto_ctrlq_callback) from xx_core.c to xx_common.c,
> then the xx_core.c supports:
>    - probe/remove/irq affinity seting for a virtio device
>    - basic virtio related operations
> 
> xx_common.c supports:
>    - common helpers/functions for algos
> 
> Do you have any suggestion about this?
> 
> v3 -> v4:
>   - Don't create new file virtio_common.c, the new functions are added
>     into virtio_crypto_core.c
>   - Split the first patch into two parts:
>       1, change code style,
>       2, use private buffer instead of shared buffer
>   - Remove relevant change.
>   - Other minor changes.
> 
> v2 -> v3:
>   - Jason suggested that spliting the first patch into two part:
>       1, using private buffer
>       2, remove the busy polling
>     Rework as Jason's suggestion, this makes the smaller change in
>     each one and clear.
> 
> v1 -> v2:
>   - Use kfree instead of kfree_sensitive for insensitive buffer.
>   - Several coding style fix.
>   - Use memory from current node, instead of memory close to device
>   - Add more message in commit, also explain why removing per-device
>     request buffer.
>   - Add necessary comment in code to explain why using kzalloc to
>     allocate struct virtio_crypto_ctrl_request.
> 
> v1:
> The main point of this series is to improve the performance for
> virtio crypto:
> - Use wait mechanism instead of busy polling for ctrl queue, this
>    reduces CPU and lock racing, it's possiable to create/destroy session
>    parallelly, QPS increases from ~40K/s to ~200K/s.
> - Enable retry on crypto engine to improve performance for data queue,
>    this allows the larger depth instead of 1.
> - Fix dst data length in akcipher service.
> - Other style fix.
> 
> lei he (2):
>    virtio-crypto: adjust dst_len at ops callback
>    virtio-crypto: enable retry for virtio-crypto-dev
> 
> zhenwei pi (3):
>    virtio-crypto: change code style
>    virtio-crypto: use private buffer for control request
>    virtio-crypto: wait ctrl queue instead of busy polling
> 
>   .../virtio/virtio_crypto_akcipher_algs.c      |  83 ++++++-----
>   drivers/crypto/virtio/virtio_crypto_common.h  |  21 ++-
>   drivers/crypto/virtio/virtio_crypto_core.c    |  55 ++++++-
>   .../virtio/virtio_crypto_skcipher_algs.c      | 140 ++++++++----------
>   4 files changed, 180 insertions(+), 119 deletions(-)
> 

-- 
zhenwei pi

RE: PING: [PATCH v4 0/5] virtio-crypto: Improve performance

[Gonglei (Arei)]

> -----Original Message-----
> From: zhenwei pi [mailto:pizhenwei@bytedance.com]
> Sent: Thursday, May 5, 2022 10:35 AM
> To: Gonglei (Arei) <arei.gonglei@huawei.com>; mst@redhat.com;
> jasowang@redhat.com
> Cc: herbert@gondor.apana.org.au; linux-kernel@vger.kernel.org;
> virtualization@lists.linux-foundation.org; linux-crypto@vger.kernel.org;
> helei.sig11@bytedance.com; davem@davemloft.net
> Subject: PING: [PATCH v4 0/5] virtio-crypto: Improve performance
> 
> Hi, Lei
> 
> Jason replied in another patch:
> Still hundreds of lines of changes, I'd leave this change to other maintainers to
> decide.
> 
> Quite frankly, the virtio crypto driver changed only a few in the past, and the
> performance of control queue is not good enough. I am in doubt about that this
> driver is not used widely. So I'd like to rework a lot, it would be best to complete
> this work in 5.18 window.
> 
> This gets different point with Jason. I would appreciate it if you could give me
> any hint.
> 

This is already in my todo list.

Regards,
-Gonglei

> On 4/24/22 18:41, zhenwei pi wrote:
> > Hi, Lei
> > I'd like to move helper and callback functions(Eg, virtcrypto_clear_request
> >   and virtcrypto_ctrlq_callback) from xx_core.c to xx_common.c, then
> > the xx_core.c supports:
> >    - probe/remove/irq affinity seting for a virtio device
> >    - basic virtio related operations
> >
> > xx_common.c supports:
> >    - common helpers/functions for algos
> >
> > Do you have any suggestion about this?
> >
> > v3 -> v4:
> >   - Don't create new file virtio_common.c, the new functions are added
> >     into virtio_crypto_core.c
> >   - Split the first patch into two parts:
> >       1, change code style,
> >       2, use private buffer instead of shared buffer
> >   - Remove relevant change.
> >   - Other minor changes.
> >
> > v2 -> v3:
> >   - Jason suggested that spliting the first patch into two part:
> >       1, using private buffer
> >       2, remove the busy polling
> >     Rework as Jason's suggestion, this makes the smaller change in
> >     each one and clear.
> >
> > v1 -> v2:
> >   - Use kfree instead of kfree_sensitive for insensitive buffer.
> >   - Several coding style fix.
> >   - Use memory from current node, instead of memory close to device
> >   - Add more message in commit, also explain why removing per-device
> >     request buffer.
> >   - Add necessary comment in code to explain why using kzalloc to
> >     allocate struct virtio_crypto_ctrl_request.
> >
> > v1:
> > The main point of this series is to improve the performance for virtio
> > crypto:
> > - Use wait mechanism instead of busy polling for ctrl queue, this
> >    reduces CPU and lock racing, it's possiable to create/destroy session
> >    parallelly, QPS increases from ~40K/s to ~200K/s.
> > - Enable retry on crypto engine to improve performance for data queue,
> >    this allows the larger depth instead of 1.
> > - Fix dst data length in akcipher service.
> > - Other style fix.
> >
> > lei he (2):
> >    virtio-crypto: adjust dst_len at ops callback
> >    virtio-crypto: enable retry for virtio-crypto-dev
> >
> > zhenwei pi (3):
> >    virtio-crypto: change code style
> >    virtio-crypto: use private buffer for control request
> >    virtio-crypto: wait ctrl queue instead of busy polling
> >
> >   .../virtio/virtio_crypto_akcipher_algs.c      |  83 ++++++-----
> >   drivers/crypto/virtio/virtio_crypto_common.h  |  21 ++-
> >   drivers/crypto/virtio/virtio_crypto_core.c    |  55 ++++++-
> >   .../virtio/virtio_crypto_skcipher_algs.c      | 140 ++++++++----------
> >   4 files changed, 180 insertions(+), 119 deletions(-)
> >
> 
> --
> zhenwei pi

Re: PING: [PATCH v4 0/5] virtio-crypto: Improve performance

[Michael S. Tsirkin]

On Thu, May 05, 2022 at 03:14:40AM +0000, Gonglei (Arei) wrote:
> 
> 
> > -----Original Message-----
> > From: zhenwei pi [mailto:pizhenwei@bytedance.com]
> > Sent: Thursday, May 5, 2022 10:35 AM
> > To: Gonglei (Arei) <arei.gonglei@huawei.com>; mst@redhat.com;
> > jasowang@redhat.com
> > Cc: herbert@gondor.apana.org.au; linux-kernel@vger.kernel.org;
> > virtualization@lists.linux-foundation.org; linux-crypto@vger.kernel.org;
> > helei.sig11@bytedance.com; davem@davemloft.net
> > Subject: PING: [PATCH v4 0/5] virtio-crypto: Improve performance
> > 
> > Hi, Lei
> > 
> > Jason replied in another patch:
> > Still hundreds of lines of changes, I'd leave this change to other maintainers to
> > decide.
> > 
> > Quite frankly, the virtio crypto driver changed only a few in the past, and the
> > performance of control queue is not good enough. I am in doubt about that this
> > driver is not used widely. So I'd like to rework a lot, it would be best to complete
> > this work in 5.18 window.
> > 
> > This gets different point with Jason. I would appreciate it if you could give me
> > any hint.
> > 
> 
> This is already in my todo list.
> 
> Regards,
> -Gonglei

It's been out a month though, not really acceptable latency for review.
So I would apply this for next,  but you need to address Dan Captenter's
comment, and look for simular patterns elesewhere in your patch.


> > On 4/24/22 18:41, zhenwei pi wrote:
> > > Hi, Lei
> > > I'd like to move helper and callback functions(Eg, virtcrypto_clear_request
> > >   and virtcrypto_ctrlq_callback) from xx_core.c to xx_common.c, then
> > > the xx_core.c supports:
> > >    - probe/remove/irq affinity seting for a virtio device
> > >    - basic virtio related operations
> > >
> > > xx_common.c supports:
> > >    - common helpers/functions for algos
> > >
> > > Do you have any suggestion about this?
> > >
> > > v3 -> v4:
> > >   - Don't create new file virtio_common.c, the new functions are added
> > >     into virtio_crypto_core.c
> > >   - Split the first patch into two parts:
> > >       1, change code style,
> > >       2, use private buffer instead of shared buffer
> > >   - Remove relevant change.
> > >   - Other minor changes.
> > >
> > > v2 -> v3:
> > >   - Jason suggested that spliting the first patch into two part:
> > >       1, using private buffer
> > >       2, remove the busy polling
> > >     Rework as Jason's suggestion, this makes the smaller change in
> > >     each one and clear.
> > >
> > > v1 -> v2:
> > >   - Use kfree instead of kfree_sensitive for insensitive buffer.
> > >   - Several coding style fix.
> > >   - Use memory from current node, instead of memory close to device
> > >   - Add more message in commit, also explain why removing per-device
> > >     request buffer.
> > >   - Add necessary comment in code to explain why using kzalloc to
> > >     allocate struct virtio_crypto_ctrl_request.
> > >
> > > v1:
> > > The main point of this series is to improve the performance for virtio
> > > crypto:
> > > - Use wait mechanism instead of busy polling for ctrl queue, this
> > >    reduces CPU and lock racing, it's possiable to create/destroy session
> > >    parallelly, QPS increases from ~40K/s to ~200K/s.
> > > - Enable retry on crypto engine to improve performance for data queue,
> > >    this allows the larger depth instead of 1.
> > > - Fix dst data length in akcipher service.
> > > - Other style fix.
> > >
> > > lei he (2):
> > >    virtio-crypto: adjust dst_len at ops callback
> > >    virtio-crypto: enable retry for virtio-crypto-dev
> > >
> > > zhenwei pi (3):
> > >    virtio-crypto: change code style
> > >    virtio-crypto: use private buffer for control request
> > >    virtio-crypto: wait ctrl queue instead of busy polling
> > >
> > >   .../virtio/virtio_crypto_akcipher_algs.c      |  83 ++++++-----
> > >   drivers/crypto/virtio/virtio_crypto_common.h  |  21 ++-
> > >   drivers/crypto/virtio/virtio_crypto_core.c    |  55 ++++++-
> > >   .../virtio/virtio_crypto_skcipher_algs.c      | 140 ++++++++----------
> > >   4 files changed, 180 insertions(+), 119 deletions(-)
> > >
> > 
> > --
> > zhenwei pi

Re: Re: PING: [PATCH v4 0/5] virtio-crypto: Improve performance

[zhenwei pi]

On 5/5/22 12:57, Michael S. Tsirkin wrote:
> On Thu, May 05, 2022 at 03:14:40AM +0000, Gonglei (Arei) wrote:
>>
>>
>>> -----Original Message-----
>>> From: zhenwei pi [mailto:pizhenwei@bytedance.com]
>>> Sent: Thursday, May 5, 2022 10:35 AM
>>> To: Gonglei (Arei) <arei.gonglei@huawei.com>; mst@redhat.com;
>>> jasowang@redhat.com
>>> Cc: herbert@gondor.apana.org.au; linux-kernel@vger.kernel.org;
>>> virtualization@lists.linux-foundation.org; linux-crypto@vger.kernel.org;
>>> helei.sig11@bytedance.com; davem@davemloft.net
>>> Subject: PING: [PATCH v4 0/5] virtio-crypto: Improve performance
>>>
>>> Hi, Lei
>>>
>>> Jason replied in another patch:
>>> Still hundreds of lines of changes, I'd leave this change to other maintainers to
>>> decide.
>>>
>>> Quite frankly, the virtio crypto driver changed only a few in the past, and the
>>> performance of control queue is not good enough. I am in doubt about that this
>>> driver is not used widely. So I'd like to rework a lot, it would be best to complete
>>> this work in 5.18 window.
>>>
>>> This gets different point with Jason. I would appreciate it if you could give me
>>> any hint.
>>>
>>
>> This is already in my todo list.
>>
>> Regards,
>> -Gonglei
> 
> It's been out a month though, not really acceptable latency for review.
> So I would apply this for next,  but you need to address Dan Captenter's
> comment, and look for simular patterns elesewhere in your patch.
> 

I fixed this in the v5 series. Thanks!

-- 
zhenwei pi