| Message ID | 20230602092206.50108-4-xuanzhuo@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | virtio core prepares for AF_XDP | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Fri, Jun 2, 2023 at 5:22 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > If the vq is the premapped mode, use the sg_dma_address() directly. > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > --- > drivers/virtio/virtio_ring.c | 46 ++++++++++++++++++++++-------------- > 1 file changed, 28 insertions(+), 18 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 2afdfb9e3e30..18212c3e056b 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -598,8 +598,12 @@ 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)) > - goto unmap_release; > + if (vq->premapped) { > + addr = sg_dma_address(sg); > + } else { > + if (vring_map_one_sg(vq, sg, DMA_TO_DEVICE, &addr)) > + goto unmap_release; > + } Btw, I wonder whether or not it would be simple to implement the vq->premapped check inside vring_map_one_sg() assuming the !use_dma_api is done there as well. > > prev = i; > /* Note that we trust indirect descriptor > @@ -614,8 +618,12 @@ 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)) > - goto unmap_release; > + if (vq->premapped) { > + addr = sg_dma_address(sg); > + } else { > + if (vring_map_one_sg(vq, sg, DMA_FROM_DEVICE, &addr)) > + goto unmap_release; > + } > > prev = i; > /* Note that we trust indirect descriptor > @@ -689,21 +697,23 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > return 0; > > unmap_release: > - err_idx = i; > + if (!vq->premapped) { Can vq->premapped be true here? The label is named as "unmap_relase" which implies "map" beforehand which seems not the case for premapping. Thanks > + err_idx = i; > > - if (indirect) > - i = 0; > - else > - i = head; > - > - 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); > + if (indirect) > + i = 0; > + else > + i = head; > + > + 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); > + } > } > > if (indirect) > -- > 2.32.0.3.g01195cf9f >
On Tue, 27 Jun 2023 16:03:26 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Fri, Jun 2, 2023 at 5:22 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > If the vq is the premapped mode, use the sg_dma_address() directly. > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > --- > > drivers/virtio/virtio_ring.c | 46 ++++++++++++++++++++++-------------- > > 1 file changed, 28 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 2afdfb9e3e30..18212c3e056b 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -598,8 +598,12 @@ 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)) > > - goto unmap_release; > > + if (vq->premapped) { > > + addr = sg_dma_address(sg); > > + } else { > > + if (vring_map_one_sg(vq, sg, DMA_TO_DEVICE, &addr)) > > + goto unmap_release; > > + } > > Btw, I wonder whether or not it would be simple to implement the > vq->premapped check inside vring_map_one_sg() assuming the > !use_dma_api is done there as well. YES, That will more simple for the caller. But we will have things like: int func(bool do) { if (!do) return; } I like this way, but you don't like it in last version. > > > > > prev = i; > > /* Note that we trust indirect descriptor > > @@ -614,8 +618,12 @@ 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)) > > - goto unmap_release; > > + if (vq->premapped) { > > + addr = sg_dma_address(sg); > > + } else { > > + if (vring_map_one_sg(vq, sg, DMA_FROM_DEVICE, &addr)) > > + goto unmap_release; > > + } > > > > prev = i; > > /* Note that we trust indirect descriptor > > @@ -689,21 +697,23 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, > > return 0; > > > > unmap_release: > > - err_idx = i; > > + if (!vq->premapped) { > > Can vq->premapped be true here? The label is named as "unmap_relase" > which implies "map" beforehand which seems not the case for > premapping. I see. Rethink about this, there is a better way. I will fix in next version. Thanks. > > Thanks > > > > + err_idx = i; > > > > - if (indirect) > > - i = 0; > > - else > > - i = head; > > - > > - 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); > > + if (indirect) > > + i = 0; > > + else > > + i = head; > > + > > + 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); > > + } > > } > > > > if (indirect) > > -- > > 2.32.0.3.g01195cf9f > > >
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 2afdfb9e3e30..18212c3e056b 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -598,8 +598,12 @@ 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)) - goto unmap_release; + if (vq->premapped) { + addr = sg_dma_address(sg); + } else { + if (vring_map_one_sg(vq, sg, DMA_TO_DEVICE, &addr)) + goto unmap_release; + } prev = i; /* Note that we trust indirect descriptor @@ -614,8 +618,12 @@ 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)) - goto unmap_release; + if (vq->premapped) { + addr = sg_dma_address(sg); + } else { + if (vring_map_one_sg(vq, sg, DMA_FROM_DEVICE, &addr)) + goto unmap_release; + } prev = i; /* Note that we trust indirect descriptor @@ -689,21 +697,23 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, return 0; unmap_release: - err_idx = i; + if (!vq->premapped) { + err_idx = i; - if (indirect) - i = 0; - else - i = head; - - 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); + if (indirect) + i = 0; + else + i = head; + + 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); + } } if (indirect)
If the vq is the premapped mode, use the sg_dma_address() directly. Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> --- drivers/virtio/virtio_ring.c | 46 ++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 18 deletions(-)