RE: [PATCH v5 1/5] virtio-crypto: change code style
> -Original Message-
> From: zhenwei pi [mailto:pizhen...@bytedance.com]
> Sent: Thursday, May 5, 2022 5:24 PM
> To: Gonglei (Arei) ; m...@redhat.com
> Cc: jasow...@redhat.com; herb...@gondor.apana.org.au;
> linux-ker...@vger.kernel.org; virtualization@lists.linux-foundation.org;
> linux-cry...@vger.kernel.org; helei.si...@bytedance.com;
> pizhen...@bytedance.com; da...@davemloft.net
> Subject: [PATCH v5 1/5] virtio-crypto: change code style
>
> 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
> Cc: Jason Wang
> Cc: Gonglei
> Signed-off-by: zhenwei pi
> ---
> .../virtio/virtio_crypto_akcipher_algs.c | 40 ++-
> .../virtio/virtio_crypto_skcipher_algs.c | 72 +--
> 2 files changed, 59 insertions(+), 53 deletions(-)
>
Reviewed-by: Gonglei
Regards,
-Gonglei
> 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_CRYP
RE: [PATCH v5 2/5] virtio-crypto: use private buffer for control request
> -Original Message-
> From: zhenwei pi [mailto:pizhen...@bytedance.com]
> Sent: Thursday, May 5, 2022 5:24 PM
> To: Gonglei (Arei) ; m...@redhat.com
> Cc: jasow...@redhat.com; herb...@gondor.apana.org.au;
> linux-ker...@vger.kernel.org; virtualization@lists.linux-foundation.org;
> linux-cry...@vger.kernel.org; helei.si...@bytedance.com;
> pizhen...@bytedance.com; da...@davemloft.net; kernel test robot
> ; Dan Carpenter
> Subject: [PATCH v5 2/5] virtio-crypto: use private buffer for control request
>
> 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.
>
> Potentially dereferencing uninitialized variables:
> Reported-by: kernel test robot
> Reported-by: Dan Carpenter
>
> Cc: Michael S. Tsirkin
> Cc: Jason Wang
> Cc: Gonglei
> Signed-off-by: zhenwei pi
> ---
> .../virtio/virtio_crypto_akcipher_algs.c | 57 ---
> drivers/crypto/virtio/virtio_crypto_common.h | 17 --
> .../virtio/virtio_crypto_skcipher_algs.c | 50 ++--
> 3 files changed, 79 insertions(+), 45 deletions(-)
>
Reviewed-by: Gonglei
Regards,
-Gonglei
> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> index 20901a263fc8..698ea57e2649 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,16 +135,22 @@
> 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) {
> + pr_err("virtio_crypto: Create session failed status: %u\n",
> + le32_to_cpu(input->status));
> err = -EINVAL;
> goto out;
> }
> @@ -148,13 +160,9 @@ 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)
> - pr_err("virtio_crypto: Create session failed status: %u\n",
> - le32_to_cpu(input->status));
> -
> return err;
> }
>
> @@ -167,15 +175,18 @@ 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;
> + if (!ctx->s
RE: [PATCH v5 3/5] virtio-crypto: wait ctrl queue instead of busy polling
> -Original Message- > From: zhenwei pi [mailto:pizhen...@bytedance.com] > Sent: Thursday, May 5, 2022 5:24 PM > To: Gonglei (Arei) ; m...@redhat.com > Cc: jasow...@redhat.com; herb...@gondor.apana.org.au; > linux-ker...@vger.kernel.org; virtualization@lists.linux-foundation.org; > linux-cry...@vger.kernel.org; helei.si...@bytedance.com; > pizhen...@bytedance.com; da...@davemloft.net > Subject: [PATCH v5 3/5] virtio-crypto: wait ctrl queue instead of busy polling > > Originally, after submitting request into virtio crypto control queue, the > guest > side polls the result from the virt queue. This works like following: > CPU0 CPU1 ... CPUx CPUy > | | | | > \ \ / / > \spin_lock(&vcrypto->ctrl_lock)---/ >| > virtqueue add & kick >| > busy poll virtqueue >| > spin_unlock(&vcrypto->ctrl_lock) > ... > > There are two problems: > 1, The queue depth is always 1, the performance of a virtio crypto >device gets limited. Multi user processes share a single control >queue, and hit spin lock race from control queue. Test on Intel >Platinum 8260, a single worker gets ~35K/s create/close session >operations, and 8 workers get ~40K/s operations with 800% CPU >utilization. > 2, The control request is supposed to get handled immediately, but >in the current implementation of QEMU(v6.2), the vCPU thread kicks >another thread to do this work, the latency also gets unstable. >Tracking latency of virtio_crypto_alg_akcipher_close_session in 5s: > usecs : count distribution > 0 -> 1 : 0|| > 2 -> 3 : 7|| > 4 -> 7 : 72 || > 8 -> 15 : 186485 || > 16 -> 31 : 687 || > 32 -> 63 : 5|| > 64 -> 127: 3|| >128 -> 255: 1|| >256 -> 511: 0|| >512 -> 1023 : 0|| > 1024 -> 2047 : 0|| > 2048 -> 4095 : 0|| > 4096 -> 8191 : 0|| > 8192 -> 16383 : 2|| > This means that a CPU may hold vcrypto->ctrl_lock as long as 8192~16383us. > > To improve the performance of control queue, a request on control queue waits > completion instead of busy polling to reduce lock racing, and gets completed > by > control queue callback. > CPU0 CPU1 ... CPUx CPUy > | | | | > \ \ / / > \spin_lock(&vcrypto->ctrl_lock)---/ >| > virtqueue add & kick >| > -spin_unlock(&vcrypto->ctrl_lock)-- > / / \ \ > | | | | > wait wait wait wait > > Test this patch, the guest side get ~200K/s operations with 300% CPU > utilization. > > Cc: Michael S. Tsirkin > Cc: Jason Wang > Cc: Gonglei > Signed-off-by: zhenwei pi > --- > .../virtio/virtio_crypto_akcipher_algs.c | 29 ++- > drivers/crypto/virtio/virtio_crypto_common.h | 4 ++ > drivers/crypto/virtio/virtio_crypto_core.c| 52 ++- > .../virtio/virtio_crypto_skcipher_algs.c | 34 ++-- > 4 files changed, 64 insertions(+), 55 deletions(-) > Reviewed-by: Gonglei Regards, -Gonglei > diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c > b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c > index 698ea57e2649..382ccec9ab12 100644 > --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c > +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c > @@ -103,7 +103,6 @@ static int > virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher > struct scatterlist outhdr_sg, key_sg, inhdr_sg, *sgs[3]; > struct virtio_crypto *vcrypto = ctx->vcrypto; > uint8_t *pkey; > - unsigned int inlen; > int err; > unsigned int num_out = 0, num_in = 0; > struct virtio_crypto_op_ctrl_req *ctrl; @@ -135,18 +134,9 @@ static int > virtio_crypto_alg_akcipher_init_session(struct virtio_crypto_akcipher > sg_init_one(&inhdr_sg, input, sizeof(*input)); > sgs[num_out + num
RE: [PATCH v5 4/5] virtio-crypto: adjust dst_len at ops callback
> -Original Message-
> From: zhenwei pi [mailto:pizhen...@bytedance.com]
> Sent: Thursday, May 5, 2022 5:24 PM
> To: Gonglei (Arei) ; m...@redhat.com
> Cc: jasow...@redhat.com; herb...@gondor.apana.org.au;
> linux-ker...@vger.kernel.org; virtualization@lists.linux-foundation.org;
> linux-cry...@vger.kernel.org; helei.si...@bytedance.com;
> pizhen...@bytedance.com; da...@davemloft.net
> Subject: [PATCH v5 4/5] virtio-crypto: adjust dst_len at ops callback
>
> From: lei he
>
> 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
> Cc: Jason Wang
> Cc: Gonglei
> Signed-off-by: lei he
> Signed-off-by: zhenwei pi
> ---
> drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
Reviewed-by: Gonglei
Regards,
-Gonglei
> diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> index 382ccec9ab12..2a60d0525cde 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH v5 5/5] virtio-crypto: enable retry for virtio-crypto-dev
> -Original Message-
> From: zhenwei pi [mailto:pizhen...@bytedance.com]
> Sent: Thursday, May 5, 2022 5:24 PM
> To: Gonglei (Arei) ; m...@redhat.com
> Cc: jasow...@redhat.com; herb...@gondor.apana.org.au;
> linux-ker...@vger.kernel.org; virtualization@lists.linux-foundation.org;
> linux-cry...@vger.kernel.org; helei.si...@bytedance.com;
> pizhen...@bytedance.com; da...@davemloft.net
> Subject: [PATCH v5 5/5] virtio-crypto: enable retry for virtio-crypto-dev
>
> From: lei he
>
> Enable retry for virtio-crypto-dev, so that crypto-engine can process
> cipher-requests parallelly.
>
> Cc: Michael S. Tsirkin
> Cc: Jason Wang
> Cc: Gonglei
> Signed-off-by: lei he
> Signed-off-by: zhenwei pi
> ---
> 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]));
Here the '1' can be 'true' too.
Sure, you can add
Reviewed-by: Gonglei
Regards,
-Gonglei
> if (!vi->data_vq[i].engine) {
> ret = -ENOMEM;
> goto err_engine;
> --
> 2.20.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: RE: [PATCH v5 5/5] virtio-crypto: enable retry for virtio-crypto-dev
On 5/6/22 17:34, Gonglei (Arei) wrote:
-Original Message-
From: zhenwei pi [mailto:pizhen...@bytedance.com]
Sent: Thursday, May 5, 2022 5:24 PM
To: Gonglei (Arei) ; m...@redhat.com
Cc: jasow...@redhat.com; herb...@gondor.apana.org.au;
linux-ker...@vger.kernel.org; virtualization@lists.linux-foundation.org;
linux-cry...@vger.kernel.org; helei.si...@bytedance.com;
pizhen...@bytedance.com; da...@davemloft.net
Subject: [PATCH v5 5/5] virtio-crypto: enable retry for virtio-crypto-dev
From: lei he
Enable retry for virtio-crypto-dev, so that crypto-engine can process
cipher-requests parallelly.
Cc: Michael S. Tsirkin
Cc: Jason Wang
Cc: Gonglei
Signed-off-by: lei he
Signed-off-by: zhenwei pi
---
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]));
Here the '1' can be 'true' too.
Sure, you can add
Reviewed-by: Gonglei
Regards,
-Gonglei
if (!vi->data_vq[i].engine) {
ret = -ENOMEM;
goto err_engine;
--
2.20.1
Thanks to Lei!
Hi, Michael
I would appreciate it if you could apply this minor change, or I send
the v6 series, which one do you prefer?
--
zhenwei pi
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: RE: [PATCH v5 5/5] virtio-crypto: enable retry for virtio-crypto-dev
On Fri, May 06, 2022 at 05:55:33PM +0800, zhenwei pi wrote:
> On 5/6/22 17:34, Gonglei (Arei) wrote:
> >
> >
> > > -Original Message-
> > > From: zhenwei pi [mailto:pizhen...@bytedance.com]
> > > Sent: Thursday, May 5, 2022 5:24 PM
> > > To: Gonglei (Arei) ; m...@redhat.com
> > > Cc: jasow...@redhat.com; herb...@gondor.apana.org.au;
> > > linux-ker...@vger.kernel.org; virtualization@lists.linux-foundation.org;
> > > linux-cry...@vger.kernel.org; helei.si...@bytedance.com;
> > > pizhen...@bytedance.com; da...@davemloft.net
> > > Subject: [PATCH v5 5/5] virtio-crypto: enable retry for virtio-crypto-dev
> > >
> > > From: lei he
> > >
> > > Enable retry for virtio-crypto-dev, so that crypto-engine can process
> > > cipher-requests parallelly.
> > >
> > > Cc: Michael S. Tsirkin
> > > Cc: Jason Wang
> > > Cc: Gonglei
> > > Signed-off-by: lei he
> > > Signed-off-by: zhenwei pi
> > > ---
> > > 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]));
> >
> > Here the '1' can be 'true' too.
> >
> > Sure, you can add
> >
> > Reviewed-by: Gonglei
> >
> > Regards,
> > -Gonglei
> >
> > > if (!vi->data_vq[i].engine) {
> > > ret = -ENOMEM;
> > > goto err_engine;
> > > --
> > > 2.20.1
> >
>
> Thanks to Lei!
>
> Hi, Michael
> I would appreciate it if you could apply this minor change, or I send the v6
> series, which one do you prefer?
>
> --
send v6 with acks and change pls
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v6 0/5] virtio-crypto: Improve performance
v5 -> v6: - Minor fix for crypto_engine_alloc_init_and_set(). - All the patches have been reviewed by Gonglei, add this in patch. Thanks to Gonglei. v4 -> v5: - Fix potentially dereferencing uninitialized variables in 'virtio-crypto: use private buffer for control request'. Thanks to Dan Carpenter! 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 | 95 ++-- 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, 182 insertions(+), 129 deletions(-) -- 2.20.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v6 1/5] virtio-crypto: change code style
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
Cc: Jason Wang
Cc: Gonglei
Reviewed-by: Gonglei
Signed-off-by: zhenwei pi
---
.../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->c
[PATCH v6 5/5] virtio-crypto: enable retry for virtio-crypto-dev
From: lei he
Enable retry for virtio-crypto-dev, so that crypto-engine
can process cipher-requests parallelly.
Cc: Michael S. Tsirkin
Cc: Jason Wang
Cc: Gonglei
Reviewed-by: Gonglei
Signed-off-by: lei he
Signed-off-by: zhenwei pi
---
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..1198bd306365 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, true,
+
virtqueue_get_vring_size(vqs[i]));
if (!vi->data_vq[i].engine) {
ret = -ENOMEM;
goto err_engine;
--
2.20.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v6 3/5] virtio-crypto: wait ctrl queue instead of busy polling
Originally, after submitting request into virtio crypto control
queue, the guest side polls the result from the virt queue. This
works like following:
CPU0 CPU1 ... CPUx CPUy
| | | |
\ \ / /
\spin_lock(&vcrypto->ctrl_lock)---/
|
virtqueue add & kick
|
busy poll virtqueue
|
spin_unlock(&vcrypto->ctrl_lock)
...
There are two problems:
1, The queue depth is always 1, the performance of a virtio crypto
device gets limited. Multi user processes share a single control
queue, and hit spin lock race from control queue. Test on Intel
Platinum 8260, a single worker gets ~35K/s create/close session
operations, and 8 workers get ~40K/s operations with 800% CPU
utilization.
2, The control request is supposed to get handled immediately, but
in the current implementation of QEMU(v6.2), the vCPU thread kicks
another thread to do this work, the latency also gets unstable.
Tracking latency of virtio_crypto_alg_akcipher_close_session in 5s:
usecs : count distribution
0 -> 1 : 0||
2 -> 3 : 7||
4 -> 7 : 72 ||
8 -> 15 : 186485 ||
16 -> 31 : 687 ||
32 -> 63 : 5||
64 -> 127: 3||
128 -> 255: 1||
256 -> 511: 0||
512 -> 1023 : 0||
1024 -> 2047 : 0||
2048 -> 4095 : 0||
4096 -> 8191 : 0||
8192 -> 16383 : 2||
This means that a CPU may hold vcrypto->ctrl_lock as long as 8192~16383us.
To improve the performance of control queue, a request on control queue
waits completion instead of busy polling to reduce lock racing, and gets
completed by control queue callback.
CPU0 CPU1 ... CPUx CPUy
| | | |
\ \ / /
\spin_lock(&vcrypto->ctrl_lock)---/
|
virtqueue add & kick
|
-spin_unlock(&vcrypto->ctrl_lock)--
/ / \ \
| | | |
wait wait wait wait
Test this patch, the guest side get ~200K/s operations with 300% CPU
utilization.
Cc: Michael S. Tsirkin
Cc: Jason Wang
Cc: Gonglei
Reviewed-by: Gonglei
Signed-off-by: zhenwei pi
---
.../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 698ea57e2649..382ccec9ab12 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -103,7 +103,6 @@ static int virtio_crypto_alg_akcipher_init_session(struct
virtio_crypto_akcipher
struct scatterlist outhdr_sg, key_sg, inhdr_sg, *sgs[3];
struct virtio_crypto *vcrypto = ctx->vcrypto;
uint8_t *pkey;
- unsigned int inlen;
int err;
unsigned int num_out = 0, num_in = 0;
struct virtio_crypto_op_ctrl_req *ctrl;
@@ -135,18 +134,9 @@ static int virtio_crypto_alg_akcipher_init_session(struct
virtio_crypto_akcipher
sg_init_one(&inhdr_sg, input, sizeof(*input));
sgs[num_out + num_in++] = &inhdr_sg;
- spin_lock(&vcrypto->ctrl_lock);
- err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in,
vcrypto, GFP_ATOMIC);
- if (err < 0) {
- spin_unlock(&vcrypto->ctrl_lock);
+ err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, num_in,
vc_ctrl_req);
+ if (err < 0)
goto out;
- }
-
- virtqueue_kick(vcrypto->ctrl_vq);
- while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) &&
- !virtqueue_is_broken(vcrypto->ctrl_vq))
- cpu_relax();
- spin_unlock(&vcrypto->ctrl_lock);
if (le32_to_cpu(input->status) != VIRTIO_CRYP
[PATCH v6 4/5] virtio-crypto: adjust dst_len at ops callback
From: lei he
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
Cc: Jason Wang
Cc: Gonglei
Reviewed-by: Gonglei
Signed-off-by: lei he
Signed-off-by: zhenwei pi
---
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 382ccec9ab12..2a60d0525cde 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v6 2/5] virtio-crypto: use private buffer for control request
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.
Potentially dereferencing uninitialized variables:
Reported-by: kernel test robot
Reported-by: Dan Carpenter
Cc: Michael S. Tsirkin
Cc: Jason Wang
Cc: Gonglei
Reviewed-by: Gonglei
Signed-off-by: zhenwei pi
---
.../virtio/virtio_crypto_akcipher_algs.c | 57 ---
drivers/crypto/virtio/virtio_crypto_common.h | 17 --
.../virtio/virtio_crypto_skcipher_algs.c | 50 ++--
3 files changed, 79 insertions(+), 45 deletions(-)
diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 20901a263fc8..698ea57e2649 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,16 +135,22 @@ 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) {
+ pr_err("virtio_crypto: Create session failed status: %u\n",
+ le32_to_cpu(input->status));
err = -EINVAL;
goto out;
}
@@ -148,13 +160,9 @@ 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)
- pr_err("virtio_crypto: Create session failed status: %u\n",
- le32_to_cpu(input->status));
-
return err;
}
@@ -167,15 +175,18 @@ 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;
+ if (!ctx->session_valid)
+ return 0;
+
+ 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;
- 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,16 +199,22 @@ static int
virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
sg_init_one(&inhdr_sg, &ctrl_status->status,
sizeof(ctrl_status->status));
Re: Re: [PATCH 3/4] mm/memofy-failure.c: optimize hwpoison_filter
On 5/6/22 16:59, Naoya Horiguchi wrote:
On Fri, Apr 29, 2022 at 10:22:05PM +0800, zhenwei pi wrote:
In the memory failure procedure, hwpoison_filter has higher priority,
if memory_filter() filters the error event, there is no need to do
the further work.
Could you clarify what problem you are trying to solve (what does
"optimize" mean in this context or what is the benefit)?
OK. The background of this work:
As well known, the memory failure mechanism handles memory corrupted
event, and try to send SIGBUS to the user process which uses this
corrupted page.
For the virtualization case, QEMU catches SIGBUS and tries to inject MCE
into the guest, and the guest handles memory failure again. Thus the
guest gets the minimal effect from hardware memory corruption.
The further step I'm working on:
1, try to modify code to decrease poisoned pages in a single place
(mm/memofy-failure.c: simplify num_poisoned_pages_dec in this series).
2, try to use page_handle_poison() to handle SetPageHWPoison() and
num_poisoned_pages_inc() together. It would be best to call
num_poisoned_pages_inc() in a single place too. I'm not sure if this is
possible or not, please correct me if I misunderstand.
3, introduce memory failure notifier list in memory-failure.c: notify
the corrupted PFN to someone who registers this list.
If I can complete [1] and [2] part, [3] will be quite easy(just call
notifier list after increasing poisoned page).
4, introduce memory recover VQ for memory balloon device, and registers
memory failure notifier list. During the guest kernel handles memory
failure, balloon device gets notified by memory failure notifier list,
and tells the host to recover the corrupted PFN(GPA) by the new VQ.
5, host side remaps the corrupted page(HVA), and tells the guest side to
unpoison the PFN(GPA). Then the guest fixes the corrupted page(GPA)
dynamically.
Because [4] and [5] are related to balloon device, also CC Michael,
David and Jason.
Now hwpoison_filter() can be called both with *and* without taking page
refcount.
It's mainly called *with* taking page refcount in order to make sure that the
actual handling process is executed only for pages that meet a given condition.
IOW, it's important to prevent pages which do not meet the condition from going
ahead to further steps (false-positive is not permitted). So this type of
callsite should not be omittable.
As for the other case, hwpoison_filter() is also called in hwpoison_inject()
*without* taking page refcount. This actually has a different nuance and
intended to speculatively filter the injection events before setting
PageHWPoison flag to reduce the noise due to setting PG_hwpoison temporary.
The point is that it's not intended here to filter precisely and this callsite
is omittable.
So in my understanding, we need keep hwpoison_filter() after taking page
refcount as we do now. Maybe optionally and additionally calling
hwpoison_filter() at the beginning of memory_failure() might be possible,
but I'm not sure yet how helpful...
Thanks,
Naoya Horiguchi
Cc: Wu Fengguang
Signed-off-by: zhenwei pi
---
mm/memory-failure.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index ece05858568f..a6a27c8b800f 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1800,6 +1800,11 @@ int memory_failure(unsigned long pfn, int flags)
goto unlock_mutex;
}
+ if (hwpoison_filter(p)) {
+ res = -EOPNOTSUPP;
+ goto unlock_mutex;
+ }
+
try_again:
res = try_memory_failure_hugetlb(pfn, flags, &hugetlb);
if (hugetlb)
@@ -1937,15 +1942,6 @@ int memory_failure(unsigned long pfn, int flags)
*/
page_flags = p->flags;
- if (hwpoison_filter(p)) {
- if (TestClearPageHWPoison(p))
- num_poisoned_pages_dec();
- unlock_page(p);
- put_page(p);
- res = -EOPNOTSUPP;
- goto unlock_mutex;
- }
-
/*
* __munlock_pagevec may clear a writeback page's LRU flag without
* page_lock. We need wait writeback completion for this page or it
--
2.20.1
--
zhenwei pi
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] VMCI: Add support for ARM64
vd...@vmware.com writes:
> From: Vishnu Dasa
>
> Add support for ARM64 architecture so that the driver can now be built
> and VMCI device can be used.
>
> Update Kconfig file to allow the driver to be built on ARM64 as well.
> Fail vmci_guest_probe_device() on ARM64 if the device does not support
> MMIO register access. Lastly, add virtualization specific barriers
> which map to actual memory barrier instructions on ARM64, because it
> is required in case of ARM64 for queuepair (de)queuing.
>
FWIW, it seems you're doing three things at once, better split this into
a 3-patch series.
> Reviewed-by: Bryan Tan
> Reviewed-by: Cyprien Laplace
> Signed-off-by: Vishnu Dasa
> ---
> drivers/misc/vmw_vmci/Kconfig | 2 +-
> drivers/misc/vmw_vmci/vmci_guest.c | 4
> drivers/misc/vmw_vmci/vmci_queue_pair.c | 12
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/vmw_vmci/Kconfig b/drivers/misc/vmw_vmci/Kconfig
> index 605794aadf11..b6d4d7fd686a 100644
> --- a/drivers/misc/vmw_vmci/Kconfig
> +++ b/drivers/misc/vmw_vmci/Kconfig
> @@ -5,7 +5,7 @@
>
> config VMWARE_VMCI
> tristate "VMware VMCI Driver"
> - depends on X86 && PCI
> + depends on (X86 || ARM64) && !CPU_BIG_ENDIAN && PCI
> help
> This is VMware's Virtual Machine Communication Interface. It enables
> high-speed communication between host and guest in a virtual
> diff --git a/drivers/misc/vmw_vmci/vmci_guest.c
> b/drivers/misc/vmw_vmci/vmci_guest.c
> index 57a6157209a1..aa7b05de97dd 100644
> --- a/drivers/misc/vmw_vmci/vmci_guest.c
> +++ b/drivers/misc/vmw_vmci/vmci_guest.c
> @@ -614,6 +614,10 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> }
>
> if (!mmio_base) {
> + if (IS_ENABLED(CONFIG_ARM64)) {
> + dev_err(&pdev->dev, "MMIO base is invalid\n");
> + return -ENXIO;
> + }
> error = pcim_iomap_regions(pdev, BIT(0), KBUILD_MODNAME);
> if (error) {
> dev_err(&pdev->dev, "Failed to reserve/map IO
> regions\n");
> diff --git a/drivers/misc/vmw_vmci/vmci_queue_pair.c
> b/drivers/misc/vmw_vmci/vmci_queue_pair.c
> index 94ebf7f3fd58..8f2de1893245 100644
> --- a/drivers/misc/vmw_vmci/vmci_queue_pair.c
> +++ b/drivers/misc/vmw_vmci/vmci_queue_pair.c
> @@ -2577,6 +2577,12 @@ static ssize_t qp_enqueue_locked(struct vmci_queue
> *produce_q,
> if (result < VMCI_SUCCESS)
> return result;
>
> + /*
> + * This virt_wmb() ensures that data written to the queue
> + * is observable before the new producer_tail is.
> + */
> + virt_wmb();
> +
> vmci_q_header_add_producer_tail(produce_q->q_header, written,
> produce_q_size);
> return written;
> @@ -2620,6 +2626,12 @@ static ssize_t qp_dequeue_locked(struct vmci_queue
> *produce_q,
> if (buf_ready < VMCI_SUCCESS)
> return (ssize_t) buf_ready;
>
> + /*
> + * This virt_rmb() ensures that data from the queue will be read
> + * after we have determined how much is ready to be consumed.
> + */
> + virt_rmb();
> +
> read = (size_t) (buf_ready > buf_size ? buf_size : buf_ready);
> head = vmci_q_header_consumer_head(produce_q->q_header);
> if (likely(head + read < consume_q_size)) {
--
Vitaly
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/4] mm/memofy-failure.c: optimize hwpoison_filter
On 06.05.22 15:38, zhenwei pi wrote: > > > On 5/6/22 16:59, Naoya Horiguchi wrote: >> On Fri, Apr 29, 2022 at 10:22:05PM +0800, zhenwei pi wrote: >>> In the memory failure procedure, hwpoison_filter has higher priority, >>> if memory_filter() filters the error event, there is no need to do >>> the further work. >> >> Could you clarify what problem you are trying to solve (what does >> "optimize" mean in this context or what is the benefit)? >> > > OK. The background of this work: > As well known, the memory failure mechanism handles memory corrupted > event, and try to send SIGBUS to the user process which uses this > corrupted page. > > For the virtualization case, QEMU catches SIGBUS and tries to inject MCE > into the guest, and the guest handles memory failure again. Thus the > guest gets the minimal effect from hardware memory corruption. > > The further step I'm working on: > 1, try to modify code to decrease poisoned pages in a single place > (mm/memofy-failure.c: simplify num_poisoned_pages_dec in this series). > > 2, try to use page_handle_poison() to handle SetPageHWPoison() and > num_poisoned_pages_inc() together. It would be best to call > num_poisoned_pages_inc() in a single place too. I'm not sure if this is > possible or not, please correct me if I misunderstand. > > 3, introduce memory failure notifier list in memory-failure.c: notify > the corrupted PFN to someone who registers this list. > If I can complete [1] and [2] part, [3] will be quite easy(just call > notifier list after increasing poisoned page). > > 4, introduce memory recover VQ for memory balloon device, and registers > memory failure notifier list. During the guest kernel handles memory > failure, balloon device gets notified by memory failure notifier list, > and tells the host to recover the corrupted PFN(GPA) by the new VQ. Most probably you might want to do that asynchronously, and once the callback succeeds, un-poison the page. > > 5, host side remaps the corrupted page(HVA), and tells the guest side to > unpoison the PFN(GPA). Then the guest fixes the corrupted page(GPA) > dynamically. I think QEMU already does that during reboots. Now it would be triggered by the guest for individual pages. > > Because [4] and [5] are related to balloon device, also CC Michael, > David and Jason. Doesn't sound too crazy for me, although it's a shame that we always have to use virtio-balloon for such fairly balloon-unrelated things. -- Thanks, David / dhildenb ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Re: [PATCH 3/4] mm/memofy-failure.c: optimize hwpoison_filter
On 5/7/22 00:28, David Hildenbrand wrote: On 06.05.22 15:38, zhenwei pi wrote: On 5/6/22 16:59, Naoya Horiguchi wrote: On Fri, Apr 29, 2022 at 10:22:05PM +0800, zhenwei pi wrote: In the memory failure procedure, hwpoison_filter has higher priority, if memory_filter() filters the error event, there is no need to do the further work. Could you clarify what problem you are trying to solve (what does "optimize" mean in this context or what is the benefit)? OK. The background of this work: As well known, the memory failure mechanism handles memory corrupted event, and try to send SIGBUS to the user process which uses this corrupted page. For the virtualization case, QEMU catches SIGBUS and tries to inject MCE into the guest, and the guest handles memory failure again. Thus the guest gets the minimal effect from hardware memory corruption. The further step I'm working on: 1, try to modify code to decrease poisoned pages in a single place (mm/memofy-failure.c: simplify num_poisoned_pages_dec in this series). 2, try to use page_handle_poison() to handle SetPageHWPoison() and num_poisoned_pages_inc() together. It would be best to call num_poisoned_pages_inc() in a single place too. I'm not sure if this is possible or not, please correct me if I misunderstand. 3, introduce memory failure notifier list in memory-failure.c: notify the corrupted PFN to someone who registers this list. If I can complete [1] and [2] part, [3] will be quite easy(just call notifier list after increasing poisoned page). 4, introduce memory recover VQ for memory balloon device, and registers memory failure notifier list. During the guest kernel handles memory failure, balloon device gets notified by memory failure notifier list, and tells the host to recover the corrupted PFN(GPA) by the new VQ. Most probably you might want to do that asynchronously, and once the callback succeeds, un-poison the page. Yes! 5, host side remaps the corrupted page(HVA), and tells the guest side to unpoison the PFN(GPA). Then the guest fixes the corrupted page(GPA) dynamically. I think QEMU already does that during reboots. Now it would be triggered by the guest for individual pages. Yes, currently QEMU supports to un-poison corrupted pages during reset/reboot. We can reuse some code to do the work in this case, this allows a VM to fix corrupted pages as soon as possible(also no need to reset/reboot). Because [4] and [5] are related to balloon device, also CC Michael, David and Jason. Doesn't sound too crazy for me, although it's a shame that we always have to use virtio-balloon for such fairly balloon-unrelated things. Thanks! -- zhenwei pi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 5/6] net: virtio: switch to netif_napi_add_weight()
On Fri, 6 May 2022 10:07:50 -0700, Jakub Kicinski wrote:
> virtio netdev driver uses a custom napi weight, switch to the new
> API for setting custom weight.
>
> Signed-off-by: Jakub Kicinski
Reviewed-by: Xuan Zhuo
> ---
> CC: m...@redhat.com
> CC: jasow...@redhat.com
> CC: virtualization@lists.linux-foundation.org
> ---
> drivers/net/virtio_net.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ebb98b796352..db05b5e930be 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3313,8 +3313,8 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
> INIT_DELAYED_WORK(&vi->refill, refill_work);
> for (i = 0; i < vi->max_queue_pairs; i++) {
> vi->rq[i].pages = NULL;
> - netif_napi_add(vi->dev, &vi->rq[i].napi, virtnet_poll,
> -napi_weight);
> + netif_napi_add_weight(vi->dev, &vi->rq[i].napi, virtnet_poll,
> + napi_weight);
> netif_napi_add_tx_weight(vi->dev, &vi->sq[i].napi,
>virtnet_poll_tx,
>napi_tx ? napi_weight : 0);
> --
> 2.34.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 5/6] net: virtio: switch to netif_napi_add_weight()
On Sat, May 7, 2022 at 1:08 AM Jakub Kicinski wrote:
>
> virtio netdev driver uses a custom napi weight, switch to the new
> API for setting custom weight.
>
> Signed-off-by: Jakub Kicinski
Acked-by: Jason Wang
> ---
> CC: m...@redhat.com
> CC: jasow...@redhat.com
> CC: virtualization@lists.linux-foundation.org
> ---
> drivers/net/virtio_net.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ebb98b796352..db05b5e930be 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3313,8 +3313,8 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
> INIT_DELAYED_WORK(&vi->refill, refill_work);
> for (i = 0; i < vi->max_queue_pairs; i++) {
> vi->rq[i].pages = NULL;
> - netif_napi_add(vi->dev, &vi->rq[i].napi, virtnet_poll,
> - napi_weight);
> + netif_napi_add_weight(vi->dev, &vi->rq[i].napi, virtnet_poll,
> + napi_weight);
> netif_napi_add_tx_weight(vi->dev, &vi->sq[i].napi,
> virtnet_poll_tx,
> napi_tx ? napi_weight : 0);
> --
> 2.34.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH
On Thu, May 5, 2022 at 4:40 PM Stefano Garzarella wrote:
>
> On Thu, May 05, 2022 at 04:26:24PM +0800, Jason Wang wrote:
> >On Fri, Apr 29, 2022 at 3:14 PM Stefano Garzarella
> >wrote:
> >>
> >> On Fri, Apr 29, 2022 at 10:46:40AM +0800, Jason Wang wrote:
> >> >On Thu, Apr 28, 2022 at 11:13 PM Stefano Garzarella
> >> >wrote:
> >> >>
> >> >> The simulator behaves like a ramdisk, so we don't have to do
> >> >> anything when a VIRTIO_BLK_T_FLUSH request is received, but it
> >> >> could be useful to test driver behavior.
> >> >>
> >> >> Let's expose the VIRTIO_BLK_F_FLUSH feature to inform the driver
> >> >> that we support the flush command.
> >> >>
> >> >> Signed-off-by: Stefano Garzarella
> >> >> ---
> >> >> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 12
> >> >> 1 file changed, 12 insertions(+)
> >> >>
> >> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> >> b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> >> index 42d401d43911..a6dd1233797c 100644
> >> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> >> @@ -25,6 +25,7 @@
> >> >> #define DRV_LICENSE "GPL v2"
> >> >>
> >> >> #define VDPASIM_BLK_FEATURES (VDPASIM_FEATURES | \
> >> >> +(1ULL << VIRTIO_BLK_F_FLUSH)| \
> >> >> (1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
> >> >> (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
> >> >> (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
> >> >> @@ -166,6 +167,17 @@ static bool vdpasim_blk_handle_req(struct vdpasim
> >> >> *vdpasim,
> >> >> pushed += bytes;
> >> >> break;
> >> >>
> >> >> + case VIRTIO_BLK_T_FLUSH:
> >> >> + if (sector != 0) {
> >> >> + dev_err(&vdpasim->vdpa.dev,
> >> >> + "A driver MUST set sector to 0 for a
> >> >> VIRTIO_BLK_T_FLUSH request - sector: 0x%llx\n",
> >> >> + sector);
> >> >
> >> >If this is something that could be triggered by userspace/guest, then
> >> >we should avoid this.
> >>
> >> It can only be triggered by an erratic driver.
> >
> >Right, so guest can try to DOS the host via this.
>
> Yes, but I don't expect the simulator to be used in the real world, but
> only for testing and development, so the user should have full control
> of the guest.
Right, but from kernel POV it's better to avoid any guest triggerable behaviour.
>
> >
> >>
> >> I was using the simulator to test a virtio-blk driver that I'm writing
> >> in userspace and I forgot to set `sector` to zero, so I thought it would
> >> be useful.
> >>
> >> Do you mean to remove the error message?
> >
> >Some like dev_warn_once() might be better here.
>
> We also have other checks we do for each request (in and out header
> length, etc.) where we use dev_err(), should we change those too?
I think so.
>
> I don't know, from a developer's point of view I'd prefer to have them
> all printed, but actually if we have a totally wrong driver in the
> guest, we risk to hang our host to print an infinite number of messages.
Or we can use pr_debug() or tracepoints. Then the log is enabled conditally.
>
> Maybe we should change all the errors in the data path to
> dev_warn_once() and keep returning VIRTIO_BLK_S_IOERR to the guest which
> will surely get angry and print something.
>
> If you agree, I'll send a patch to change all the printing and then
> repost this with your suggestion as well.
Yes.
Thanks
>
> Thanks,
> Stefano
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 5/6] net: virtio: switch to netif_napi_add_weight()
On Fri, May 06, 2022 at 10:07:50AM -0700, Jakub Kicinski wrote:
> virtio netdev driver uses a custom napi weight, switch to the new
> API for setting custom weight.
>
> Signed-off-by: Jakub Kicinski
Acked-by: Michael S. Tsirkin
> ---
> CC: m...@redhat.com
> CC: jasow...@redhat.com
> CC: virtualization@lists.linux-foundation.org
> ---
> drivers/net/virtio_net.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ebb98b796352..db05b5e930be 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3313,8 +3313,8 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
> INIT_DELAYED_WORK(&vi->refill, refill_work);
> for (i = 0; i < vi->max_queue_pairs; i++) {
> vi->rq[i].pages = NULL;
> - netif_napi_add(vi->dev, &vi->rq[i].napi, virtnet_poll,
> -napi_weight);
> + netif_napi_add_weight(vi->dev, &vi->rq[i].napi, virtnet_poll,
> + napi_weight);
> netif_napi_add_tx_weight(vi->dev, &vi->sq[i].napi,
>virtnet_poll_tx,
>napi_tx ? napi_weight : 0);
> --
> 2.34.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
