Re: [RFC net-next v1 02/12] virtio_ring: split: record extras for indirect buffers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Sep 24, 2024 at 9:32 AM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
>
> The subsequent commit needs to know whether every indirect buffer is
> premapped or not. So we need to introduce an extra struct for every
> indirect buffer to record this info.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> ---
>  drivers/virtio/virtio_ring.c | 126 +++++++++++++++++------------------
>  1 file changed, 61 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 228e9fbcba3f..62901bee97c0 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -67,9 +67,20 @@
>  #define LAST_ADD_TIME_INVALID(vq)
>  #endif
>
> +struct vring_desc_extra {
> +       dma_addr_t addr;                /* Descriptor DMA addr. */
> +       u32 len;                        /* Descriptor length. */
> +       u16 flags;                      /* Descriptor flags. */
> +       u16 next;                       /* The next desc state in a list. */
> +};
> +
>  struct vring_desc_state_split {
>         void *data;                     /* Data for callback. */
> -       struct vring_desc *indir_desc;  /* Indirect descriptor, if any. */
> +
> +       /* Indirect extra table and desc table, if any. These two will be
> +        * allocated together. So we won't stress more to the memory allocator.
> +        */
> +       struct vring_desc_extra *indir;
>  };
>
>  struct vring_desc_state_packed {
> @@ -79,13 +90,6 @@ struct vring_desc_state_packed {
>         u16 last;                       /* The last desc state in a list. */
>  };
>
> -struct vring_desc_extra {
> -       dma_addr_t addr;                /* Descriptor DMA addr. */
> -       u32 len;                        /* Descriptor length. */
> -       u16 flags;                      /* Descriptor flags. */
> -       u16 next;                       /* The next desc state in a list. */
> -};
> -
>  struct vring_virtqueue_split {
>         /* Actual memory layout for this queue. */
>         struct vring vring;
> @@ -440,38 +444,20 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
>   * Split ring specific functions - *_split().
>   */
>
> -static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
> -                                          const struct vring_desc *desc)
> -{
> -       u16 flags;
> -
> -       if (!vring_need_unmap_buffer(vq))
> -               return;
> -
> -       flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
> -
> -       dma_unmap_page(vring_dma_dev(vq),
> -                      virtio64_to_cpu(vq->vq.vdev, desc->addr),
> -                      virtio32_to_cpu(vq->vq.vdev, desc->len),
> -                      (flags & VRING_DESC_F_WRITE) ?
> -                      DMA_FROM_DEVICE : DMA_TO_DEVICE);
> -}
> -
>  static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
> -                                         unsigned int i)
> +                                         struct vring_desc_extra *extra)
>  {
> -       struct vring_desc_extra *extra = vq->split.desc_extra;
>         u16 flags;
>
> -       flags = extra[i].flags;
> +       flags = extra->flags;
>
>         if (flags & VRING_DESC_F_INDIRECT) {
>                 if (!vq->use_dma_api)
>                         goto out;
>
>                 dma_unmap_single(vring_dma_dev(vq),
> -                                extra[i].addr,
> -                                extra[i].len,
> +                                extra->addr,
> +                                extra->len,
>                                  (flags & VRING_DESC_F_WRITE) ?
>                                  DMA_FROM_DEVICE : DMA_TO_DEVICE);
>         } else {
> @@ -479,20 +465,22 @@ static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
>                         goto out;
>
>                 dma_unmap_page(vring_dma_dev(vq),
> -                              extra[i].addr,
> -                              extra[i].len,
> +                              extra->addr,
> +                              extra->len,
>                                (flags & VRING_DESC_F_WRITE) ?
>                                DMA_FROM_DEVICE : DMA_TO_DEVICE);
>         }
>
>  out:
> -       return extra[i].next;
> +       return extra->next;
>  }
>
>  static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq,
>                                                unsigned int total_sg,
> +                                              struct vring_desc_extra **pextra,

This kind of API seems a little bit strange. In the caller we had:

if (desc) {
...
} else {
extra = vq->split.desc_extra;
}

