Re: [PATCH vhost v11 03/10] virtio_ring: introduce virtqueue_set_premapped()
On Thu, Jul 13, 2023 at 10:47:23AM -0400, Michael S. Tsirkin wrote: > There are a gazillion virtio drivers and most of them just use the > virtio API, without bothering with these micro-optimizations. virtio > already tracks addresses so mapping/unmapping them for DMA is easier > done in the core. It's only networking and only with XDP where the > difference becomes measureable. Yes, but now you two differing code paths (which then branch into another two with the fake DMA mappings). I'm really worried about the madness that follows like the USB dma mapping code that is a constant soure of trouble. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vhost v11 03/10] virtio_ring: introduce virtqueue_set_premapped()
On Thu, Jul 13, 2023 at 04:14:45AM -0700, Christoph Hellwig wrote: > On Mon, Jul 10, 2023 at 11:42:30AM +0800, Xuan Zhuo wrote: > > This helper allows the driver change the dma mode to premapped mode. > > Under the premapped mode, the virtio core do not do dma mapping > > internally. > > > > This just work when the use_dma_api is true. If the use_dma_api is false, > > the dma options is not through the DMA APIs, that is not the standard > > way of the linux kernel. > > I have a hard time parsing this. Me too unfortunately. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vhost v11 03/10] virtio_ring: introduce virtqueue_set_premapped()
On Thu, Jul 13, 2023 at 04:14:45AM -0700, Christoph Hellwig wrote: > On Mon, Jul 10, 2023 at 11:42:30AM +0800, Xuan Zhuo wrote: > > This helper allows the driver change the dma mode to premapped mode. > > Under the premapped mode, the virtio core do not do dma mapping > > internally. > > > > This just work when the use_dma_api is true. If the use_dma_api is false, > > the dma options is not through the DMA APIs, that is not the standard > > way of the linux kernel. > > I have a hard time parsing this. > > More importantly having two modes seems very error prone going down > the route. If the premapping is so important, why don't we do it > always? There are a gazillion virtio drivers and most of them just use the virtio API, without bothering with these micro-optimizations. virtio already tracks addresses so mapping/unmapping them for DMA is easier done in the core. It's only networking and only with XDP where the difference becomes measureable. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vhost v11 03/10] virtio_ring: introduce virtqueue_set_premapped()
On Mon, Jul 10, 2023 at 11:42:30AM +0800, Xuan Zhuo wrote: > This helper allows the driver change the dma mode to premapped mode. > Under the premapped mode, the virtio core do not do dma mapping > internally. > > This just work when the use_dma_api is true. If the use_dma_api is false, > the dma options is not through the DMA APIs, that is not the standard > way of the linux kernel. I have a hard time parsing this. More importantly having two modes seems very error prone going down the route. If the premapping is so important, why don't we do it always? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vhost v11 03/10] virtio_ring: introduce virtqueue_set_premapped()
On Wed, 12 Jul 2023 16:24:18 +0800, Jason Wang wrote:
> On Mon, Jul 10, 2023 at 11:42 AM Xuan Zhuo
> wrote:
>
> > This helper allows the driver change the dma mode to premapped mode.
> > Under the premapped mode, the virtio core do not do dma mapping
> > internally.
> >
> > This just work when the use_dma_api is true. If the use_dma_api is false,
> > the dma options is not through the DMA APIs, that is not the standard
> > way of the linux kernel.
> >
> > Signed-off-by: Xuan Zhuo
> > ---
> > drivers/virtio/virtio_ring.c | 45
> > include/linux/virtio.h | 2 ++
> > 2 files changed, 47 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 87d7ceeecdbd..5ace4539344c 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -172,6 +172,9 @@ struct vring_virtqueue {
> > /* Host publishes avail event idx */
> > bool event;
> >
> > + /* Do DMA mapping by driver */
> > + bool premapped;
> > +
> > /* Head of free buffer list. */
> > unsigned int free_head;
> > /* Number we've added since last sync. */
> > @@ -2061,6 +2064,7 @@ static struct virtqueue
> > *vring_create_virtqueue_packed(
> > vq->packed_ring = true;
> > vq->dma_dev = dma_dev;
> > vq->use_dma_api = vring_use_dma_api(vdev);
> > + vq->premapped = false;
> >
> > vq->indirect = virtio_has_feature(vdev,
> > VIRTIO_RING_F_INDIRECT_DESC) &&
> > !context;
> > @@ -2550,6 +2554,7 @@ static struct virtqueue
> > *__vring_new_virtqueue(unsigned int index,
> > #endif
> > vq->dma_dev = dma_dev;
> > vq->use_dma_api = vring_use_dma_api(vdev);
> > + vq->premapped = false;
> >
> > vq->indirect = virtio_has_feature(vdev,
> > VIRTIO_RING_F_INDIRECT_DESC) &&
> > !context;
> > @@ -2693,6 +2698,46 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > }
> > EXPORT_SYMBOL_GPL(virtqueue_resize);
> >
> > +/**
> > + * virtqueue_set_premapped - set the vring premapped mode
> > + * @_vq: the struct virtqueue we're talking about.
> > + *
> > + * Enable the premapped mode of the vq.
> > + *
> > + * The vring in premapped mode does not do dma internally, so the driver
> > must
> > + * do dma mapping in advance. The driver must pass the dma_address through
> > + * dma_address of scatterlist. When the driver got a used buffer from
> > + * the vring, it has to unmap the dma address.
> > + *
> > + * This function must be called immediately after creating the vq, or
> > after vq
> > + * reset, and before adding any buffers to it.
> > + *
> > + * Caller must ensure we don't call this with other virtqueue operations
> > + * at the same time (except where noted).
> > + *
> > + * Returns zero or a negative error.
> > + * 0: success.
> > + * -EINVAL: vring does not use the dma api, so we can not enable
> > premapped mode.
> > + */
> > +int virtqueue_set_premapped(struct virtqueue *_vq)
> > +{
> > + struct vring_virtqueue *vq = to_vvq(_vq);
> > + u32 num;
> > +
> > + num = vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
> > +
> > + if (num != vq->vq.num_free)
> > + return -EINVAL;
> >
>
> If we check this, I think we need to protect this with
> START_USE()/END_USE().
YES.
>
>
> > +
> > + if (!vq->use_dma_api)
> > + return -EINVAL;
> >
>
> Not a native spreak, but I think "dma_premapped" is better than "premapped"
> as "dma_premapped" implies "use_dma_api".
I am ok to fix this.
Thanks.
>
> Thanks
>
>
> > +
> > + vq->premapped = true;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(virtqueue_set_premapped);
> > +
> > /* Only available for split ring */
> > struct virtqueue *vring_new_virtqueue(unsigned int index,
> > unsigned int num,
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index de6041deee37..2efd07b79ecf 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -78,6 +78,8 @@ bool virtqueue_enable_cb(struct virtqueue *vq);
> >
> > unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq);
> >
> > +int virtqueue_set_premapped(struct virtqueue *_vq);
> > +
> > bool virtqueue_poll(struct virtqueue *vq, unsigned);
> >
> > bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
> > --
> > 2.32.0.3.g01195cf9f
> >
> >
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vhost v11 03/10] virtio_ring: introduce virtqueue_set_premapped()
On Mon, Jul 10, 2023 at 11:42 AM Xuan Zhuo
wrote:
> This helper allows the driver change the dma mode to premapped mode.
> Under the premapped mode, the virtio core do not do dma mapping
> internally.
>
> This just work when the use_dma_api is true. If the use_dma_api is false,
> the dma options is not through the DMA APIs, that is not the standard
> way of the linux kernel.
>
> Signed-off-by: Xuan Zhuo
> ---
> drivers/virtio/virtio_ring.c | 45
> include/linux/virtio.h | 2 ++
> 2 files changed, 47 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 87d7ceeecdbd..5ace4539344c 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -172,6 +172,9 @@ struct vring_virtqueue {
> /* Host publishes avail event idx */
> bool event;
>
> + /* Do DMA mapping by driver */
> + bool premapped;
> +
> /* Head of free buffer list. */
> unsigned int free_head;
> /* Number we've added since last sync. */
> @@ -2061,6 +2064,7 @@ static struct virtqueue
> *vring_create_virtqueue_packed(
> vq->packed_ring = true;
> vq->dma_dev = dma_dev;
> vq->use_dma_api = vring_use_dma_api(vdev);
> + vq->premapped = false;
>
> vq->indirect = virtio_has_feature(vdev,
> VIRTIO_RING_F_INDIRECT_DESC) &&
> !context;
> @@ -2550,6 +2554,7 @@ static struct virtqueue
> *__vring_new_virtqueue(unsigned int index,
> #endif
> vq->dma_dev = dma_dev;
> vq->use_dma_api = vring_use_dma_api(vdev);
> + vq->premapped = false;
>
> vq->indirect = virtio_has_feature(vdev,
> VIRTIO_RING_F_INDIRECT_DESC) &&
> !context;
> @@ -2693,6 +2698,46 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> }
> EXPORT_SYMBOL_GPL(virtqueue_resize);
>
> +/**
> + * virtqueue_set_premapped - set the vring premapped mode
> + * @_vq: the struct virtqueue we're talking about.
> + *
> + * Enable the premapped mode of the vq.
> + *
> + * The vring in premapped mode does not do dma internally, so the driver
> must
> + * do dma mapping in advance. The driver must pass the dma_address through
> + * dma_address of scatterlist. When the driver got a used buffer from
> + * the vring, it has to unmap the dma address.
> + *
> + * This function must be called immediately after creating the vq, or
> after vq
> + * reset, and before adding any buffers to it.
> + *
> + * Caller must ensure we don't call this with other virtqueue operations
> + * at the same time (except where noted).
> + *
> + * Returns zero or a negative error.
> + * 0: success.
> + * -EINVAL: vring does not use the dma api, so we can not enable
> premapped mode.
> + */
> +int virtqueue_set_premapped(struct virtqueue *_vq)
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + u32 num;
> +
> + num = vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
> +
> + if (num != vq->vq.num_free)
> + return -EINVAL;
>
If we check this, I think we need to protect this with
START_USE()/END_USE().
> +
> + if (!vq->use_dma_api)
> + return -EINVAL;
>
Not a native spreak, but I think "dma_premapped" is better than "premapped"
as "dma_premapped" implies "use_dma_api".
Thanks
> +
> + vq->premapped = true;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_set_premapped);
> +
> /* Only available for split ring */
> struct virtqueue *vring_new_virtqueue(unsigned int index,
> unsigned int num,
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index de6041deee37..2efd07b79ecf 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -78,6 +78,8 @@ bool virtqueue_enable_cb(struct virtqueue *vq);
>
> unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq);
>
> +int virtqueue_set_premapped(struct virtqueue *_vq);
> +
> bool virtqueue_poll(struct virtqueue *vq, unsigned);
>
> bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
> --
> 2.32.0.3.g01195cf9f
>
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
