Re: [PATCH vhost 1/6] virtio_ring: introduce dma map api for page

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Thu, 11 Apr 2024 13:45:28 +0200, Alexander Lobakin <aleksander.lobakin@intel.com> wrote:
> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Date: Thu, 11 Apr 2024 10:51:22 +0800
>
> > The virtio-net big mode sq will use these APIs to map the pages.
> >
> > dma_addr_t virtqueue_dma_map_page_attrs(struct virtqueue *_vq, struct page *page,
> >                                        size_t offset, size_t size,
> >                                        enum dma_data_direction dir,
> >                                        unsigned long attrs);
> > void virtqueue_dma_unmap_page_attrs(struct virtqueue *_vq, dma_addr_t addr,
> >                                    size_t size, enum dma_data_direction dir,
> >                                    unsigned long attrs);
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  drivers/virtio/virtio_ring.c | 52 ++++++++++++++++++++++++++++++++++++
> >  include/linux/virtio.h       |  7 +++++
> >  2 files changed, 59 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 70de1a9a81a3..1b9fb680cff3 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -3100,6 +3100,58 @@ void virtqueue_dma_unmap_single_attrs(struct virtqueue *_vq, dma_addr_t addr,
> >  }
> >  EXPORT_SYMBOL_GPL(virtqueue_dma_unmap_single_attrs);
> >
> > +/**
> > + * virtqueue_dma_map_page_attrs - map DMA for _vq
> > + * @_vq: the struct virtqueue we're talking about.
> > + * @page: the page to do dma
> > + * @offset: the offset inside the page
> > + * @size: the size of the page to do dma
> > + * @dir: DMA direction
> > + * @attrs: DMA Attrs
> > + *
> > + * The caller calls this to do dma mapping in advance. The DMA address can be
> > + * passed to this _vq when it is in pre-mapped mode.
> > + *
> > + * return DMA address. Caller should check that by virtqueue_dma_mapping_error().
> > + */
> > +dma_addr_t virtqueue_dma_map_page_attrs(struct virtqueue *_vq, struct page *page,
> > +					size_t offset, size_t size,
> > +					enum dma_data_direction dir,
> > +					unsigned long attrs)
> > +{
> > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > +
> > +	if (!vq->use_dma_api)
> > +		return page_to_phys(page) + offset;
>
> page_to_phys() and the actual page DMA address may differ. See
> page_to_dma()/virt_to_dma(). I believe this is not correct.


For the virtio, if use_dma_api is false, we do not try to get the
dma address. We try to get the physical address.


>
> > +
> > +	return dma_map_page_attrs(vring_dma_dev(vq), page, offset, size, dir, attrs);
> > +}
> > +EXPORT_SYMBOL_GPL(virtqueue_dma_map_page_attrs);
>
> Could you try make these functions static inlines and run bloat-o-meter?
> They seem to be small and probably you'd get better performance.

YES.

But struct vring_virtqueue is in the .c file, we must do that with moving
structure to the .h file.

I plan to do that in the future.

Thanks



>
> > +
> > +/**
> > + * virtqueue_dma_unmap_page_attrs - unmap DMA for _vq
> > + * @_vq: the struct virtqueue we're talking about.
> > + * @addr: the dma address to unmap
> > + * @size: the size of the buffer
> > + * @dir: DMA direction
> > + * @attrs: DMA Attrs
> > + *
> > + * Unmap the address that is mapped by the virtqueue_dma_map_* APIs.
> > + *
> > + */
> > +void virtqueue_dma_unmap_page_attrs(struct virtqueue *_vq, dma_addr_t addr,
> > +				    size_t size, enum dma_data_direction dir,
> > +				    unsigned long attrs)
> > +{
> > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > +
> > +	if (!vq->use_dma_api)
> > +		return;
> > +
> > +	dma_unmap_page_attrs(vring_dma_dev(vq), addr, size, dir, attrs);
> > +}
> > +EXPORT_SYMBOL_GPL(virtqueue_dma_unmap_page_attrs);
> > +
> >  /**
> >   * virtqueue_dma_mapping_error - check dma address
> >   * @_vq: the struct virtqueue we're talking about.
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index 26c4325aa373..d6c699553979 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -228,6 +228,13 @@ dma_addr_t virtqueue_dma_map_single_attrs(struct virtqueue *_vq, void *ptr, size
> >  void virtqueue_dma_unmap_single_attrs(struct virtqueue *_vq, dma_addr_t addr,
> >  				      size_t size, enum dma_data_direction dir,
> >  				      unsigned long attrs);
> > +dma_addr_t virtqueue_dma_map_page_attrs(struct virtqueue *_vq, struct page *page,
> > +					size_t offset, size_t size,
> > +					enum dma_data_direction dir,
> > +					unsigned long attrs);
> > +void virtqueue_dma_unmap_page_attrs(struct virtqueue *_vq, dma_addr_t addr,
> > +				    size_t size, enum dma_data_direction dir,
> > +				    unsigned long attrs);
> >  int virtqueue_dma_mapping_error(struct virtqueue *_vq, dma_addr_t addr);
> >
> >  bool virtqueue_dma_need_sync(struct virtqueue *_vq, dma_addr_t addr);
>
> Thanks,
> Olek