On Thu, Mar 2, 2023 at 7:59 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
>
> This commit splits virtqueue_add_split() to two functions. The purpose
> of such splitting is to separate DMA operations.
>
> The first function includes all codes that may fail before the DMA
> operation. The subsequent part is used as the second function.
>
> In this way, we can perform DMA operations in the middle of the two
> functions. If the first function fails, we do not need to perform DMA
> operations. If it is premapped, we can pass the DMA operation.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> ---
> drivers/virtio/virtio_ring.c | 131 +++++++++++++++++++++++------------
> 1 file changed, 88 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 41144b5246a8..3005893ecc61 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -520,57 +520,38 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
> return next;
> }
>
> -static inline int virtqueue_add_split(struct virtqueue *_vq,
> - struct scatterlist *sgs[],
> - unsigned int total_sg,
> - unsigned int out_sgs,
> - unsigned int in_sgs,
> - void *data,
> - void *ctx,
> - gfp_t gfp)
> +/* note: return NULL means no indirect that is valid. */
> +static struct vring_desc *virtqueue_get_desc_split(struct vring_virtqueue *vq,
> + unsigned int total_sg,
> + unsigned int out_sgs,
> + void *data,
> + void *ctx,
> + gfp_t gfp)
> {
I don't object to the changes but I don't see much value in factoring
this out, since it's not related to any dma mapping logic.
> - struct vring_virtqueue *vq = to_vvq(_vq);
> - struct scatterlist *sg;
> struct vring_desc *desc;
> - unsigned int i, n, avail, descs_used, prev, err_idx;
> - int head;
> - bool indirect;
> -
> - START_USE(vq);
> + unsigned int descs_used;
>
> BUG_ON(data == NULL);
> BUG_ON(ctx && vq->indirect);
>
> - if (unlikely(vq->broken)) {
> - END_USE(vq);
> - return -EIO;
> - }
> + if (unlikely(vq->broken))
> + return ERR_PTR(-EIO);
>
> LAST_ADD_TIME_UPDATE(vq);
>
> BUG_ON(total_sg == 0);
>
> - head = vq->free_head;
> -
> if (virtqueue_use_indirect(vq, total_sg))
> - desc = alloc_indirect_split(_vq, total_sg, gfp);
> + desc = alloc_indirect_split(&vq->vq, total_sg, gfp);
> else {
> desc = NULL;
> WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
> }
>
> - if (desc) {
> - /* Use a single buffer which doesn't continue */
> - indirect = true;
> - /* Set up rest to use this indirect table. */
> - i = 0;
> + if (desc)
> descs_used = 1;
> - } else {
> - indirect = false;
> - desc = vq->split.vring.desc;
> - i = head;
> + else
> descs_used = total_sg;
> - }
>
> if (unlikely(vq->vq.num_free < descs_used)) {
> pr_debug("Can't add buf len %i - avail = %i\n",
> @@ -580,10 +561,39 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> * host should service the ring ASAP. */
> if (out_sgs)
> vq->notify(&vq->vq);
> - if (indirect)
> - kfree(desc);
> - END_USE(vq);
> - return -ENOSPC;
> + kfree(desc);
> + return ERR_PTR(-ENOSPC);
> + }
> +
> + return desc;
> +}
> +
> +static inline int virtqueue_add_vring_split(struct vring_virtqueue *vq,
> + struct scatterlist *sgs[],
> + unsigned int total_sg,
> + unsigned int out_sgs,
> + unsigned int in_sgs,
> + struct vring_desc *desc)
> +{
> + struct virtqueue *_vq = &vq->vq;
> + struct scatterlist *sg;
> + unsigned int i, n, avail, descs_used, prev, err_idx;
> + int head;
> + bool indirect;
> +
> + head = vq->free_head;
> +
> + if (desc) {
> + /* Use a single buffer which doesn't continue */
> + indirect = true;
> + /* Set up rest to use this indirect table. */
> + i = 0;
> + descs_used = 1;
> + } else {
> + indirect = false;
> + desc = vq->split.vring.desc;
> + i = head;
> + descs_used = total_sg;
> }
>
> for (n = 0; n < out_sgs; n++) {
> @@ -648,13 +658,6 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> else
> vq->free_head = i;
>
> - /* Store token and indirect buffer state. */
> - vq->split.desc_state[head].data = data;
> - if (indirect)
> - vq->split.desc_state[head].indir_desc = desc;
> - else
> - vq->split.desc_state[head].indir_desc = ctx;
> -
> /* Put entry in available array (but don't update avail->idx until they
> * do sync). */
> avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1);
> @@ -703,6 +706,48 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> return -ENOMEM;
> }
>
> +static inline int virtqueue_add_split(struct virtqueue *_vq,
> + struct scatterlist *sgs[],
> + unsigned int total_sg,
> + unsigned int out_sgs,
> + unsigned int in_sgs,
> + void *data,
> + void *ctx,
> + gfp_t gfp)
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + struct vring_desc *desc;
> + int head;
> + int err;
> +
> + START_USE(vq);
> +
> + /* check vq state and try to alloc desc for indirect. */
> + desc = virtqueue_get_desc_split(vq, total_sg, out_sgs, data, ctx, gfp);
> + if (IS_ERR(desc)) {
> + err = PTR_ERR(desc);
> + goto end;
> + }
> +
> + head = vq->free_head;
> + err = virtqueue_add_vring_split(vq, sgs, total_sg, out_sgs, in_sgs, desc);
> + if (err)
> + goto err;
> +
> + /* Store token and indirect buffer state. */
> + vq->split.desc_state[head].data = data;
> + vq->split.desc_state[head].indir_desc = desc ? desc : ctx;
> +
> + goto end;
Nit: this seems not elegant. I would rather duplicate
END_USE(vq);
return err;
here.
Thanks
> +
> +err:
> + kfree(desc);
> +
> +end:
> + END_USE(vq);
> + return err;
> +}
> +
> static bool virtqueue_kick_prepare_split(struct virtqueue *_vq)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> --
> 2.32.0.3.g01195cf9f
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
