Jason Wang
2023-May-18 06:51 UTC
[PATCH vhost v9 05/12] virtio_ring: split: virtqueue_add_split() support premapped
On Wed, May 17, 2023 at 10:23?AM Xuan Zhuo <xuanzhuo at linux.alibaba.com> wrote:> > virtqueue_add_split() only supports virtual addresses, dma is completed > in virtqueue_add_split(). > > In some scenarios (such as the AF_XDP scenario), the memory is allocated > and DMA is completed in advance, so it is necessary for us to support > passing the DMA address to virtqueue_add_split(). > > Record this information in desc_state, we can skip unmap based on this > when executing dma unmap.I would also suggest documenting why a per descriptor metadata is needed instead of a per virtqueue one.> > Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com> > --- > drivers/virtio/virtio_ring.c | 38 +++++++++++++++++++++++++++--------- > 1 file changed, 29 insertions(+), 9 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index e2fc50c05bec..bd5e84afab37 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -70,6 +70,7 @@ > struct vring_desc_state_split { > void *data; /* Data for callback. */ > struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ > + bool premapped; /* DMA mapping is done by driver. */Going back to the original discussion around where this should be placed. I wonder if we can find a common place to store this since it has nothing related to virtqueue layout. Maybe desc_extra? And it would be even better if we can avoid stressing the cache like above.> }; > > struct vring_desc_state_packed { > @@ -356,8 +357,14 @@ static struct device *vring_dma_dev(const struct vring_virtqueue *vq) > > /* Map one sg entry. */ > static int vring_map_one_sg(const struct vring_virtqueue *vq, struct scatterlist *sg, > - enum dma_data_direction direction, static dma_addr_t *addr) > + enum dma_data_direction direction, > + bool premapped, dma_addr_t *addr)having things like: int func(bool do) { if (!do) return; } is a hint that the check needs to be done by the caller? And this change should work for both packed and split. I think we need to squash the packed changes here. Looking at how packed virtqueue uses this in this patch, I don't think this patch can even be built. I will wait for a new version and continue the review from there. Thanks> { > + if (premapped) { > + *addr = sg_dma_address(sg); > + return 0; > + } > + > if (!vq->use_dma_api) { > /* > * If DMA is not used, KMSAN doesn't know that the scatterlist > @@ -445,7 +452,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, bool premapped) > { > struct vring_desc_extra *extra = vq->split.desc_extra; > u16 flags; > @@ -462,6 +469,9 @@ 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 (premapped) > + goto out; > + > dma_unmap_page(vring_dma_dev(vq), > extra[i].addr, > extra[i].len, > @@ -532,6 +542,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > unsigned int in_sgs, > void *data, > void *ctx, > + bool premapped, > gfp_t gfp) > { > struct vring_virtqueue *vq = to_vvq(_vq); > @@ -595,7 +606,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > for (sg = sgs[n]; sg; sg = sg_next(sg)) { > dma_addr_t addr; > > - if (vring_map_one_sg(vq, sg, DMA_TO_DEVICE, &addr)) > + if (vring_map_one_sg(vq, sg, DMA_TO_DEVICE, premapped, &addr)) > goto unmap_release; > > prev = i; > @@ -611,7 +622,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > for (sg = sgs[n]; sg; sg = sg_next(sg)) { > dma_addr_t addr; > > - if (vring_map_one_sg(vq, sg, DMA_FROM_DEVICE, &addr)) > + if (vring_map_one_sg(vq, sg, DMA_FROM_DEVICE, premapped, &addr)) > goto unmap_release; > > prev = i; > @@ -657,6 +668,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > > /* Store token and indirect buffer state. */ > vq->split.desc_state[head].data = data; > + vq->split.desc_state[head].premapped = premapped; > if (indirect) > vq->split.desc_state[head].indir_desc = desc; > else > @@ -686,6 +698,14 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > return 0; > > unmap_release: > + if (premapped) { > + if (indirect) > + kfree(desc); > + > + END_USE(vq); > + return -ENOMEM; > + } > + > err_idx = i; > > if (indirect) > @@ -700,7 +720,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > 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, i, false); > } > > if (indirect) > @@ -757,12 +777,12 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head, > i = head; > > while (vq->split.vring.desc[i].flags & nextflag) { > - vring_unmap_one_split(vq, i); > + vring_unmap_one_split(vq, i, state->premapped); > i = vq->split.desc_extra[i].next; > vq->vq.num_free++; > } > > - vring_unmap_one_split(vq, i); > + vring_unmap_one_split(vq, i, state->premapped); > vq->split.desc_extra[i].next = vq->free_head; > vq->free_head = head; > > @@ -783,7 +803,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head, > VRING_DESC_F_INDIRECT)); > BUG_ON(len == 0 || len % sizeof(struct vring_desc)); > > - if (vq->use_dma_api) { > + if (vq->use_dma_api && !state->premapped) { > for (j = 0; j < len / sizeof(struct vring_desc); j++) > vring_unmap_one_split_indirect(vq, &indir_desc[j]); > } > @@ -2143,7 +2163,7 @@ static inline int virtqueue_add(struct virtqueue *_vq, > return vq->packed_ring ? virtqueue_add_packed(_vq, sgs, total_sg, > out_sgs, in_sgs, data, ctx, gfp) : > virtqueue_add_split(_vq, sgs, total_sg, > - out_sgs, in_sgs, data, ctx, gfp); > + out_sgs, in_sgs, data, ctx, premapped, gfp); > } > > /** > -- > 2.32.0.3.g01195cf9f >
Michael S. Tsirkin
2023-May-18 07:11 UTC
[PATCH vhost v9 05/12] virtio_ring: split: virtqueue_add_split() support premapped
On Thu, May 18, 2023 at 02:51:57PM +0800, Jason Wang wrote:> On Wed, May 17, 2023 at 10:23?AM Xuan Zhuo <xuanzhuo at linux.alibaba.com> wrote: > > > > virtqueue_add_split() only supports virtual addresses, dma is completed > > in virtqueue_add_split(). > > > > In some scenarios (such as the AF_XDP scenario), the memory is allocated > > and DMA is completed in advance, so it is necessary for us to support > > passing the DMA address to virtqueue_add_split(). > > > > Record this information in desc_state, we can skip unmap based on this > > when executing dma unmap. > > I would also suggest documenting why a per descriptor metadata is > needed instead of a per virtqueue one.I think we could make it per virtqueue. That would mean all code in virtio net would have to change to do dma mapping itself instead of relying on virtio core though. Which is maybe a good idea? Definitely a very intrusive change though, will need a lot of performance testing to make sure we don't break anything.> > > > Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com> > > --- > > drivers/virtio/virtio_ring.c | 38 +++++++++++++++++++++++++++--------- > > 1 file changed, 29 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index e2fc50c05bec..bd5e84afab37 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -70,6 +70,7 @@ > > struct vring_desc_state_split { > > void *data; /* Data for callback. */ > > struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ > > + bool premapped; /* DMA mapping is done by driver. */ > > Going back to the original discussion around where this should be > placed. I wonder if we can find a common place to store this since it > has nothing related to virtqueue layout. Maybe desc_extra? And it > would be even better if we can avoid stressing the cache like above. > > > }; > > > > struct vring_desc_state_packed { > > @@ -356,8 +357,14 @@ static struct device *vring_dma_dev(const struct vring_virtqueue *vq) > > > > /* Map one sg entry. */ > > static int vring_map_one_sg(const struct vring_virtqueue *vq, struct scatterlist *sg, > > - enum dma_data_direction direction, static dma_addr_t *addr) > > + enum dma_data_direction direction, > > + bool premapped, dma_addr_t *addr) > > having things like: > > int func(bool do) > { > if (!do) > return; > } > > is a hint that the check needs to be done by the caller? > > And this change should work for both packed and split. I think we need > to squash the packed changes here. > > Looking at how packed virtqueue uses this in this patch, I don't think > this patch can even be built. I will wait for a new version and > continue the review from there. > > Thanks > > > > > { > > + if (premapped) { > > + *addr = sg_dma_address(sg); > > + return 0; > > + } > > + > > if (!vq->use_dma_api) { > > /* > > * If DMA is not used, KMSAN doesn't know that the scatterlist > > @@ -445,7 +452,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, bool premapped) > > { > > struct vring_desc_extra *extra = vq->split.desc_extra; > > u16 flags; > > @@ -462,6 +469,9 @@ 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 (premapped) > > + goto out; > > + > > dma_unmap_page(vring_dma_dev(vq), > > extra[i].addr, > > extra[i].len, > > @@ -532,6 +542,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > > unsigned int in_sgs, > > void *data, > > void *ctx, > > + bool premapped, > > gfp_t gfp) > > { > > struct vring_virtqueue *vq = to_vvq(_vq); > > @@ -595,7 +606,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > > for (sg = sgs[n]; sg; sg = sg_next(sg)) { > > dma_addr_t addr; > > > > - if (vring_map_one_sg(vq, sg, DMA_TO_DEVICE, &addr)) > > + if (vring_map_one_sg(vq, sg, DMA_TO_DEVICE, premapped, &addr)) > > goto unmap_release; > > > > prev = i; > > @@ -611,7 +622,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > > for (sg = sgs[n]; sg; sg = sg_next(sg)) { > > dma_addr_t addr; > > > > - if (vring_map_one_sg(vq, sg, DMA_FROM_DEVICE, &addr)) > > + if (vring_map_one_sg(vq, sg, DMA_FROM_DEVICE, premapped, &addr)) > > goto unmap_release; > > > > prev = i; > > @@ -657,6 +668,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > > > > /* Store token and indirect buffer state. */ > > vq->split.desc_state[head].data = data; > > + vq->split.desc_state[head].premapped = premapped; > > if (indirect) > > vq->split.desc_state[head].indir_desc = desc; > > else > > @@ -686,6 +698,14 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > > return 0; > > > > unmap_release: > > + if (premapped) { > > + if (indirect) > > + kfree(desc); > > + > > + END_USE(vq); > > + return -ENOMEM; > > + } > > + > > err_idx = i; > > > > if (indirect) > > @@ -700,7 +720,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > > 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, i, false); > > } > > > > if (indirect) > > @@ -757,12 +777,12 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head, > > i = head; > > > > while (vq->split.vring.desc[i].flags & nextflag) { > > - vring_unmap_one_split(vq, i); > > + vring_unmap_one_split(vq, i, state->premapped); > > i = vq->split.desc_extra[i].next; > > vq->vq.num_free++; > > } > > > > - vring_unmap_one_split(vq, i); > > + vring_unmap_one_split(vq, i, state->premapped); > > vq->split.desc_extra[i].next = vq->free_head; > > vq->free_head = head; > > > > @@ -783,7 +803,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head, > > VRING_DESC_F_INDIRECT)); > > BUG_ON(len == 0 || len % sizeof(struct vring_desc)); > > > > - if (vq->use_dma_api) { > > + if (vq->use_dma_api && !state->premapped) { > > for (j = 0; j < len / sizeof(struct vring_desc); j++) > > vring_unmap_one_split_indirect(vq, &indir_desc[j]); > > } > > @@ -2143,7 +2163,7 @@ static inline int virtqueue_add(struct virtqueue *_vq, > > return vq->packed_ring ? virtqueue_add_packed(_vq, sgs, total_sg, > > out_sgs, in_sgs, data, ctx, gfp) : > > virtqueue_add_split(_vq, sgs, total_sg, > > - out_sgs, in_sgs, data, ctx, gfp); > > + out_sgs, in_sgs, data, ctx, premapped, gfp); > > } > > > > /** > > -- > > 2.32.0.3.g01195cf9f > >