On Tue, 7 Mar 2023 14:43:49 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Thu, Mar 2, 2023 at 7:59 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> >
> > This commit splits virtqueue_add_packed() 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 | 120 +++++++++++++++++++++++------------
> > 1 file changed, 81 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 178edf1171e2..6796cbee0207 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -1347,7 +1347,6 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> > unsigned int total_sg,
> > unsigned int out_sgs,
> > unsigned int in_sgs,
> > - void *data,
> > struct vring_packed_desc *desc)
> > {
> > struct scatterlist *sg;
> > @@ -1422,14 +1421,12 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> >
> > /* Store token and indirect buffer state. */
> > vq->packed.desc_state[id].num = 1;
> > - vq->packed.desc_state[id].data = data;
> > vq->packed.desc_state[id].indir_desc = desc;
> > vq->packed.desc_state[id].last = id;
> >
> > vq->num_added += 1;
> >
> > pr_debug("Added buffer head %i to %p\n", head, vq);
> > - END_USE(vq);
> >
> > return 0;
> >
> > @@ -1441,74 +1438,76 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> >
> > kfree(desc);
> >
> > - END_USE(vq);
> > return -ENOMEM;
> > }
> >
> > -static inline int virtqueue_add_packed(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)
> > +static inline struct vring_packed_desc *virtqueue_get_desc_packed(struct vring_virtqueue *vq,
> > + unsigned int total_sg,
> > + void *data,
> > + void *ctx,
> > + gfp_t gfp)
> > {
> > - struct vring_virtqueue *vq = to_vvq(_vq);
> > struct vring_packed_desc *desc;
> > - struct scatterlist *sg;
> > - unsigned int i, n, c, descs_used, err_idx;
> > - __le16 head_flags, flags;
> > - u16 head, id, prev, curr, avail_used_flags;
> > - int err;
> > -
> > - 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);
> >
> > + desc = NULL;
> > +
> > if (virtqueue_use_indirect(vq, total_sg)) {
> > desc = alloc_indirect_packed(total_sg, gfp);
> > if (desc) {
> > if (unlikely(vq->vq.num_free < 1)) {
> > pr_debug("Can't add buf len 1 - avail = 0\n");
> > kfree(desc);
> > - END_USE(vq);
> > - return -ENOSPC;
> > + return ERR_PTR(-ENOSPC);
> > }
> >
> > - return virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
> > - in_sgs, data, desc);
> > + return NULL;
> > }
> >
> > /* fall back on direct */
> > }
> >
> > - head = vq->packed.next_avail_idx;
> > - avail_used_flags = vq->packed.avail_used_flags;
> > -
> > WARN_ON_ONCE(total_sg > vq->packed.vring.num && !vq->indirect);
> >
> > - desc = vq->packed.vring.desc;
> > - i = head;
> > descs_used = total_sg;
> >
> > if (unlikely(vq->vq.num_free < descs_used)) {
> > pr_debug("Can't add buf len %i - avail = %i\n",
> > descs_used, vq->vq.num_free);
> > - END_USE(vq);
> > - return -ENOSPC;
> > + return ERR_PTR(-ENOSPC);
> > }
> >
> > + return desc;
> > +}
> > +
> > +static inline int virtqueue_add_vring_packed(struct vring_virtqueue *vq,
> > + struct scatterlist *sgs[],
> > + unsigned int total_sg,
> > + unsigned int out_sgs,
> > + unsigned int in_sgs)
> > +{
> > + struct vring_packed_desc *desc;
> > + struct scatterlist *sg;
> > + unsigned int i, n, c, descs_used, err_idx;
> > + __le16 head_flags, flags;
> > + u16 head, id, prev, curr, avail_used_flags;
> > +
> > + desc = vq->packed.vring.desc;
> > + head = vq->packed.next_avail_idx;
> > + i = head;
> > + descs_used = total_sg;
> > + avail_used_flags = vq->packed.avail_used_flags;
> > +
> > id = vq->free_head;
> > BUG_ON(id == vq->packed.vring.num);
> >
> > @@ -1563,8 +1562,6 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> >
> > /* Store token. */
> > vq->packed.desc_state[id].num = descs_used;
> > - vq->packed.desc_state[id].data = data;
> > - vq->packed.desc_state[id].indir_desc = ctx;
> > vq->packed.desc_state[id].last = prev;
> >
> > /*
> > @@ -1577,7 +1574,6 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > vq->num_added += descs_used;
> >
> > pr_debug("Added buffer head %i to %p\n", head, vq);
> > - END_USE(vq);
> >
> > return 0;
> >
> > @@ -1598,10 +1594,56 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > i = 0;
> > }
> >
> > - END_USE(vq);
> > return -EIO;
> > }
> >
> > +static inline int virtqueue_add_packed(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_packed_desc *desc;
> > + u16 id;
> > + int err;
> > +
> > + START_USE(vq);
> > +
> > + /* check vq state and try to alloc desc for indirect. */
> > + desc = virtqueue_get_desc_packed(vq, total_sg, data, ctx, gfp);
> > + if (IS_ERR(desc)) {
> > + err = PTR_ERR(desc);
> > + goto end;
> > + }
> > +
> > + id = vq->free_head;
> > +
> > + if (desc) {
> > + err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs, in_sgs, desc);
> > + if (err)
> > + goto err;
> > + } else {
> > + virtqueue_add_vring_packed(vq, sgs, total_sg, out_sgs, in_sgs);
>
> While at it, can we unify the logic of virtqueue_add_indirect_packed()
> and virtqueue_add_vring_packed()?
I guess, you mean to merge these two functions, I will try it.
>
> > + vq->packed.desc_state[id].indir_desc = ctx;
> > + }
> > +
> > + vq->packed.desc_state[id].data = data;
> > +
> > + goto end;
>
> Similar to split, I'd rather duplicate the END_USE() and return.
Will fix.
Thanks.
>
> Thanks
>
>
> > +
> > +err:
> > + kfree(desc);
> > +
> > +end:
> > + END_USE(vq);
> > + return err;
> > +}
> > +
> > static bool virtqueue_kick_prepare_packed(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