I'd move the assignment of the extra there to make the code more readable.

>                                                gfp_t gfp)
>  {
> +       struct vring_desc_extra *extra;
>         struct vring_desc *desc;
>         unsigned int i;
>
> @@ -503,40 +491,45 @@ static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq,
>          */
>         gfp &= ~__GFP_HIGHMEM;
>
> -       desc = kmalloc_array(total_sg, sizeof(struct vring_desc), gfp);
> -       if (!desc)
> +       extra = kmalloc_array(total_sg, sizeof(*desc) + sizeof(*extra), gfp);

I'd suggest using kmalloc() directly, as it seems not an array where
each descriptor is followed by one extra.

> +       if (!extra)
>                 return NULL;
>
> -       for (i = 0; i < total_sg; i++)
> +       desc = (struct vring_desc *)&extra[total_sg];

Still seems strange, I would rather make the descriptor come first
then. This makes code more readable. And we can keep the indir_desc
variable name.

> +
> +       for (i = 0; i < total_sg; i++) {
>                 desc[i].next = cpu_to_virtio16(_vq->vdev, i + 1);
> +               extra[i].next = i + 1;
> +       }
> +
> +       *pextra = extra;
> +
>         return desc;
>  }
>
>  static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
>                                                     struct vring_desc *desc,
> +                                                   struct vring_desc_extra *extra,
>                                                     unsigned int i,
>                                                     dma_addr_t addr,
>                                                     unsigned int len,
>                                                     u16 flags,
>                                                     bool indirect)
>  {
> -       struct vring_virtqueue *vring = to_vvq(vq);
> -       struct vring_desc_extra *extra = vring->split.desc_extra;
>         u16 next;
>
>         desc[i].flags = cpu_to_virtio16(vq->vdev, flags);
>         desc[i].addr = cpu_to_virtio64(vq->vdev, addr);
>         desc[i].len = cpu_to_virtio32(vq->vdev, len);
>
> -       if (!indirect) {
> -               next = extra[i].next;
> -               desc[i].next = cpu_to_virtio16(vq->vdev, next);
> +       extra[i].addr = addr;
> +       extra[i].len = len;
> +       extra[i].flags = flags;
> +
> +       next = extra[i].next;
>
> -               extra[i].addr = addr;
> -               extra[i].len = len;
> -               extra[i].flags = flags;
> -       } else
> -               next = virtio16_to_cpu(vq->vdev, desc[i].next);
> +       if (!indirect)
> +               desc[i].next = cpu_to_virtio16(vq->vdev, next);

Any chance we can remove the check for indirect here? For example,
remove the assignment in alloc_indirect_split(). This would be more
consistent with the way when we want to use desc_extra for both
indirect and direct descriptors.

>
>         return next;
>  }
> @@ -551,6 +544,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>                                       gfp_t gfp)
>  {
>         struct vring_virtqueue *vq = to_vvq(_vq);
> +       struct vring_desc_extra *extra;
>         struct scatterlist *sg;
>         struct vring_desc *desc;
>         unsigned int i, n, avail, descs_used, prev, err_idx;
> @@ -574,7 +568,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>         head = vq->free_head;
>
>         if (virtqueue_use_indirect(vq, total_sg))
> -               desc = alloc_indirect_split(_vq, total_sg, gfp);
> +               desc = alloc_indirect_split(_vq, total_sg, &extra, gfp);
>         else {
>                 desc = NULL;
>                 WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
> @@ -589,6 +583,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>         } else {
>                 indirect = false;
>                 desc = vq->split.vring.desc;
> +               extra = vq->split.desc_extra;
>                 i = head;
>                 descs_used = total_sg;
>         }
> @@ -618,7 +613,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>                         /* Note that we trust indirect descriptor
>                          * table since it use stream DMA mapping.
>                          */
> -                       i = virtqueue_add_desc_split(_vq, desc, i, addr, sg->length,
> +                       i = virtqueue_add_desc_split(_vq, desc, extra, i, addr, sg->length,
>                                                      VRING_DESC_F_NEXT,
>                                                      indirect);
>                 }
> @@ -634,7 +629,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>                         /* Note that we trust indirect descriptor
>                          * table since it use stream DMA mapping.
>                          */
> -                       i = virtqueue_add_desc_split(_vq, desc, i, addr,
> +                       i = virtqueue_add_desc_split(_vq, desc, extra, i, addr,
>                                                      sg->length,
>                                                      VRING_DESC_F_NEXT |
>                                                      VRING_DESC_F_WRITE,
> @@ -660,6 +655,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>                 }
>
>                 virtqueue_add_desc_split(_vq, vq->split.vring.desc,
> +                                        vq->split.desc_extra,
>                                          head, addr,
>                                          total_sg * sizeof(struct vring_desc),
>                                          VRING_DESC_F_INDIRECT,
> @@ -678,9 +674,9 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>         /* Store token and indirect buffer state. */
>         vq->split.desc_state[head].data = data;
>         if (indirect)
> -               vq->split.desc_state[head].indir_desc = desc;
> +               vq->split.desc_state[head].indir = extra;
>         else
> -               vq->split.desc_state[head].indir_desc = ctx;
> +               vq->split.desc_state[head].indir = ctx;
>
>         /* Put entry in available array (but don't update avail->idx until they
>          * do sync). */
> @@ -716,11 +712,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>         for (n = 0; n < total_sg; n++) {
>                 if (i == err_idx)
>                         break;
> -               if (indirect) {
> -                       vring_unmap_one_split_indirect(vq, &desc[i]);
> -                       i = virtio16_to_cpu(_vq->vdev, desc[i].next);
> -               } else
> -                       i = vring_unmap_one_split(vq, i);
> +
> +               i = vring_unmap_one_split(vq, &extra[i]);
>         }
>
>  free_indirect:
> @@ -765,22 +758,25 @@ static bool virtqueue_kick_prepare_split(struct virtqueue *_vq)
>  static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>                              void **ctx)
>  {
> +       struct vring_desc_extra *extra;
>         unsigned int i, j;
>         __virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
>
>         /* Clear data ptr. */
>         vq->split.desc_state[head].data = NULL;
>
> +       extra = vq->split.desc_extra;
> +
>         /* Put back on free list: unmap first-level descriptors and find end */
>         i = head;
>
>         while (vq->split.vring.desc[i].flags & nextflag) {
> -               vring_unmap_one_split(vq, i);
> +               vring_unmap_one_split(vq, &extra[i]);
>                 i = vq->split.desc_extra[i].next;
>                 vq->vq.num_free++;
>         }
>
> -       vring_unmap_one_split(vq, i);
> +       vring_unmap_one_split(vq, &extra[i]);
>         vq->split.desc_extra[i].next = vq->free_head;
>         vq->free_head = head;
>
> @@ -788,12 +784,12 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>         vq->vq.num_free++;
>
>         if (vq->indirect) {
> -               struct vring_desc *indir_desc =
> -                               vq->split.desc_state[head].indir_desc;
>                 u32 len;
>
> +               extra = vq->split.desc_state[head].indir;
> +
>                 /* Free the indirect table, if any, now that it's unmapped. */
> -               if (!indir_desc)
> +               if (!extra)
>                         return;
>
>                 len = vq->split.desc_extra[head].len;
> @@ -804,13 +800,13 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>
>                 if (vring_need_unmap_buffer(vq)) {
>                         for (j = 0; j < len / sizeof(struct vring_desc); j++)
> -                               vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> +                               vring_unmap_one_split(vq, &extra[j]);
>                 }
>
> -               kfree(indir_desc);
> -               vq->split.desc_state[head].indir_desc = NULL;
> +               kfree(extra);
> +               vq->split.desc_state[head].indir = NULL;
>         } else if (ctx) {
> -               *ctx = vq->split.desc_state[head].indir_desc;
> +               *ctx = vq->split.desc_state[head].indir;
>         }
>  }
>
> --
> 2.32.0.3.g01195cf9f
>

Thanks






[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux