Re: Re: [PATCH 1/4] virtio-crypto: wait ctrl queue instead of busy polling

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Apr 15, 2022 at 06:50:19PM +0800, zhenwei pi wrote:
> On 4/15/22 16:41, Michael S. Tsirkin wrote:
> > > diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> > > index f3ec9420215e..bf7c1aa4be37 100644
> > > --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> > > +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> > > @@ -102,107 +102,100 @@ 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;
> > > +	struct virtio_crypto_ctrl_request *vc_ctrl_req = NULL;
> > 
> > this is initialized down the road, I think you can skip = NULL here.
> > 
> OK.
> > >   	uint8_t *pkey;
> > > -	unsigned int inlen;
> > > -	int err;
> > > +	int err = -ENOMEM;
> > 
> > I would assign this in the single case where this value is used.
> > 
> OK
> > >   	unsigned int num_out = 0, num_in = 0;
> > > +	int node = dev_to_node(&vcrypto->vdev->dev);
> > are you sure it is
> > better to allocate close to device and not to current node
> > which is the default?
> > 
> Also with this part:
>  /* Internal representation of a data virtqueue */
> @@ -65,11 +66,6 @@ struct virtio_crypto {
>  	/* Maximum size of per request */
>  	u64 max_size;
> 
> -	/* Control VQ buffers: protected by the ctrl_lock */
> -	struct virtio_crypto_op_ctrl_req ctrl;
> -	struct virtio_crypto_session_input input;
> -	struct virtio_crypto_inhdr ctrl_status;
> -
>  	unsigned long status;
>  	atomic_t ref_count;
> 
> Orignally virtio crypto driver allocates ctrl&input&ctrl_status per-device,
> and protects this with ctrl_lock. This is the reason why the control queue
> reaches the bottleneck of performance. I'll append this in the next version
> in commit message.
> 
> Instead of the single request buffer, declare struct
> virtio_crypto_ctrl_request {
>         struct virtio_crypto_op_ctrl_req ctrl;
>         struct virtio_crypto_session_input input;
>         struct virtio_crypto_inhdr ctrl_status;
> 	... }
> 
> The motivation of this change is to allocate buffer from the same node with
> device during control queue operations.

But are you sure it's a win?  quite possibly it's a win to
have it close to driver not close to device.
This kind of change is really best done separately with some
testing showing it's a win. If that is too much to ask,
make it a separate patch and add some analysis explaining
why device accesses the structure more than the driver.


> > 
> > >   	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);
> > > +	vc_ctrl_req = kzalloc_node(sizeof(*vc_ctrl_req), GFP_KERNEL, node);
> > > +	if (!vc_ctrl_req)
> > > +		goto out;
> > 
> > do you need to allocate it with kzalloc?
> > is anything wrong with just keeping it part of device?
> > even if yes this change is better split in a separate patch, would make the patch smaller.
> Because there are padding field in
> virtio_crypto_op_ctrl_req&virtio_crypto_session_input, I suppose the
> original version also needs to clear padding field.
> So I use kzalloc to make sure that the padding field gets cleared.
> If this is reasonable, to separate this patch is OK to me, or I append this
> reason into commit message and comments in code.

Not sure I understand. Maybe add a code comment explaining
what is cleared and why.

> > > +
> > > +void virtcrypto_ctrlq_callback(struct virtqueue *vq)
> > > +{
> > > +	struct virtio_crypto *vcrypto = vq->vdev->priv;
> > > +	struct virtio_crypto_ctrl_request *vc_ctrl_req;
> > > +	unsigned long flags;
> > > +	unsigned int len;
> > > +
> > > +	spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
> > > +	do {
> > > +		virtqueue_disable_cb(vq);
> > > +		while ((vc_ctrl_req = virtqueue_get_buf(vq, &len)) != NULL) {
> > 
> > 
> > you really need to break out of this loop if vq is broken,
> > virtqueue_get_buf will keep returning NULL in this case.
> > 
> I'm a little confused here, if virtqueue_get_buf return NULL, this loop will
> break. Could you please give me more hints?

Oh right. Sorry was confused.

> > 
> > > +			spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
> > > +			if (vc_ctrl_req->ctrl_cb)
> > > +				vc_ctrl_req->ctrl_cb(vc_ctrl_req);
> > > +			spin_lock_irqsave(&vcrypto->ctrl_lock, flags);
> > > +		}
> > > +		if (unlikely(virtqueue_is_broken(vq)))
> > > +			break;
> > > +	} while (!virtqueue_enable_cb(vq));
> > > +	spin_unlock_irqrestore(&vcrypto->ctrl_lock, flags);
> > 
> > speaking of which existing code does not handle vq broken case
> > all that well but it looks like this patch makes it a bit worse.
> > want to try fixing? basically report an error ...
> > 
> > if virtqueue is broken, I can print log.
> 
> > > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c b/drivers/crypto/virtio/virtio_crypto_core.c
> > > index c6f482db0bc0..e668d4b1bc6a 100644
> > > --- a/drivers/crypto/virtio/virtio_crypto_core.c
> > > +++ b/drivers/crypto/virtio/virtio_crypto_core.c
> > > @@ -73,7 +73,7 @@ static int virtcrypto_find_vqs(struct virtio_crypto *vi)
> > >   		goto err_names;
> > >   	/* Parameters for control virtqueue */
> > > -	callbacks[total_vqs - 1] = NULL;
> > > +	callbacks[total_vqs - 1] = virtcrypto_ctrlq_callback;
> > >   	names[total_vqs - 1] = "controlq";
> > >   	/* Allocate/initialize parameters for data virtqueues */
> > > diff --git a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> > > index a618c46a52b8..b8999dab3e66 100644
> > > --- a/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> > > +++ b/drivers/crypto/virtio/virtio_crypto_skcipher_algs.c
> > > +	err = 0;
> > > +out:
> > > +	kfree_sensitive(vc_ctrl_req);
> > 
> > it is interesting that you use kfree_sensitive here. why is that?
> > is there in fact anything sensitive here? if yes this is a security
> > improvement and might need its own patch, or at least documentation.
> > 
> 
> OK, kfree is good enough here, I'll fix this.
> 
> 
> Thanks a lot!
> 
> 
> -- 
> zhenwei pi