| Message ID | 20220424104140.44841-1-pizhenwei@bytedance.com |
|---|---|
| Headers | show |
| Series | virtio-crypto: Improve performance | expand |
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 >
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 >> >
> -----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
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
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!
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(-)