Re: [RFC 1/4] virtio_ring: introduce virtqueue_get_buf_ctx_dma()

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

 



On Fri, 24 Nov 2023 12:14:57 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Wed, Nov 22, 2023 at 2:57 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, 22 Nov 2023 14:47:02 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > On Wed, Nov 22, 2023 at 2:34 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Wed, 22 Nov 2023 14:03:32 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > > > On Tue, Nov 21, 2023 at 2:27 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Thu, 16 Nov 2023 16:11:11 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > > > > > On Tue, Nov 14, 2023 at 7:31 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > introduce virtqueue_get_buf_ctx_dma() to collect the dma info when
> > > > > > > > get buf from virtio core for premapped mode.
> > > > > > > >
> > > > > > > > If the virtio queue is premapped mode, the virtio-net send buf may
> > > > > > > > have many desc. Every desc dma address need to be unmap. So here we
> > > > > > > > introduce a new helper to collect the dma address of the buffer from
> > > > > > > > the virtio core.
> > > > > > >
> > > > > > > So looking at vring_desc_extra, what we have right now is:
> > > > > > >
> > > > > > > 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. */
> > > > > > > };
> > > > > > >
> > > > > > > And sg is
> > > > > > >
> > > > > > > struct scatterlist {
> > > > > > >         unsigned long   page_link;
> > > > > > > unsigned int    offset;
> > > > > > >         unsigned int    length;
> > > > > > >         dma_addr_t      dma_address;
> > > > > > > #ifdef CONFIG_NEED_SG_DMA_LENGTH
> > > > > > >         unsigned int    dma_length;
> > > > > > > #endif
> > > > > > > #ifdef CONFIG_NEED_SG_DMA_FLAGS
> > > > > > >         unsigned int    dma_flags;
> > > > > > > #endif
> > > > > > > };
> > > > > > >
> > > > > > > Would it better just store sg?
> > > > > >
> > > > > > Do you mean we expose the vring_desc_extra to dirver?
> > > > >
> > > > > Maybe or not, (or just us sg for desc_extra).
> > > > >
> > > > > Anyhow, any method needs to be benchmarked to see the performance impact.
> > > > >
> > > > > >
> > > > > > How about introducing such a new structure?
> > > > > >
> > > > > > struct virtio_dma_item {
> > > > > >          dma_addr_t addr;
> > > > > >          u32 len;
> > > > > > };
> > > > > >
> > > > > > struct virtio_dma_head {
> > > > > >         u32 num;
> > > > > >         u32 used;
> > > > > >         struct virtio_dma_item items[];
> > > > > > };
> > > > > >
> > > > > > Then we just need to pass one pointer to the virtio.
> > > > >
> > > > > Just to make sure I understand here, where did we store those arrays?
> > > >
> > > >
> > > > On the stack, or we can reuse the memory space of sq->sg.
> > > >
> > > > dma = (struct virtio_dma_head*)sq->sg;
> > >
> > > Ok, we need some benchmark for sure.
> > >
> > > >
> > > >
> > > > >
> > > > > > num is used to pass the size of items
> > > > > > used is used to return the num used by virtio core.
> > > > > >
> > > > > > >
> > > > > > > More below
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > > > > > > > ---
> > > > > > > >  drivers/virtio/virtio_ring.c | 148 ++++++++++++++++++++++++++---------
> > > > > > > >  include/linux/virtio.h       |   2 +
> > > > > > > >  2 files changed, 115 insertions(+), 35 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > > > index 51d8f3299c10..0b3caee4ef9d 100644
> > > > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > > > @@ -362,6 +362,20 @@ static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
> > > > > > > >         return vq->dma_dev;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static void store_dma_to_sg(struct scatterlist **sgp, dma_addr_t addr, unsigned int length)
> > > > > > > > +{
> > > > > > > > +       struct scatterlist *sg;
> > > > > > > > +
> > > > > > > > +       sg = *sgp;
> > > > > > > > +
> > > > > > > > +       sg->dma_address = addr;
> > > > > > > > +       sg->length = length;
> > > > > > > > +
> > > > > > > > +       sg = sg_next(sg);
> > > > > > > > +
> > > > > > > > +       *sgp = sg;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  /* Map one sg entry. */
> > > > > > > >  static int vring_map_one_sg(const struct vring_virtqueue *vq, struct scatterlist *sg,
> > > > > > > >                             enum dma_data_direction direction, dma_addr_t *addr)
> > > > > > > > @@ -441,12 +455,18 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
> > > > > > > >   */
> > > > > > > >
> > > > > > > >  static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
> > > > > > > > -                                          const struct vring_desc *desc)
> > > > > > > > +                                          const struct vring_desc *desc,
> > > > > > > > +                                          struct scatterlist **sg)
> > > > > > > >  {
> > > > > > > >         u16 flags;
> > > > > > > >
> > > > > > > > -       if (!vq->do_unmap)
> > > > > > > > +       if (!vq->do_unmap) {
> > > > > > > > +               if (*sg)
> > > > > > >
> > > > > > > Can we simply move the
> > > > > > >
> > > > > > > if (*sg) to store_dma_to_sg()?
> > > > > >
> > > > > > OK
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > +                       store_dma_to_sg(sg,
> > > > > > > > +                                       virtio64_to_cpu(vq->vq.vdev, desc->addr),
> > > > > > > > +                                       virtio32_to_cpu(vq->vq.vdev, desc->len));
> > > > > > > >                 return;
> > > > > > > > +       }
> > > > > > > >
> > > > > > > >         flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
> > > > > > > >
> > > > > > > > @@ -458,7 +478,7 @@ static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
> > > > > > > > -                                         unsigned int i)
> > > > > > > > +                                         unsigned int i, struct scatterlist **sg)
> > > > > > > >  {
> > > > > > > >         struct vring_desc_extra *extra = vq->split.desc_extra;
> > > > > > > >         u16 flags;
> > > > > > > > @@ -475,8 +495,11 @@ static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
> > > > > > > >                                  (flags & VRING_DESC_F_WRITE) ?
> > > > > > > >                                  DMA_FROM_DEVICE : DMA_TO_DEVICE);
> > > > > > > >         } else {
> > > > > > > > -               if (!vq->do_unmap)
> > > > > > > > +               if (!vq->do_unmap) {
> > > > > > >
> > > > > > > The:
> > > > > > >
> > > > > > > else {
> > > > > > >       if {
> > > > > > >             if {
> > > > > > > }
> > > > > > >
> > > > > > > seems odd. I think I would prefer
> > > > > > >
> > > > > > > if (flags & VRING_DESC_F_INDIRECT) {
> > > > > > > } else if (!vq->do_unmap) {
> > > > > > > } else {
> > > > > > > }
> > > > > >
> > > > > >
> > > > > > Will fix.
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > here
> > > > > > >
> > > > > > > Btw, I really think do_unmap is not a good name, we probably need to
> > > > > > > rename it as "unmap_desc".
> > > > > >
> > > > > > How about a separate patch for this?
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > +                       if (*sg)
> > > > > > > > +                               store_dma_to_sg(sg, extra[i].addr, extra[i].len);
> > > > > > >
> > > > > > > In which case we need to unmap by driver but we don't need a dma address?
> > > > > >
> > > > > > Sorry, do not get it.
> > > > >
> > > > > I meant, I don't get how we can reach to this check.
> > > >
> > > > This is the main way. I think you miss something.
> > >
> > > Or maybe you can explain the case where vq->do_unmap is false but *sg is NULL?
> >
> >
> > If vq->do_unmap is false, that is premapped mode or use_dma_api is false.
> > If the *sg is null, that means that the driver call the API
> > virtqueue_get_buf_ctx or virtqueue_get_buf.
> > If the *sg is not null, that means that the driver call the API
> >
> >         void *virtqueue_get_buf_ctx_dma(struct virtqueue *_vq, unsigned int *len,
> >                                         struct scatterlist *sg, void **ctx);
> >
> > Thanks.
>
> Ok, but my question still:
>
> 1) you explain one possible way here is use_dma_api is false and the
> driver doesn't call virtqueue_get_buf_ctx_dma().

If the use_dma_api is false, then we cannot set to premapped mode.
If the driver calls virtqueue_get_buf_ctx_dma(), that works as
virtqueue_get_buf_ctx().


> 2) what happens if the driver uses a preamp but doesn't use
> virtqueue_get_buf_ctx_dma()? Should we warn in this case?

The developer should know that he need to do unmap, so I think
that is not needed.

Thanks.



>
> Thanks
>





[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux