[PATCH vhost v13 00/12] virtio core prepares for AF_XDP

[PATCH vhost v13 00/12] virtio core prepares for AF_XDP

[Xuan Zhuo]

## About DMA APIs

Now, virtio may can not work with DMA APIs when virtio features do not have
VIRTIO_F_ACCESS_PLATFORM.

1. I tried to let DMA APIs return phy address by virtio-device. But DMA APIs just
   work with the "real" devices.
2. I tried to let xsk support callballs to get phy address from virtio-net
   driver as the dma address. But the maintainers of xsk may want to use dma-buf
   to replace the DMA APIs. I think that may be a larger effort. We will wait
   too long.

So rethinking this, firstly, we can support premapped-dma only for devices with
VIRTIO_F_ACCESS_PLATFORM. In the case of af-xdp, if the users want to use it,
they have to update the device to support VIRTIO_F_RING_RESET, and they can also
enable the device's VIRTIO_F_ACCESS_PLATFORM feature.

Thanks for the help from Christoph.

## For AF_XDP

XDP socket(AF_XDP) is an excellent bypass kernel network framework. The zero
copy feature of xsk (XDP socket) needs to be supported by the driver. The
performance of zero copy is very good.

ENV: Qemu with vhost.

                   vhost cpu | Guest APP CPU |Guest Softirq CPU | PPS
-----------------------------|---------------|------------------|------------
xmit by sockperf:     90%    |   100%        |                  |  318967
xmit by xsk:          100%   |   30%         |   33%            | 1192064
recv by sockperf:     100%   |   68%         |   100%           |  692288
recv by xsk:          100%   |   33%         |   43%            |  771670

Before achieving the function of Virtio-Net, we also have to let virtio core
support these features:

1. virtio core support premapped
2. virtio core support reset per-queue

## VirtioNET rx dma merge

After introducing premapping, I added an example to virtio-net. virtio-net can
merge dma mappings through this feature. @Jason

kernel command line: intel_iommu=on iommu.passthrough=0

           |  strict=0  | strict=1
    Before |  775496pps | 428614pps
    After  | 1109316pps | 742853pps

Please review.

Thanks.

v13:
 1. virtio-net uses the virtqueue_dma_* APIs
 2. virtio-net unmap with the flags DMA_ATTR_SKIP_CPU_SYNC

v12:
 1. Alloc dma info from the alloc frag. Avoid alloc array to store the dma info.
 2. rename virtqueue_set_premapped() to virtqueue_set_dma_premapped()

v11
 1. virtio-net merges dma operates based on the feature premapped
 2. A better way to handle the map error with the premapped

v10:
 1. support to set vq to premapped mode, then the vq just handles the premapped request.
 2. virtio-net support to do dma mapping in advance

v9:
 1. use flag to distinguish the premapped operations. no do judgment by sg.

v8:
 1. vring_sg_address: check by sg_page(sg) not dma_address. Because 0 is a valid dma address
 2. remove unused code from vring_map_one_sg()

v7:
 1. virtqueue_dma_dev() return NULL when virtio is without DMA API.

v6:
 1. change the size of the flags to u32.

v5:
 1. fix for error handler
 2. add flags to record internal dma mapping

v4:
 1. rename map_inter to dma_map_internal
 2. fix: Excess function parameter 'vq' description in 'virtqueue_dma_dev'

v3:
 1. add map_inter to struct desc state to reocrd whether virtio core do dma map

v2:
 1. based on sgs[0]->dma_address to judgment is premapped
 2. based on extra.addr to judgment to do unmap for no-indirect desc
 3. based on indir_desc to judgment to do unmap for indirect desc
 4. rename virtqueue_get_dma_dev to virtqueue_dma_dev

v1:
 1. expose dma device. NO introduce the api for dma and sync
 2. split some commit for review.







Xuan Zhuo (12):
  virtio_ring: check use_dma_api before unmap desc for indirect
  virtio_ring: put mapping error check in vring_map_one_sg
  virtio_ring: introduce virtqueue_set_dma_premapped()
  virtio_ring: support add premapped buf
  virtio_ring: introduce virtqueue_dma_dev()
  virtio_ring: skip unmap for premapped
  virtio_ring: correct the expression of the description of
    virtqueue_resize()
  virtio_ring: separate the logic of reset/enable from virtqueue_resize
  virtio_ring: introduce virtqueue_reset()
  virtio_ring: introduce dma map api for virtqueue
  virtio_ring: introduce dma sync api for virtqueue
  virtio_net: merge dma operations when filling mergeable buffers

 drivers/net/virtio_net.c     | 228 ++++++++++++++++---
 drivers/virtio/virtio_ring.c | 410 ++++++++++++++++++++++++++++++-----
 include/linux/virtio.h       |  22 ++
 3 files changed, 582 insertions(+), 78 deletions(-)

--
2.32.0.3.g01195cf9f

[PATCH vhost v13 01/12] virtio_ring: check use_dma_api before unmap desc for indirect

[Xuan Zhuo]

Inside detach_buf_split(), if use_dma_api is false,
vring_unmap_one_split_indirect will be called many times, but actually
nothing is done. So this patch check use_dma_api firstly.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index c5310eaf8b46..f8754f1d64d3 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -774,8 +774,10 @@ 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));
 
-		for (j = 0; j < len / sizeof(struct vring_desc); j++)
-			vring_unmap_one_split_indirect(vq, &indir_desc[j]);
+		if (vq->use_dma_api) {
+			for (j = 0; j < len / sizeof(struct vring_desc); j++)
+				vring_unmap_one_split_indirect(vq, &indir_desc[j]);
+		}
 
 		kfree(indir_desc);
 		vq->split.desc_state[head].indir_desc = NULL;
-- 
2.32.0.3.g01195cf9f

[PATCH vhost v13 02/12] virtio_ring: put mapping error check in vring_map_one_sg

[Xuan Zhuo]

This patch put the dma addr error check in vring_map_one_sg().

The benefits of doing this:

1. reduce one judgment of vq->use_dma_api.
2. make vring_map_one_sg more simple, without calling
   vring_mapping_error to check the return value. simplifies subsequent
   code

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 37 +++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index f8754f1d64d3..87d7ceeecdbd 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -355,9 +355,8 @@ static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
 }
 
 /* Map one sg entry. */
-static dma_addr_t vring_map_one_sg(const struct vring_virtqueue *vq,
-				   struct scatterlist *sg,
-				   enum dma_data_direction direction)
+static int vring_map_one_sg(const struct vring_virtqueue *vq, struct scatterlist *sg,
+			    enum dma_data_direction direction, dma_addr_t *addr)
 {
 	if (!vq->use_dma_api) {
 		/*
@@ -366,7 +365,8 @@ static dma_addr_t vring_map_one_sg(const struct vring_virtqueue *vq,
 		 * depending on the direction.
 		 */
 		kmsan_handle_dma(sg_page(sg), sg->offset, sg->length, direction);
-		return (dma_addr_t)sg_phys(sg);
+		*addr = (dma_addr_t)sg_phys(sg);
+		return 0;
 	}
 
 	/*
@@ -374,9 +374,14 @@ static dma_addr_t vring_map_one_sg(const struct vring_virtqueue *vq,
 	 * the way it expects (we don't guarantee that the scatterlist
 	 * will exist for the lifetime of the mapping).
 	 */
-	return dma_map_page(vring_dma_dev(vq),
+	*addr = dma_map_page(vring_dma_dev(vq),
 			    sg_page(sg), sg->offset, sg->length,
 			    direction);
+
+	if (dma_mapping_error(vring_dma_dev(vq), *addr))
+		return -ENOMEM;
+
+	return 0;
 }
 
 static dma_addr_t vring_map_single(const struct vring_virtqueue *vq,
@@ -588,8 +593,9 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 
 	for (n = 0; n < out_sgs; n++) {
 		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
-			dma_addr_t addr = vring_map_one_sg(vq, sg, DMA_TO_DEVICE);
-			if (vring_mapping_error(vq, addr))
+			dma_addr_t addr;
+
+			if (vring_map_one_sg(vq, sg, DMA_TO_DEVICE, &addr))
 				goto unmap_release;
 
 			prev = i;
@@ -603,8 +609,9 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 	}
 	for (; n < (out_sgs + in_sgs); n++) {
 		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
-			dma_addr_t addr = vring_map_one_sg(vq, sg, DMA_FROM_DEVICE);
-			if (vring_mapping_error(vq, addr))
+			dma_addr_t addr;
+
+			if (vring_map_one_sg(vq, sg, DMA_FROM_DEVICE, &addr))
 				goto unmap_release;
 
 			prev = i;
@@ -1281,9 +1288,8 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 
 	for (n = 0; n < out_sgs + in_sgs; n++) {
 		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
-			addr = vring_map_one_sg(vq, sg, n < out_sgs ?
-					DMA_TO_DEVICE : DMA_FROM_DEVICE);
-			if (vring_mapping_error(vq, addr))
+			if (vring_map_one_sg(vq, sg, n < out_sgs ?
+					     DMA_TO_DEVICE : DMA_FROM_DEVICE, &addr))
 				goto unmap_release;
 
 			desc[i].flags = cpu_to_le16(n < out_sgs ?
@@ -1428,9 +1434,10 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 	c = 0;
 	for (n = 0; n < out_sgs + in_sgs; n++) {
 		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
-			dma_addr_t addr = vring_map_one_sg(vq, sg, n < out_sgs ?
-					DMA_TO_DEVICE : DMA_FROM_DEVICE);
-			if (vring_mapping_error(vq, addr))
+			dma_addr_t addr;
+
+			if (vring_map_one_sg(vq, sg, n < out_sgs ?
+					     DMA_TO_DEVICE : DMA_FROM_DEVICE, &addr))
 				goto unmap_release;
 
 			flags = cpu_to_le16(vq->packed.avail_used_flags |
-- 
2.32.0.3.g01195cf9f

[PATCH vhost v13 03/12] virtio_ring: introduce virtqueue_set_dma_premapped()

[Xuan Zhuo]

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 <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 53 ++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h       |  2 ++
 2 files changed, 55 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 87d7ceeecdbd..8e81b01e0735 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,54 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
 }
 EXPORT_SYMBOL_GPL(virtqueue_resize);
 
+/**
+ * virtqueue_set_dma_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_dma_premapped(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+	u32 num;
+
+	START_USE(vq);
+
+	num = vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
+
+	if (num != vq->vq.num_free) {
+		END_USE(vq);
+		return -EINVAL;
+	}
+
+	if (!vq->use_dma_api) {
+		END_USE(vq);
+		return -EINVAL;
+	}
+
+	vq->premapped = true;
+
+	END_USE(vq);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(virtqueue_set_dma_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..8add38038877 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_dma_premapped(struct virtqueue *_vq);
+
 bool virtqueue_poll(struct virtqueue *vq, unsigned);
 
 bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
-- 
2.32.0.3.g01195cf9f

[PATCH vhost v13 04/12] virtio_ring: support add premapped buf

[Xuan Zhuo]

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 | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 8e81b01e0735..f9f772e85a38 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -361,6 +361,11 @@ static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
 static int vring_map_one_sg(const struct vring_virtqueue *vq, struct scatterlist *sg,
 			    enum dma_data_direction direction, dma_addr_t *addr)
 {
+	if (vq->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
@@ -639,8 +644,12 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 		dma_addr_t addr = vring_map_single(
 			vq, desc, total_sg * sizeof(struct vring_desc),
 			DMA_TO_DEVICE);
-		if (vring_mapping_error(vq, addr))
+		if (vring_mapping_error(vq, addr)) {
+			if (vq->premapped)
+				goto free_indirect;
+
 			goto unmap_release;
+		}
 
 		virtqueue_add_desc_split(_vq, vq->split.vring.desc,
 					 head, addr,
@@ -706,6 +715,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 			i = vring_unmap_one_split(vq, i);
 	}
 
+free_indirect:
 	if (indirect)
 		kfree(desc);
 
@@ -1307,8 +1317,12 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 	addr = vring_map_single(vq, desc,
 			total_sg * sizeof(struct vring_packed_desc),
 			DMA_TO_DEVICE);
-	if (vring_mapping_error(vq, addr))
+	if (vring_mapping_error(vq, addr)) {
+		if (vq->premapped)
+			goto free_desc;
+
 		goto unmap_release;
+	}
 
 	vq->packed.vring.desc[head].addr = cpu_to_le64(addr);
 	vq->packed.vring.desc[head].len = cpu_to_le32(total_sg *
@@ -1366,6 +1380,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 	for (i = 0; i < err_idx; i++)
 		vring_unmap_desc_packed(vq, &desc[i]);
 
+free_desc:
 	kfree(desc);
 
 	END_USE(vq);
-- 
2.32.0.3.g01195cf9f

Re: [PATCH vhost v13 04/12] virtio_ring: support add premapped buf

[Ilya Leoshkevich]

On Thu, 2023-08-10 at 20:30 +0800, Xuan Zhuo 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 | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c
> b/drivers/virtio/virtio_ring.c
> index 8e81b01e0735..f9f772e85a38 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -361,6 +361,11 @@ static struct device *vring_dma_dev(const struct
> vring_virtqueue *vq)
>  static int vring_map_one_sg(const struct vring_virtqueue *vq, struct
> scatterlist *sg,
>  			    enum dma_data_direction direction,
> dma_addr_t *addr)
>  {
> +	if (vq->premapped) {
> +		*addr = sg_dma_address(sg);
> +		return 0;
> +	}
> +

I wonder if something needs to be done for KMSAN here, like it's done
by the next block in this function? I'm looking into what seems to be a
KMSAN false positive on s390x:

BUG: KMSAN: uninit-value in receive_buf+0x45ca/0x6990             
 receive_buf+0x45ca/0x6990                                            
 virtnet_poll+0x17e0/0x3130                                           
 net_rx_action+0x832/0x26e0                                           
 handle_softirqs+0x330/0x10f0                                         
 [...]
                                                                                                         
Uninit was created at:                                                
 __alloc_pages_noprof+0x62a/0xe60                                     
 alloc_pages_noprof+0x392/0x830                                       
 skb_page_frag_refill+0x21a/0x5c0                                     
 virtnet_rq_alloc+0x50/0x1500                                         
 try_fill_recv+0x372/0x54c0                                           
 virtnet_open+0x210/0xbe0                                             
 __dev_open+0x56e/0x920                                               
 __dev_change_flags+0x39c/0x2000                                      
 dev_change_flags+0xaa/0x200                                          
 do_setlink+0x197a/0x7420                                             
 rtnl_setlink+0x77c/0x860                                             
 [...]

My understanding is that virtnet_rq_alloc() allocates a page for
receiving data from a virtio device, which is then wrapped in struct
scatterlist by virtnet_rq_init_one_sg(), which is in turn associated
with a virtqueue through the virtqueue_add_inbuf_ctx() ->
virtqueue_add() -> virtqueue_add_split() -> vring_map_one_sg()
call chain.

Someone should unpoison this page (since KMSAN doesn't know that the
hypervisor writes to it), and today for the non-premapped case this is
vring_map_one_sg(). So I tried the following naive fix:

        if (vq->premapped) {
                *addr = sg_dma_address(sg);
+               if (!vq->use_dma_api) {
+                       kmsan_handle_dma(phys_to_page(*addr), sg-
>offset, sg->length, direction);
+               }

but it didn't help. I plan to investigate this further, but any hints
are much appreciated.

>  	if (!vq->use_dma_api) {
>  		/*
>  		 * If DMA is not used, KMSAN doesn't know that the
> scatterlist

Re: [PATCH vhost v13 04/12] virtio_ring: support add premapped buf

[Alexander Potapenko]

On Tue, Jun 4, 2024 at 6:07 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Thu, 2023-08-10 at 20:30 +0800, Xuan Zhuo 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 | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c
> > b/drivers/virtio/virtio_ring.c
> > index 8e81b01e0735..f9f772e85a38 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -361,6 +361,11 @@ static struct device *vring_dma_dev(const struct
> > vring_virtqueue *vq)
> >  static int vring_map_one_sg(const struct vring_virtqueue *vq, struct
> > scatterlist *sg,
> >                           enum dma_data_direction direction,
> > dma_addr_t *addr)
> >  {
> > +     if (vq->premapped) {
> > +             *addr = sg_dma_address(sg);
> > +             return 0;
> > +     }
> > +
>
> I wonder if something needs to be done for KMSAN here, like it's done
> by the next block in this function? I'm looking into what seems to be a
> KMSAN false positive on s390x:
>
> BUG: KMSAN: uninit-value in receive_buf+0x45ca/0x6990
>  receive_buf+0x45ca/0x6990
>  virtnet_poll+0x17e0/0x3130
>  net_rx_action+0x832/0x26e0
>  handle_softirqs+0x330/0x10f0
>  [...]

I think there's a similar problem on x86 as well:
https://syzkaller.appspot.com/bug?extid=c5336dcd1b741349d27a

I was going to look closer this week.

Re: [PATCH vhost v13 04/12] virtio_ring: support add premapped buf

[Ilya Leoshkevich]

On Tue, 2024-06-04 at 18:17 +0200, Alexander Potapenko wrote:
> On Tue, Jun 4, 2024 at 6:07 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On Thu, 2023-08-10 at 20:30 +0800, Xuan Zhuo 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 | 19 +++++++++++++++++--
> > >  1 file changed, 17 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/virtio/virtio_ring.c
> > > b/drivers/virtio/virtio_ring.c
> > > index 8e81b01e0735..f9f772e85a38 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -361,6 +361,11 @@ static struct device *vring_dma_dev(const
> > > struct
> > > vring_virtqueue *vq)
> > >  static int vring_map_one_sg(const struct vring_virtqueue *vq,
> > > struct
> > > scatterlist *sg,
> > >                           enum dma_data_direction direction,
> > > dma_addr_t *addr)
> > >  {
> > > +     if (vq->premapped) {
> > > +             *addr = sg_dma_address(sg);
> > > +             return 0;
> > > +     }
> > > +
> > 
> > I wonder if something needs to be done for KMSAN here, like it's
> > done
> > by the next block in this function? I'm looking into what seems to
> > be a
> > KMSAN false positive on s390x:
> > 
> > BUG: KMSAN: uninit-value in receive_buf+0x45ca/0x6990
> >  receive_buf+0x45ca/0x6990
> >  virtnet_poll+0x17e0/0x3130
> >  net_rx_action+0x832/0x26e0
> >  handle_softirqs+0x330/0x10f0
> >  [...]
> 
> I think there's a similar problem on x86 as well:
> https://syzkaller.appspot.com/bug?extid=c5336dcd1b741349d27a
> 
> I was going to look closer this week.

Thanks! I bisected it in the meantime and the first failing commit is:

commit f9dac92ba9081062a6477ee015bd3b8c5914efc4
Author: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Date:   Sat May 11 11:14:01 2024 +0800

    virtio_ring: enable premapped mode whatever use_dma_api

so it's definitely related.

Re: [PATCH vhost v13 04/12] virtio_ring: support add premapped buf

[Xuan Zhuo]

On Tue, 04 Jun 2024 18:07:44 +0200, Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> On Thu, 2023-08-10 at 20:30 +0800, Xuan Zhuo 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 | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c
> > b/drivers/virtio/virtio_ring.c
> > index 8e81b01e0735..f9f772e85a38 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -361,6 +361,11 @@ static struct device *vring_dma_dev(const struct
> > vring_virtqueue *vq)
> >  static int vring_map_one_sg(const struct vring_virtqueue *vq, struct
> > scatterlist *sg,
> >  			    enum dma_data_direction direction,
> > dma_addr_t *addr)
> >  {
> > +	if (vq->premapped) {
> > +		*addr = sg_dma_address(sg);
> > +		return 0;
> > +	}
> > +
>
> I wonder if something needs to be done for KMSAN here, like it's done
> by the next block in this function? I'm looking into what seems to be a
> KMSAN false positive on s390x:
>
> BUG: KMSAN: uninit-value in receive_buf+0x45ca/0x6990
>  receive_buf+0x45ca/0x6990
>  virtnet_poll+0x17e0/0x3130
>  net_rx_action+0x832/0x26e0
>  handle_softirqs+0x330/0x10f0
>  [...]
>
> Uninit was created at:
>  __alloc_pages_noprof+0x62a/0xe60
>  alloc_pages_noprof+0x392/0x830
>  skb_page_frag_refill+0x21a/0x5c0
>  virtnet_rq_alloc+0x50/0x1500
>  try_fill_recv+0x372/0x54c0
>  virtnet_open+0x210/0xbe0
>  __dev_open+0x56e/0x920
>  __dev_change_flags+0x39c/0x2000
>  dev_change_flags+0xaa/0x200
>  do_setlink+0x197a/0x7420
>  rtnl_setlink+0x77c/0x860
>  [...]
>
> My understanding is that virtnet_rq_alloc() allocates a page for
> receiving data from a virtio device, which is then wrapped in struct
> scatterlist by virtnet_rq_init_one_sg(), which is in turn associated
> with a virtqueue through the virtqueue_add_inbuf_ctx() ->
> virtqueue_add() -> virtqueue_add_split() -> vring_map_one_sg()
> call chain.
>
> Someone should unpoison this page (since KMSAN doesn't know that the
> hypervisor writes to it), and today for the non-premapped case this is
> vring_map_one_sg(). So I tried the following naive fix:
>
>         if (vq->premapped) {
>                 *addr = sg_dma_address(sg);
> +               if (!vq->use_dma_api) {
> +                       kmsan_handle_dma(phys_to_page(*addr), sg-
> >offset, sg->length, direction);
> +               }
>
> but it didn't help. I plan to investigate this further, but any hints
> are much appreciated.
>
> >  	if (!vq->use_dma_api) {
> >  		/*
> >  		 * If DMA is not used, KMSAN doesn't know that the
> > scatterlist


Could you try this?

Thanks.

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 37c9c5b55864..cb280b66c7a2 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -3119,8 +3119,10 @@ dma_addr_t virtqueue_dma_map_single_attrs(struct virtqueue *_vq, void *ptr,
 {
        struct vring_virtqueue *vq = to_vvq(_vq);

-       if (!vq->use_dma_api)
+       if (!vq->use_dma_api) {
+               kmsan_handle_dma(virt_to_page(ptr), offset_in_page(ptr), size, dir);
                return (dma_addr_t)virt_to_phys(ptr);
+       }

        return dma_map_single_attrs(vring_dma_dev(vq), ptr, size, dir, attrs);
 }
@@ -3171,8 +3173,10 @@ dma_addr_t virtqueue_dma_map_page_attrs(struct virtqueue *_vq, struct page *page
 {
        struct vring_virtqueue *vq = to_vvq(_vq);

-       if (!vq->use_dma_api)
+       if (!vq->use_dma_api) {
+               kmsan_handle_dma(page, offset, size, dir);
                return page_to_phys(page) + offset;
+       }

        return dma_map_page_attrs(vring_dma_dev(vq), page, offset, size, dir, attrs);
 }

Re: [PATCH vhost v13 04/12] virtio_ring: support add premapped buf

[Xuan Zhuo]

On Thu, 6 Jun 2024 10:20:21 +0200, Alexander Potapenko <glider@google.com> wrote:
> > Could you try this?
>
> Hi Xuan,
>
> What kernel revision does this patch apply to? I tried it against
> v6.10-rc2, and only the first hunk applied.
> However this seems to fix the problem, at least the kernel boots without
> warnings now.


Sorry, I have some changes locally.

If the hunk #1 is applied, then it is ok.

Do you think we need more test? Or I post an new patch directly.

Thanks.


>
> > Thanks.
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 37c9c5b55864..cb280b66c7a2 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -3119,8 +3119,10 @@ dma_addr_t virtqueue_dma_map_single_attrs(struct
> virtqueue *_vq, void *ptr,
> >  {
> >         struct vring_virtqueue *vq = to_vvq(_vq);
> >
> > -       if (!vq->use_dma_api)
> > +       if (!vq->use_dma_api) {
> > +               kmsan_handle_dma(virt_to_page(ptr), offset_in_page(ptr),
> size, dir);
> >                 return (dma_addr_t)virt_to_phys(ptr);
> > +       }
> >
> >         return dma_map_single_attrs(vring_dma_dev(vq), ptr, size, dir,
> attrs);
> >  }
> > @@ -3171,8 +3173,10 @@ dma_addr_t virtqueue_dma_map_page_attrs(struct
> virtqueue *_vq, struct page *page
> >  {
> >         struct vring_virtqueue *vq = to_vvq(_vq);
> >
> > -       if (!vq->use_dma_api)
> > +       if (!vq->use_dma_api) {
> > +               kmsan_handle_dma(page, offset, size, dir);
> >                 return page_to_phys(page) + offset;
> > +       }
> >
> >         return dma_map_page_attrs(vring_dma_dev(vq), page, offset, size,
> dir, attrs);
> >  }
>
>
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Paul Manicle, Liana Sebastian
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
>

Re: [PATCH vhost v13 04/12] virtio_ring: support add premapped buf

[Alexander Potapenko]

On Thu, Jun 6, 2024 at 10:27 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Thu, 6 Jun 2024 10:20:21 +0200, Alexander Potapenko <glider@google.com> wrote:
> > > Could you try this?
> >
> > Hi Xuan,
> >
> > What kernel revision does this patch apply to? I tried it against
> > v6.10-rc2, and only the first hunk applied.
> > However this seems to fix the problem, at least the kernel boots without
> > warnings now.
>
>
> Sorry, I have some changes locally.
>
> If the hunk #1 is applied, then it is ok.
>
> Do you think we need more test? Or I post an new patch directly.

I think we're good to go (just double-checked that). Assuming you're
sending the following patch:

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 2a972752ff1bc..9d3a9942c8c82 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -3121,8 +3121,10 @@ dma_addr_t
virtqueue_dma_map_single_attrs(struct virtqueue *_vq, void *ptr,
 {
        struct vring_virtqueue *vq = to_vvq(_vq);

-       if (!vq->use_dma_api)
+       if (!vq->use_dma_api) {
+               kmsan_handle_dma(virt_to_page(ptr),
offset_in_page(ptr), size, dir);
                return (dma_addr_t)virt_to_phys(ptr);
+       }

        return dma_map_single_attrs(vring_dma_dev(vq), ptr, size, dir, attrs);
 }

, feel free to add:

Tested-by: Alexander Potapenko <glider@google.com>

Re: [PATCH vhost v13 04/12] virtio_ring: support add premapped buf

[Alexander Potapenko]

>
> Could you try this?

(resending without HTML, sorry for inconvenience).
Hi Xuan,

What kernel revision does this patch apply to? I tried it against
v6.10-rc2, and only the first hunk applied.
However this seems to fix the problem, at least the kernel boots
without warnings now.

> Thanks.
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 37c9c5b55864..cb280b66c7a2 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -3119,8 +3119,10 @@ dma_addr_t virtqueue_dma_map_single_attrs(struct virtqueue *_vq, void *ptr,
>  {
>         struct vring_virtqueue *vq = to_vvq(_vq);
>
> -       if (!vq->use_dma_api)
> +       if (!vq->use_dma_api) {
> +               kmsan_handle_dma(virt_to_page(ptr), offset_in_page(ptr), size, dir);
>                 return (dma_addr_t)virt_to_phys(ptr);
> +       }
>
>         return dma_map_single_attrs(vring_dma_dev(vq), ptr, size, dir, attrs);
>  }
> @@ -3171,8 +3173,10 @@ dma_addr_t virtqueue_dma_map_page_attrs(struct virtqueue *_vq, struct page *page
>  {
>         struct vring_virtqueue *vq = to_vvq(_vq);
>
> -       if (!vq->use_dma_api)
> +       if (!vq->use_dma_api) {
> +               kmsan_handle_dma(page, offset, size, dir);
>                 return page_to_phys(page) + offset;
> +       }
>
>         return dma_map_page_attrs(vring_dma_dev(vq), page, offset, size, dir, attrs);
>  }

[PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Xuan Zhuo]

Added virtqueue_dma_dev() to get DMA device for virtio. Then the
caller can do dma operation in advance. The purpose is to keep memory
mapped across multiple add/get buf operations.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 17 +++++++++++++++++
 include/linux/virtio.h       |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index f9f772e85a38..bb3d73d221cd 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2265,6 +2265,23 @@ int virtqueue_add_inbuf_ctx(struct virtqueue *vq,
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_inbuf_ctx);
 
+/**
+ * virtqueue_dma_dev - get the dma dev
+ * @_vq: the struct virtqueue we're talking about.
+ *
+ * Returns the dma dev. That can been used for dma api.
+ */
+struct device *virtqueue_dma_dev(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+
+	if (vq->use_dma_api)
+		return vring_dma_dev(vq);
+	else
+		return NULL;
+}
+EXPORT_SYMBOL_GPL(virtqueue_dma_dev);
+
 /**
  * virtqueue_kick_prepare - first half of split virtqueue_kick call.
  * @_vq: the struct virtqueue
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 8add38038877..bd55a05eec04 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -61,6 +61,8 @@ int virtqueue_add_sgs(struct virtqueue *vq,
 		      void *data,
 		      gfp_t gfp);
 
+struct device *virtqueue_dma_dev(struct virtqueue *vq);
+
 bool virtqueue_kick(struct virtqueue *vq);
 
 bool virtqueue_kick_prepare(struct virtqueue *vq);
-- 
2.32.0.3.g01195cf9f

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Jason Wang]

On Thu, Aug 10, 2023 at 8:31 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> Added virtqueue_dma_dev() to get DMA device for virtio. Then the
> caller can do dma operation in advance. The purpose is to keep memory
> mapped across multiple add/get buf operations.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Acked-by: Jason Wang <jasowang@redhat.com>

So I think we don't have actual users for this in this series? Can we
simply have another independent patch for this?

> ---
>  drivers/virtio/virtio_ring.c | 17 +++++++++++++++++
>  include/linux/virtio.h       |  2 ++
>  2 files changed, 19 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index f9f772e85a38..bb3d73d221cd 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2265,6 +2265,23 @@ int virtqueue_add_inbuf_ctx(struct virtqueue *vq,
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_add_inbuf_ctx);
>
> +/**
> + * virtqueue_dma_dev - get the dma dev
> + * @_vq: the struct virtqueue we're talking about.
> + *
> + * Returns the dma dev. That can been used for dma api.
> + */
> +struct device *virtqueue_dma_dev(struct virtqueue *_vq)
> +{
> +       struct vring_virtqueue *vq = to_vvq(_vq);
> +
> +       if (vq->use_dma_api)
> +               return vring_dma_dev(vq);
> +       else
> +               return NULL;
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_dma_dev);

One possible concern is that exporting things like NULL may result in
the switch in the caller (driver). I wonder if it's better to do
BUG_ON() in the path of NULL?

Thanks

> +
>  /**
>   * virtqueue_kick_prepare - first half of split virtqueue_kick call.
>   * @_vq: the struct virtqueue
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 8add38038877..bd55a05eec04 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -61,6 +61,8 @@ int virtqueue_add_sgs(struct virtqueue *vq,
>                       void *data,
>                       gfp_t gfp);
>
> +struct device *virtqueue_dma_dev(struct virtqueue *vq);
> +
>  bool virtqueue_kick(struct virtqueue *vq);
>
>  bool virtqueue_kick_prepare(struct virtqueue *vq);
> --
> 2.32.0.3.g01195cf9f
>

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Xuan Zhuo]

On Mon, 14 Aug 2023 11:05:49 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Thu, Aug 10, 2023 at 8:31 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > Added virtqueue_dma_dev() to get DMA device for virtio. Then the
> > caller can do dma operation in advance. The purpose is to keep memory
> > mapped across multiple add/get buf operations.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
>
> So I think we don't have actual users for this in this series? Can we
> simply have another independent patch for this?

I am ok. I will remove this from the next version.

But I also help merge this to 6.6. Then we can let the virtio-net to support
AF_XDP in 6.7+.


>
> > ---
> >  drivers/virtio/virtio_ring.c | 17 +++++++++++++++++
> >  include/linux/virtio.h       |  2 ++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index f9f772e85a38..bb3d73d221cd 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -2265,6 +2265,23 @@ int virtqueue_add_inbuf_ctx(struct virtqueue *vq,
> >  }
> >  EXPORT_SYMBOL_GPL(virtqueue_add_inbuf_ctx);
> >
> > +/**
> > + * virtqueue_dma_dev - get the dma dev
> > + * @_vq: the struct virtqueue we're talking about.
> > + *
> > + * Returns the dma dev. That can been used for dma api.
> > + */
> > +struct device *virtqueue_dma_dev(struct virtqueue *_vq)
> > +{
> > +       struct vring_virtqueue *vq = to_vvq(_vq);
> > +
> > +       if (vq->use_dma_api)
> > +               return vring_dma_dev(vq);
> > +       else
> > +               return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(virtqueue_dma_dev);
>
> One possible concern is that exporting things like NULL may result in
> the switch in the caller (driver). I wonder if it's better to do
> BUG_ON() in the path of NULL?


I agree.

But we need a new helper to tell the driver(or AF_XDP) that the device support
ACCESS_PLATFORM or not.

We need a switch, but we can make the switch is irrelevant to the DMA.

Thanks.



>
> Thanks
>
> > +
> >  /**
> >   * virtqueue_kick_prepare - first half of split virtqueue_kick call.
> >   * @_vq: the struct virtqueue
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index 8add38038877..bd55a05eec04 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -61,6 +61,8 @@ int virtqueue_add_sgs(struct virtqueue *vq,
> >                       void *data,
> >                       gfp_t gfp);
> >
> > +struct device *virtqueue_dma_dev(struct virtqueue *vq);
> > +
> >  bool virtqueue_kick(struct virtqueue *vq);
> >
> >  bool virtqueue_kick_prepare(struct virtqueue *vq);
> > --
> > 2.32.0.3.g01195cf9f
> >
>

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Michael S. Tsirkin]

On Mon, Aug 14, 2023 at 04:56:53PM +0800, Xuan Zhuo wrote:
> On Mon, 14 Aug 2023 11:05:49 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Thu, Aug 10, 2023 at 8:31 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > Added virtqueue_dma_dev() to get DMA device for virtio. Then the
> > > caller can do dma operation in advance. The purpose is to keep memory
> > > mapped across multiple add/get buf operations.
> > >
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > Acked-by: Jason Wang <jasowang@redhat.com>
> >
> > So I think we don't have actual users for this in this series? Can we
> > simply have another independent patch for this?
> 
> I am ok. I will remove this from the next version.
> 
> But I also help merge this to 6.6. Then we can let the virtio-net to support
> AF_XDP in 6.7+.

Is there going to be a next version? Because if yes it will be too late for the next release.
if all you want to do is drop this patch then just say so, no need
for another version.

> 
> >
> > > ---
> > >  drivers/virtio/virtio_ring.c | 17 +++++++++++++++++
> > >  include/linux/virtio.h       |  2 ++
> > >  2 files changed, 19 insertions(+)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index f9f772e85a38..bb3d73d221cd 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -2265,6 +2265,23 @@ int virtqueue_add_inbuf_ctx(struct virtqueue *vq,
> > >  }
> > >  EXPORT_SYMBOL_GPL(virtqueue_add_inbuf_ctx);
> > >
> > > +/**
> > > + * virtqueue_dma_dev - get the dma dev
> > > + * @_vq: the struct virtqueue we're talking about.
> > > + *
> > > + * Returns the dma dev. That can been used for dma api.
> > > + */
> > > +struct device *virtqueue_dma_dev(struct virtqueue *_vq)
> > > +{
> > > +       struct vring_virtqueue *vq = to_vvq(_vq);
> > > +
> > > +       if (vq->use_dma_api)
> > > +               return vring_dma_dev(vq);
> > > +       else
> > > +               return NULL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(virtqueue_dma_dev);
> >
> > One possible concern is that exporting things like NULL may result in
> > the switch in the caller (driver). I wonder if it's better to do
> > BUG_ON() in the path of NULL?
> 
> 
> I agree.
> 
> But we need a new helper to tell the driver(or AF_XDP) that the device support
> ACCESS_PLATFORM or not.
> 
> We need a switch, but we can make the switch is irrelevant to the DMA.
> 
> Thanks.
> 
> 
> 
> >
> > Thanks
> >
> > > +
> > >  /**
> > >   * virtqueue_kick_prepare - first half of split virtqueue_kick call.
> > >   * @_vq: the struct virtqueue
> > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > index 8add38038877..bd55a05eec04 100644
> > > --- a/include/linux/virtio.h
> > > +++ b/include/linux/virtio.h
> > > @@ -61,6 +61,8 @@ int virtqueue_add_sgs(struct virtqueue *vq,
> > >                       void *data,
> > >                       gfp_t gfp);
> > >
> > > +struct device *virtqueue_dma_dev(struct virtqueue *vq);
> > > +
> > >  bool virtqueue_kick(struct virtqueue *vq);
> > >
> > >  bool virtqueue_kick_prepare(struct virtqueue *vq);
> > > --
> > > 2.32.0.3.g01195cf9f
> > >
> >

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Xuan Zhuo]

On Mon, 14 Aug 2023 07:24:59 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Aug 14, 2023 at 04:56:53PM +0800, Xuan Zhuo wrote:
> > On Mon, 14 Aug 2023 11:05:49 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Thu, Aug 10, 2023 at 8:31 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > Added virtqueue_dma_dev() to get DMA device for virtio. Then the
> > > > caller can do dma operation in advance. The purpose is to keep memory
> > > > mapped across multiple add/get buf operations.
> > > >
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > >
> > > So I think we don't have actual users for this in this series? Can we
> > > simply have another independent patch for this?
> >
> > I am ok. I will remove this from the next version.
> >
> > But I also help merge this to 6.6. Then we can let the virtio-net to support
> > AF_XDP in 6.7+.
>
> Is there going to be a next version? Because if yes it will be too late for the next release.
> if all you want to do is drop this patch then just say so, no need
> for another version.


For me, I want that this patch can be merged to 6.6. Because that the AF_XDP
needs this.

Thanks.


>
> >
> > >
> > > > ---
> > > >  drivers/virtio/virtio_ring.c | 17 +++++++++++++++++
> > > >  include/linux/virtio.h       |  2 ++
> > > >  2 files changed, 19 insertions(+)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index f9f772e85a38..bb3d73d221cd 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -2265,6 +2265,23 @@ int virtqueue_add_inbuf_ctx(struct virtqueue *vq,
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(virtqueue_add_inbuf_ctx);
> > > >
> > > > +/**
> > > > + * virtqueue_dma_dev - get the dma dev
> > > > + * @_vq: the struct virtqueue we're talking about.
> > > > + *
> > > > + * Returns the dma dev. That can been used for dma api.
> > > > + */
> > > > +struct device *virtqueue_dma_dev(struct virtqueue *_vq)
> > > > +{
> > > > +       struct vring_virtqueue *vq = to_vvq(_vq);
> > > > +
> > > > +       if (vq->use_dma_api)
> > > > +               return vring_dma_dev(vq);
> > > > +       else
> > > > +               return NULL;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(virtqueue_dma_dev);
> > >
> > > One possible concern is that exporting things like NULL may result in
> > > the switch in the caller (driver). I wonder if it's better to do
> > > BUG_ON() in the path of NULL?
> >
> >
> > I agree.
> >
> > But we need a new helper to tell the driver(or AF_XDP) that the device support
> > ACCESS_PLATFORM or not.
> >
> > We need a switch, but we can make the switch is irrelevant to the DMA.
> >
> > Thanks.
> >
> >
> >
> > >
> > > Thanks
> > >
> > > > +
> > > >  /**
> > > >   * virtqueue_kick_prepare - first half of split virtqueue_kick call.
> > > >   * @_vq: the struct virtqueue
> > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > > index 8add38038877..bd55a05eec04 100644
> > > > --- a/include/linux/virtio.h
> > > > +++ b/include/linux/virtio.h
> > > > @@ -61,6 +61,8 @@ int virtqueue_add_sgs(struct virtqueue *vq,
> > > >                       void *data,
> > > >                       gfp_t gfp);
> > > >
> > > > +struct device *virtqueue_dma_dev(struct virtqueue *vq);
> > > > +
> > > >  bool virtqueue_kick(struct virtqueue *vq);
> > > >
> > > >  bool virtqueue_kick_prepare(struct virtqueue *vq);
> > > > --
> > > > 2.32.0.3.g01195cf9f
> > > >
> > >
>

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Xuan Zhuo]

Hi, Jason

Could you skip this patch?

Let we review other patches firstly?

Thanks.

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Jason Wang]

On Tue, Aug 15, 2023 at 2:32 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
>
> Hi, Jason
>
> Could you skip this patch?

I'm fine with either merging or dropping this.

>
> Let we review other patches firstly?

I will be on vacation soon, and won't have time to do this until next week.

But I spot two possible "issues":

1) the DMA metadata were stored in the headroom of the page, this
breaks frags coalescing, we need to benchmark it's impact
2) pre mapped DMA addresses were not reused in the case of XDP_TX/XDP_REDIRECT

I see Michael has merge this series so I'm fine to let it go first.

Thanks

>
> Thanks.
>

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Xuan Zhuo]

On Tue, 15 Aug 2023 15:50:23 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Aug 15, 2023 at 2:32 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> >
> > Hi, Jason
> >
> > Could you skip this patch?
>
> I'm fine with either merging or dropping this.
>
> >
> > Let we review other patches firstly?
>
> I will be on vacation soon, and won't have time to do this until next week.

Have a happly vacation.

>
> But I spot two possible "issues":
>
> 1) the DMA metadata were stored in the headroom of the page, this
> breaks frags coalescing, we need to benchmark it's impact

Not every page, just the first page of the COMP pages.

So I think there is no impact.


> 2) pre mapped DMA addresses were not reused in the case of XDP_TX/XDP_REDIRECT

Because that the tx is not the premapped mode.

Thanks.

>
> I see Michael has merge this series so I'm fine to let it go first.
>
> Thanks
>
> >
> > Thanks.
> >
>

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Jason Wang]

On Tue, Aug 15, 2023 at 5:40 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 15 Aug 2023 15:50:23 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Tue, Aug 15, 2023 at 2:32 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > >
> > > Hi, Jason
> > >
> > > Could you skip this patch?
> >
> > I'm fine with either merging or dropping this.
> >
> > >
> > > Let we review other patches firstly?
> >
> > I will be on vacation soon, and won't have time to do this until next week.
>
> Have a happly vacation.
>
> >
> > But I spot two possible "issues":
> >
> > 1) the DMA metadata were stored in the headroom of the page, this
> > breaks frags coalescing, we need to benchmark it's impact
>
> Not every page, just the first page of the COMP pages.
>
> So I think there is no impact.

Nope, see this:

        if (SKB_FRAG_PAGE_ORDER &&
            !static_branch_unlikely(&net_high_order_alloc_disable_key)) {
                /* Avoid direct reclaim but allow kswapd to wake */
                pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) |
                                          __GFP_COMP | __GFP_NOWARN |
                                          __GFP_NORETRY,
                                          SKB_FRAG_PAGE_ORDER);
                if (likely(pfrag->page)) {
                        pfrag->size = PAGE_SIZE << SKB_FRAG_PAGE_ORDER;
                        return true;
                }
        }

The comp page might be disabled due to the SKB_FRAG_PAGE_ORDER and
net_high_order_alloc_disable_key.

>
>
> > 2) pre mapped DMA addresses were not reused in the case of XDP_TX/XDP_REDIRECT
>
> Because that the tx is not the premapped mode.

Yes, we can optimize this on top.

Thanks

>
> Thanks.
>
> >
> > I see Michael has merge this series so I'm fine to let it go first.
> >
> > Thanks
> >
> > >
> > > Thanks.
> > >
> >
>

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Xuan Zhuo]

On Wed, 16 Aug 2023 09:13:48 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Aug 15, 2023 at 5:40 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Tue, 15 Aug 2023 15:50:23 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Tue, Aug 15, 2023 at 2:32 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > >
> > > > Hi, Jason
> > > >
> > > > Could you skip this patch?
> > >
> > > I'm fine with either merging or dropping this.
> > >
> > > >
> > > > Let we review other patches firstly?
> > >
> > > I will be on vacation soon, and won't have time to do this until next week.
> >
> > Have a happly vacation.
> >
> > >
> > > But I spot two possible "issues":
> > >
> > > 1) the DMA metadata were stored in the headroom of the page, this
> > > breaks frags coalescing, we need to benchmark it's impact
> >
> > Not every page, just the first page of the COMP pages.
> >
> > So I think there is no impact.
>
> Nope, see this:
>
>         if (SKB_FRAG_PAGE_ORDER &&
>             !static_branch_unlikely(&net_high_order_alloc_disable_key)) {
>                 /* Avoid direct reclaim but allow kswapd to wake */
>                 pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) |
>                                           __GFP_COMP | __GFP_NOWARN |
>                                           __GFP_NORETRY,
>                                           SKB_FRAG_PAGE_ORDER);
>                 if (likely(pfrag->page)) {
>                         pfrag->size = PAGE_SIZE << SKB_FRAG_PAGE_ORDER;
>                         return true;
>                 }
>         }
>
> The comp page might be disabled due to the SKB_FRAG_PAGE_ORDER and
> net_high_order_alloc_disable_key.


YES.

But if comp page is disabled. Then we only get one page each time. The pages are
not contiguous, so we don't have frags coalescing.

If you mean the two pages got from alloc_page may be contiguous. The coalescing
may then be broken. It's a possibility, but I think the impact will be small.

Thanks.


>
> >
> >
> > > 2) pre mapped DMA addresses were not reused in the case of XDP_TX/XDP_REDIRECT
> >
> > Because that the tx is not the premapped mode.
>
> Yes, we can optimize this on top.
>
> Thanks
>
> >
> > Thanks.
> >
> > >
> > > I see Michael has merge this series so I'm fine to let it go first.
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks.
> > > >
> > >
> >
>

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Jason Wang]

On Wed, Aug 16, 2023 at 10:16 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Wed, 16 Aug 2023 09:13:48 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Tue, Aug 15, 2023 at 5:40 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Tue, 15 Aug 2023 15:50:23 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > On Tue, Aug 15, 2023 at 2:32 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > >
> > > > >
> > > > > Hi, Jason
> > > > >
> > > > > Could you skip this patch?
> > > >
> > > > I'm fine with either merging or dropping this.
> > > >
> > > > >
> > > > > Let we review other patches firstly?
> > > >
> > > > I will be on vacation soon, and won't have time to do this until next week.
> > >
> > > Have a happly vacation.
> > >
> > > >
> > > > But I spot two possible "issues":
> > > >
> > > > 1) the DMA metadata were stored in the headroom of the page, this
> > > > breaks frags coalescing, we need to benchmark it's impact
> > >
> > > Not every page, just the first page of the COMP pages.
> > >
> > > So I think there is no impact.
> >
> > Nope, see this:
> >
> >         if (SKB_FRAG_PAGE_ORDER &&
> >             !static_branch_unlikely(&net_high_order_alloc_disable_key)) {
> >                 /* Avoid direct reclaim but allow kswapd to wake */
> >                 pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) |
> >                                           __GFP_COMP | __GFP_NOWARN |
> >                                           __GFP_NORETRY,
> >                                           SKB_FRAG_PAGE_ORDER);
> >                 if (likely(pfrag->page)) {
> >                         pfrag->size = PAGE_SIZE << SKB_FRAG_PAGE_ORDER;
> >                         return true;
> >                 }
> >         }
> >
> > The comp page might be disabled due to the SKB_FRAG_PAGE_ORDER and
> > net_high_order_alloc_disable_key.
>
>
> YES.
>
> But if comp page is disabled. Then we only get one page each time. The pages are
> not contiguous, so we don't have frags coalescing.
>
> If you mean the two pages got from alloc_page may be contiguous. The coalescing
> may then be broken. It's a possibility, but I think the impact will be small.

Let's have a simple benchmark and see?

Thanks

>
> Thanks.
>
>
> >
> > >
> > >
> > > > 2) pre mapped DMA addresses were not reused in the case of XDP_TX/XDP_REDIRECT
> > >
> > > Because that the tx is not the premapped mode.
> >
> > Yes, we can optimize this on top.
> >
> > Thanks
> >
> > >
> > > Thanks.
> > >
> > > >
> > > > I see Michael has merge this series so I'm fine to let it go first.
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks.
> > > > >
> > > >
> > >
> >
>

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Xuan Zhuo]

On Wed, 16 Aug 2023 10:19:34 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Aug 16, 2023 at 10:16 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Wed, 16 Aug 2023 09:13:48 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Tue, Aug 15, 2023 at 5:40 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > On Tue, 15 Aug 2023 15:50:23 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > On Tue, Aug 15, 2023 at 2:32 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > >
> > > > > >
> > > > > > Hi, Jason
> > > > > >
> > > > > > Could you skip this patch?
> > > > >
> > > > > I'm fine with either merging or dropping this.
> > > > >
> > > > > >
> > > > > > Let we review other patches firstly?
> > > > >
> > > > > I will be on vacation soon, and won't have time to do this until next week.
> > > >
> > > > Have a happly vacation.
> > > >
> > > > >
> > > > > But I spot two possible "issues":
> > > > >
> > > > > 1) the DMA metadata were stored in the headroom of the page, this
> > > > > breaks frags coalescing, we need to benchmark it's impact
> > > >
> > > > Not every page, just the first page of the COMP pages.
> > > >
> > > > So I think there is no impact.
> > >
> > > Nope, see this:
> > >
> > >         if (SKB_FRAG_PAGE_ORDER &&
> > >             !static_branch_unlikely(&net_high_order_alloc_disable_key)) {
> > >                 /* Avoid direct reclaim but allow kswapd to wake */
> > >                 pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) |
> > >                                           __GFP_COMP | __GFP_NOWARN |
> > >                                           __GFP_NORETRY,
> > >                                           SKB_FRAG_PAGE_ORDER);
> > >                 if (likely(pfrag->page)) {
> > >                         pfrag->size = PAGE_SIZE << SKB_FRAG_PAGE_ORDER;
> > >                         return true;
> > >                 }
> > >         }
> > >
> > > The comp page might be disabled due to the SKB_FRAG_PAGE_ORDER and
> > > net_high_order_alloc_disable_key.
> >
> >
> > YES.
> >
> > But if comp page is disabled. Then we only get one page each time. The pages are
> > not contiguous, so we don't have frags coalescing.
> >
> > If you mean the two pages got from alloc_page may be contiguous. The coalescing
> > may then be broken. It's a possibility, but I think the impact will be small.
>
> Let's have a simple benchmark and see?


That is ok.

I think you want to know the perf num with big traffic and the comp page
disabled.

Thanks.


>
> Thanks
>
> >
> > Thanks.
> >
> >
> > >
> > > >
> > > >
> > > > > 2) pre mapped DMA addresses were not reused in the case of XDP_TX/XDP_REDIRECT
> > > >
> > > > Because that the tx is not the premapped mode.
> > >
> > > Yes, we can optimize this on top.
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks.
> > > >
> > > > >
> > > > > I see Michael has merge this series so I'm fine to let it go first.
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Jason Wang]

On Wed, Aug 16, 2023 at 10:24 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Wed, 16 Aug 2023 10:19:34 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Wed, Aug 16, 2023 at 10:16 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Wed, 16 Aug 2023 09:13:48 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > On Tue, Aug 15, 2023 at 5:40 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > >
> > > > > On Tue, 15 Aug 2023 15:50:23 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > On Tue, Aug 15, 2023 at 2:32 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > > Hi, Jason
> > > > > > >
> > > > > > > Could you skip this patch?
> > > > > >
> > > > > > I'm fine with either merging or dropping this.
> > > > > >
> > > > > > >
> > > > > > > Let we review other patches firstly?
> > > > > >
> > > > > > I will be on vacation soon, and won't have time to do this until next week.
> > > > >
> > > > > Have a happly vacation.
> > > > >
> > > > > >
> > > > > > But I spot two possible "issues":
> > > > > >
> > > > > > 1) the DMA metadata were stored in the headroom of the page, this
> > > > > > breaks frags coalescing, we need to benchmark it's impact
> > > > >
> > > > > Not every page, just the first page of the COMP pages.
> > > > >
> > > > > So I think there is no impact.
> > > >
> > > > Nope, see this:
> > > >
> > > >         if (SKB_FRAG_PAGE_ORDER &&
> > > >             !static_branch_unlikely(&net_high_order_alloc_disable_key)) {
> > > >                 /* Avoid direct reclaim but allow kswapd to wake */
> > > >                 pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) |
> > > >                                           __GFP_COMP | __GFP_NOWARN |
> > > >                                           __GFP_NORETRY,
> > > >                                           SKB_FRAG_PAGE_ORDER);
> > > >                 if (likely(pfrag->page)) {
> > > >                         pfrag->size = PAGE_SIZE << SKB_FRAG_PAGE_ORDER;
> > > >                         return true;
> > > >                 }
> > > >         }
> > > >
> > > > The comp page might be disabled due to the SKB_FRAG_PAGE_ORDER and
> > > > net_high_order_alloc_disable_key.
> > >
> > >
> > > YES.
> > >
> > > But if comp page is disabled. Then we only get one page each time. The pages are
> > > not contiguous, so we don't have frags coalescing.
> > >
> > > If you mean the two pages got from alloc_page may be contiguous. The coalescing
> > > may then be broken. It's a possibility, but I think the impact will be small.
> >
> > Let's have a simple benchmark and see?
>
>
> That is ok.
>
> I think you want to know the perf num with big traffic and the comp page
> disabled.

Yes.

Thanks

>
> Thanks.
>
>
> >
> > Thanks
> >
> > >
> > > Thanks.
> > >
> > >
> > > >
> > > > >
> > > > >
> > > > > > 2) pre mapped DMA addresses were not reused in the case of XDP_TX/XDP_REDIRECT
> > > > >
> > > > > Because that the tx is not the premapped mode.
> > > >
> > > > Yes, we can optimize this on top.
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thanks.
> > > > >
> > > > > >
> > > > > > I see Michael has merge this series so I'm fine to let it go first.
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Xuan Zhuo]

On Wed, 16 Aug 2023 10:33:34 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Aug 16, 2023 at 10:24 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Wed, 16 Aug 2023 10:19:34 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Wed, Aug 16, 2023 at 10:16 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > On Wed, 16 Aug 2023 09:13:48 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > On Tue, Aug 15, 2023 at 5:40 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > >
> > > > > > On Tue, 15 Aug 2023 15:50:23 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > > > On Tue, Aug 15, 2023 at 2:32 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > Hi, Jason
> > > > > > > >
> > > > > > > > Could you skip this patch?
> > > > > > >
> > > > > > > I'm fine with either merging or dropping this.
> > > > > > >
> > > > > > > >
> > > > > > > > Let we review other patches firstly?
> > > > > > >
> > > > > > > I will be on vacation soon, and won't have time to do this until next week.
> > > > > >
> > > > > > Have a happly vacation.
> > > > > >
> > > > > > >
> > > > > > > But I spot two possible "issues":
> > > > > > >
> > > > > > > 1) the DMA metadata were stored in the headroom of the page, this
> > > > > > > breaks frags coalescing, we need to benchmark it's impact
> > > > > >
> > > > > > Not every page, just the first page of the COMP pages.
> > > > > >
> > > > > > So I think there is no impact.
> > > > >
> > > > > Nope, see this:
> > > > >
> > > > >         if (SKB_FRAG_PAGE_ORDER &&
> > > > >             !static_branch_unlikely(&net_high_order_alloc_disable_key)) {
> > > > >                 /* Avoid direct reclaim but allow kswapd to wake */
> > > > >                 pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) |
> > > > >                                           __GFP_COMP | __GFP_NOWARN |
> > > > >                                           __GFP_NORETRY,
> > > > >                                           SKB_FRAG_PAGE_ORDER);
> > > > >                 if (likely(pfrag->page)) {
> > > > >                         pfrag->size = PAGE_SIZE << SKB_FRAG_PAGE_ORDER;
> > > > >                         return true;
> > > > >                 }
> > > > >         }
> > > > >
> > > > > The comp page might be disabled due to the SKB_FRAG_PAGE_ORDER and
> > > > > net_high_order_alloc_disable_key.
> > > >
> > > >
> > > > YES.
> > > >
> > > > But if comp page is disabled. Then we only get one page each time. The pages are
> > > > not contiguous, so we don't have frags coalescing.
> > > >
> > > > If you mean the two pages got from alloc_page may be contiguous. The coalescing
> > > > may then be broken. It's a possibility, but I think the impact will be small.
> > >
> > > Let's have a simple benchmark and see?
> >
> >
> > That is ok.
> >
> > I think you want to know the perf num with big traffic and the comp page
> > disabled.
>
> Yes.


Hi,

Host:
	for ((i=0; i < 10; ++i)) do sockperf tp -i 192.168.122.100 -t 1000  -m 64000& done
Guest:
	03:23:12 AM     IFACE   rxpck/s   txpck/s    rxkB/s    txkB/s   rxcmp/s   txcmp/s  rxmcst/s   %ifutil
	03:23:13 AM        lo      0.00      0.00      0.00      0.00      0.00      0.00      0.00      0.00
	03:23:13 AM      ens4  61848.00      1.00 3868036.73      0.58      0.00      0.00      0.00      0.00

	tcpdump:
		03:25:01.741563 IP 192.168.122.1.29693 > 192.168.122.100.11111: UDP, length 64000
		03:25:01.741580 IP 192.168.122.1.22239 > 192.168.122.100.11111: UDP, length 64000
		03:25:01.741623 IP 192.168.122.1.22396 > 192.168.122.100.11111: UDP, length 64000

The Guest CPU util is low, every packet is 64000. But the Host vhost process is
100%. So we can not judge by the traffic or the cpu of the Guest.

So I use the kernel without my patches 0635819decaf9d60e6cacfecfebfabe3cbdddafb.

I want to count the frags coalescing num when the comp page is disabled.

	$ sh -x test.sh
	+ sysctl -w net.core.high_order_alloc_disable=1
	net.core.high_order_alloc_disable = 1
	+ sysctl net.core.high_order_alloc_disable
	net.core.high_order_alloc_disable = 1
	+ sleep 5
	+ timeout 5 bpftrace -e 'kprobe: skb_coalesce_rx_frag{@[nsecs/1000/1000/1000]=count()}'
	Attaching 1 probe...



	+ sysctl -w net.core.high_order_alloc_disable=0
	net.core.high_order_alloc_disable = 0
	+ sysctl net.core.high_order_alloc_disable
	net.core.high_order_alloc_disable = 0
	+ sleep 5
	+ timeout 5 bpftrace -e 'kprobe: skb_coalesce_rx_frag{@[nsecs/1000/1000/1000]=count()}'
	Attaching 1 probe...


	@[356]: 167020
	@[361]: 673653
	@[359]: 900844
	@[360]: 912657
	@[358]: 915853
	@[357]: 932245


We can see that the skb_coalesce_rx_frag is not called when comp page is disabled.
If the comp page is enable, there will be many frags coalescing.

So I think that my change will not have impact.

Thanks.




>
> Thanks
>
> >
> > Thanks.
> >
> >
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks.
> > > >
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > > 2) pre mapped DMA addresses were not reused in the case of XDP_TX/XDP_REDIRECT
> > > > > >
> > > > > > Because that the tx is not the premapped mode.
> > > > >
> > > > > Yes, we can optimize this on top.
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > > >
> > > > > > > I see Michael has merge this series so I'm fine to let it go first.
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks.
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()

[Michael S. Tsirkin]

On Tue, Aug 15, 2023 at 03:50:23PM +0800, Jason Wang wrote:
> On Tue, Aug 15, 2023 at 2:32 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> >
> > Hi, Jason
> >
> > Could you skip this patch?
> 
> I'm fine with either merging or dropping this.
> 
> >
> > Let we review other patches firstly?
> 
> I will be on vacation soon, and won't have time to do this until next week.
> 
> But I spot two possible "issues":
> 
> 1) the DMA metadata were stored in the headroom of the page, this
> breaks frags coalescing, we need to benchmark it's impact
> 2) pre mapped DMA addresses were not reused in the case of XDP_TX/XDP_REDIRECT
> 
> I see Michael has merge this series so I'm fine to let it go first.
> 
> Thanks

it's still queued for next. not too late to drop or better add
a patch on top.


> >
> > Thanks.
> >

[PATCH vhost v13 06/12] virtio_ring: skip unmap for premapped

[Xuan Zhuo]

Now we add a case where we skip dma unmap, the vq->premapped is true.

We can't just rely on use_dma_api to determine whether to skip the dma
operation. For convenience, I introduced the "do_unmap". By default, it
is the same as use_dma_api. If the driver is configured with premapped,
then do_unmap is false.

So as long as do_unmap is false, for addr of desc, we should skip dma
unmap operation.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 42 ++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index bb3d73d221cd..7973814b6e31 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -175,6 +175,11 @@ struct vring_virtqueue {
 	/* Do DMA mapping by driver */
 	bool premapped;
 
+	/* Do unmap or not for desc. Just when premapped is False and
+	 * use_dma_api is true, this is true.
+	 */
+	bool do_unmap;
+
 	/* Head of free buffer list. */
 	unsigned int free_head;
 	/* Number we've added since last sync. */
@@ -440,7 +445,7 @@ static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
 {
 	u16 flags;
 
-	if (!vq->use_dma_api)
+	if (!vq->do_unmap)
 		return;
 
 	flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
@@ -458,18 +463,21 @@ static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
 	struct vring_desc_extra *extra = vq->split.desc_extra;
 	u16 flags;
 
-	if (!vq->use_dma_api)
-		goto out;
-
 	flags = extra[i].flags;
 
 	if (flags & VRING_DESC_F_INDIRECT) {
+		if (!vq->use_dma_api)
+			goto out;
+
 		dma_unmap_single(vring_dma_dev(vq),
 				 extra[i].addr,
 				 extra[i].len,
 				 (flags & VRING_DESC_F_WRITE) ?
 				 DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	} else {
+		if (!vq->do_unmap)
+			goto out;
+
 		dma_unmap_page(vring_dma_dev(vq),
 			       extra[i].addr,
 			       extra[i].len,
@@ -635,7 +643,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
 	}
 	/* Last one doesn't continue. */
 	desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
-	if (!indirect && vq->use_dma_api)
+	if (!indirect && vq->do_unmap)
 		vq->split.desc_extra[prev & (vq->split.vring.num - 1)].flags &=
 			~VRING_DESC_F_NEXT;
 
@@ -794,7 +802,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->do_unmap) {
 			for (j = 0; j < len / sizeof(struct vring_desc); j++)
 				vring_unmap_one_split_indirect(vq, &indir_desc[j]);
 		}
@@ -1217,17 +1225,20 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
 {
 	u16 flags;
 
-	if (!vq->use_dma_api)
-		return;
-
 	flags = extra->flags;
 
 	if (flags & VRING_DESC_F_INDIRECT) {
+		if (!vq->use_dma_api)
+			return;
+
 		dma_unmap_single(vring_dma_dev(vq),
 				 extra->addr, extra->len,
 				 (flags & VRING_DESC_F_WRITE) ?
 				 DMA_FROM_DEVICE : DMA_TO_DEVICE);
 	} else {
+		if (!vq->do_unmap)
+			return;
+
 		dma_unmap_page(vring_dma_dev(vq),
 			       extra->addr, extra->len,
 			       (flags & VRING_DESC_F_WRITE) ?
@@ -1240,7 +1251,7 @@ static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
 {
 	u16 flags;
 
-	if (!vq->use_dma_api)
+	if (!vq->do_unmap)
 		return;
 
 	flags = le16_to_cpu(desc->flags);
@@ -1329,7 +1340,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
 				sizeof(struct vring_packed_desc));
 	vq->packed.vring.desc[head].id = cpu_to_le16(id);
 
-	if (vq->use_dma_api) {
+	if (vq->do_unmap) {
 		vq->packed.desc_extra[id].addr = addr;
 		vq->packed.desc_extra[id].len = total_sg *
 				sizeof(struct vring_packed_desc);
@@ -1470,7 +1481,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
 			desc[i].len = cpu_to_le32(sg->length);
 			desc[i].id = cpu_to_le16(id);
 
-			if (unlikely(vq->use_dma_api)) {
+			if (unlikely(vq->do_unmap)) {
 				vq->packed.desc_extra[curr].addr = addr;
 				vq->packed.desc_extra[curr].len = sg->length;
 				vq->packed.desc_extra[curr].flags =
@@ -1604,7 +1615,7 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
 	vq->free_head = id;
 	vq->vq.num_free += state->num;
 
-	if (unlikely(vq->use_dma_api)) {
+	if (unlikely(vq->do_unmap)) {
 		curr = id;
 		for (i = 0; i < state->num; i++) {
 			vring_unmap_extra_packed(vq,
@@ -1621,7 +1632,7 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
 		if (!desc)
 			return;
 
-		if (vq->use_dma_api) {
+		if (vq->do_unmap) {
 			len = vq->packed.desc_extra[id].len;
 			for (i = 0; i < len / sizeof(struct vring_packed_desc);
 					i++)
@@ -2080,6 +2091,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	vq->dma_dev = dma_dev;
 	vq->use_dma_api = vring_use_dma_api(vdev);
 	vq->premapped = false;
+	vq->do_unmap = vq->use_dma_api;
 
 	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC) &&
 		!context;
@@ -2587,6 +2599,7 @@ static struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	vq->dma_dev = dma_dev;
 	vq->use_dma_api = vring_use_dma_api(vdev);
 	vq->premapped = false;
+	vq->do_unmap = vq->use_dma_api;
 
 	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC) &&
 		!context;
@@ -2771,6 +2784,7 @@ int virtqueue_set_dma_premapped(struct virtqueue *_vq)
 	}
 
 	vq->premapped = true;
+	vq->do_unmap = false;
 
 	END_USE(vq);
 
-- 
2.32.0.3.g01195cf9f

[PATCH vhost v13 07/12] virtio_ring: correct the expression of the description of virtqueue_resize()

[Xuan Zhuo]

Modify the "useless" to a more accurate "unused".

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 7973814b6e31..fd9ae020e0a3 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2678,7 +2678,7 @@ EXPORT_SYMBOL_GPL(vring_create_virtqueue_dma);
  * virtqueue_resize - resize the vring of vq
  * @_vq: the struct virtqueue we're talking about.
  * @num: new ring num
- * @recycle: callback for recycle the useless buffer
+ * @recycle: callback to recycle unused buffers
  *
  * When it is really necessary to create a new vring, it will set the current vq
  * into the reset state. Then call the passed callback to recycle the buffer
-- 
2.32.0.3.g01195cf9f

[PATCH vhost v13 08/12] virtio_ring: separate the logic of reset/enable from virtqueue_resize

[Xuan Zhuo]

The subsequent reset function will reuse these logic.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 58 ++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fd9ae020e0a3..23172d98e48e 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2152,6 +2152,43 @@ static int virtqueue_resize_packed(struct virtqueue *_vq, u32 num)
 	return -ENOMEM;
 }
 
+static int virtqueue_disable_and_recycle(struct virtqueue *_vq,
+					 void (*recycle)(struct virtqueue *vq, void *buf))
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct virtio_device *vdev = vq->vq.vdev;
+	void *buf;
+	int err;
+
+	if (!vq->we_own_ring)
+		return -EPERM;
+
+	if (!vdev->config->disable_vq_and_reset)
+		return -ENOENT;
+
+	if (!vdev->config->enable_vq_after_reset)
+		return -ENOENT;
+
+	err = vdev->config->disable_vq_and_reset(_vq);
+	if (err)
+		return err;
+
+	while ((buf = virtqueue_detach_unused_buf(_vq)) != NULL)
+		recycle(_vq, buf);
+
+	return 0;
+}
+
+static int virtqueue_enable_after_reset(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct virtio_device *vdev = vq->vq.vdev;
+
+	if (vdev->config->enable_vq_after_reset(_vq))
+		return -EBUSY;
+
+	return 0;
+}
 
 /*
  * Generic functions and exported symbols.
@@ -2702,13 +2739,8 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
 		     void (*recycle)(struct virtqueue *vq, void *buf))
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
-	struct virtio_device *vdev = vq->vq.vdev;
-	void *buf;
 	int err;
 
-	if (!vq->we_own_ring)
-		return -EPERM;
-
 	if (num > vq->vq.num_max)
 		return -E2BIG;
 
@@ -2718,28 +2750,16 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
 	if ((vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num) == num)
 		return 0;
 
-	if (!vdev->config->disable_vq_and_reset)
-		return -ENOENT;
-
-	if (!vdev->config->enable_vq_after_reset)
-		return -ENOENT;
-
-	err = vdev->config->disable_vq_and_reset(_vq);
+	err = virtqueue_disable_and_recycle(_vq, recycle);
 	if (err)
 		return err;
 
-	while ((buf = virtqueue_detach_unused_buf(_vq)) != NULL)
-		recycle(_vq, buf);
-
 	if (vq->packed_ring)
 		err = virtqueue_resize_packed(_vq, num);
 	else
 		err = virtqueue_resize_split(_vq, num);
 
-	if (vdev->config->enable_vq_after_reset(_vq))
-		return -EBUSY;
-
-	return err;
+	return virtqueue_enable_after_reset(_vq);
 }
 EXPORT_SYMBOL_GPL(virtqueue_resize);
 
-- 
2.32.0.3.g01195cf9f

[PATCH vhost v13 09/12] virtio_ring: introduce virtqueue_reset()

[Xuan Zhuo]

Introduce virtqueue_reset() to release all buffer inside vq.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_ring.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/virtio.h       |  2 ++
 2 files changed, 35 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 23172d98e48e..639c20b19e06 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2812,6 +2812,39 @@ int virtqueue_set_dma_premapped(struct virtqueue *_vq)
 }
 EXPORT_SYMBOL_GPL(virtqueue_set_dma_premapped);
 
+/**
+ * virtqueue_reset - detach and recycle all unused buffers
+ * @_vq: the struct virtqueue we're talking about.
+ * @recycle: callback to recycle unused buffers
+ *
+ * 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.
+ * -EBUSY: Failed to sync with device, vq may not work properly
+ * -ENOENT: Transport or device not supported
+ * -EPERM: Operation not permitted
+ */
+int virtqueue_reset(struct virtqueue *_vq,
+		    void (*recycle)(struct virtqueue *vq, void *buf))
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+	int err;
+
+	err = virtqueue_disable_and_recycle(_vq, recycle);
+	if (err)
+		return err;
+
+	if (vq->packed_ring)
+		virtqueue_reinit_packed(vq);
+	else
+		virtqueue_reinit_split(vq);
+
+	return virtqueue_enable_after_reset(_vq);
+}
+EXPORT_SYMBOL_GPL(virtqueue_reset);
+
 /* 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 bd55a05eec04..49a640e0a6f4 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -99,6 +99,8 @@ dma_addr_t virtqueue_get_used_addr(const struct virtqueue *vq);
 
 int virtqueue_resize(struct virtqueue *vq, u32 num,
 		     void (*recycle)(struct virtqueue *vq, void *buf));
+int virtqueue_reset(struct virtqueue *vq,
+		    void (*recycle)(struct virtqueue *vq, void *buf));
 
 /**
  * struct virtio_device - representation of a device using virtio
-- 
2.32.0.3.g01195cf9f

[PATCH vhost v13 10/12] virtio_ring: introduce dma map api for virtqueue

[Xuan Zhuo]

Added virtqueue_dma_map_api* to map DMA addresses for virtual memory in
advance. The purpose is to keep memory mapped across multiple add/get
buf operations.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 69 ++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h       |  8 +++++
 2 files changed, 77 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 639c20b19e06..916479c9c72c 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -3106,4 +3106,73 @@ const struct vring *virtqueue_get_vring(const struct virtqueue *vq)
 }
 EXPORT_SYMBOL_GPL(virtqueue_get_vring);
 
+/**
+ * virtqueue_dma_map_single_attrs - map DMA for _vq
+ * @_vq: the struct virtqueue we're talking about.
+ * @ptr: the pointer of the buffer to do dma
+ * @size: the size of the buffer 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_single_attrs(struct virtqueue *_vq, void *ptr,
+					  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_addr_t)virt_to_phys(ptr);
+
+	return dma_map_single_attrs(vring_dma_dev(vq), ptr, size, dir, attrs);
+}
+EXPORT_SYMBOL_GPL(virtqueue_dma_map_single_attrs);
+
+/**
+ * virtqueue_dma_unmap_single_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_single_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_single_attrs(vring_dma_dev(vq), addr, size, dir, attrs);
+}
+EXPORT_SYMBOL_GPL(virtqueue_dma_unmap_single_attrs);
+
+/**
+ * virtqueue_dma_mapping_error - check dma address
+ * @_vq: the struct virtqueue we're talking about.
+ * @addr: DMA address
+ *
+ * Returns 0 means dma valid. Other means invalid dma address.
+ */
+int virtqueue_dma_mapping_error(struct virtqueue *_vq, dma_addr_t addr)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+
+	if (!vq->use_dma_api)
+		return 0;
+
+	return dma_mapping_error(vring_dma_dev(vq), addr);
+}
+EXPORT_SYMBOL_GPL(virtqueue_dma_mapping_error);
+
 MODULE_LICENSE("GPL");
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 49a640e0a6f4..79e3c74391e0 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/mod_devicetable.h>
 #include <linux/gfp.h>
+#include <linux/dma-mapping.h>
 
 /**
  * struct virtqueue - a queue to register buffers for sending or receiving.
@@ -212,4 +213,11 @@ void unregister_virtio_driver(struct virtio_driver *drv);
 #define module_virtio_driver(__virtio_driver) \
 	module_driver(__virtio_driver, register_virtio_driver, \
 			unregister_virtio_driver)
+
+dma_addr_t virtqueue_dma_map_single_attrs(struct virtqueue *_vq, void *ptr, size_t size,
+					  enum dma_data_direction dir, unsigned long attrs);
+void virtqueue_dma_unmap_single_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);
 #endif /* _LINUX_VIRTIO_H */
-- 
2.32.0.3.g01195cf9f

[PATCH vhost v13 11/12] virtio_ring: introduce dma sync api for virtqueue

[Xuan Zhuo]

These API has been introduced:

* virtqueue_dma_need_sync
* virtqueue_dma_sync_single_range_for_cpu
* virtqueue_dma_sync_single_range_for_device

These APIs can be used together with the premapped mechanism to sync the
DMA address.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 76 ++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h       |  8 ++++
 2 files changed, 84 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 916479c9c72c..81ecb29c88f1 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -3175,4 +3175,80 @@ int virtqueue_dma_mapping_error(struct virtqueue *_vq, dma_addr_t addr)
 }
 EXPORT_SYMBOL_GPL(virtqueue_dma_mapping_error);
 
+/**
+ * virtqueue_dma_need_sync - check a dma address needs sync
+ * @_vq: the struct virtqueue we're talking about.
+ * @addr: DMA address
+ *
+ * Check if the dma address mapped by the virtqueue_dma_map_* APIs needs to be
+ * synchronized
+ *
+ * return bool
+ */
+bool virtqueue_dma_need_sync(struct virtqueue *_vq, dma_addr_t addr)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+
+	if (!vq->use_dma_api)
+		return false;
+
+	return dma_need_sync(vring_dma_dev(vq), addr);
+}
+EXPORT_SYMBOL_GPL(virtqueue_dma_need_sync);
+
+/**
+ * virtqueue_dma_sync_single_range_for_cpu - dma sync for cpu
+ * @_vq: the struct virtqueue we're talking about.
+ * @addr: DMA address
+ * @offset: DMA address offset
+ * @size: buf size for sync
+ * @dir: DMA direction
+ *
+ * Before calling this function, use virtqueue_dma_need_sync() to confirm that
+ * the DMA address really needs to be synchronized
+ *
+ */
+void virtqueue_dma_sync_single_range_for_cpu(struct virtqueue *_vq,
+					     dma_addr_t addr,
+					     unsigned long offset, size_t size,
+					     enum dma_data_direction dir)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct device *dev = vring_dma_dev(vq);
+
+	if (!vq->use_dma_api)
+		return;
+
+	dma_sync_single_range_for_cpu(dev, addr, offset, size,
+				      DMA_BIDIRECTIONAL);
+}
+EXPORT_SYMBOL_GPL(virtqueue_dma_sync_single_range_for_cpu);
+
+/**
+ * virtqueue_dma_sync_single_range_for_device - dma sync for device
+ * @_vq: the struct virtqueue we're talking about.
+ * @addr: DMA address
+ * @offset: DMA address offset
+ * @size: buf size for sync
+ * @dir: DMA direction
+ *
+ * Before calling this function, use virtqueue_dma_need_sync() to confirm that
+ * the DMA address really needs to be synchronized
+ */
+void virtqueue_dma_sync_single_range_for_device(struct virtqueue *_vq,
+						dma_addr_t addr,
+						unsigned long offset, size_t size,
+						enum dma_data_direction dir)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct device *dev = vring_dma_dev(vq);
+
+	if (!vq->use_dma_api)
+		return;
+
+	dma_sync_single_range_for_device(dev, addr, offset, size,
+					 DMA_BIDIRECTIONAL);
+}
+EXPORT_SYMBOL_GPL(virtqueue_dma_sync_single_range_for_device);
+
 MODULE_LICENSE("GPL");
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 79e3c74391e0..1311a7fbe675 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -220,4 +220,12 @@ void virtqueue_dma_unmap_single_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);
+void virtqueue_dma_sync_single_range_for_cpu(struct virtqueue *_vq, dma_addr_t addr,
+					     unsigned long offset, size_t size,
+					     enum dma_data_direction dir);
+void virtqueue_dma_sync_single_range_for_device(struct virtqueue *_vq, dma_addr_t addr,
+						unsigned long offset, size_t size,
+						enum dma_data_direction dir);
 #endif /* _LINUX_VIRTIO_H */
-- 
2.32.0.3.g01195cf9f

Re: [PATCH vhost v13 11/12] virtio_ring: introduce dma sync api for virtqueue

[Michael S. Tsirkin]

On Thu, Aug 10, 2023 at 08:30:56PM +0800, Xuan Zhuo wrote:
> These API has been introduced:
> 
> * virtqueue_dma_need_sync
> * virtqueue_dma_sync_single_range_for_cpu
> * virtqueue_dma_sync_single_range_for_device
> 
> These APIs can be used together with the premapped mechanism to sync the
> DMA address.
> 
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/virtio/virtio_ring.c | 76 ++++++++++++++++++++++++++++++++++++
>  include/linux/virtio.h       |  8 ++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 916479c9c72c..81ecb29c88f1 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -3175,4 +3175,80 @@ int virtqueue_dma_mapping_error(struct virtqueue *_vq, dma_addr_t addr)
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_dma_mapping_error);
>  
> +/**
> + * virtqueue_dma_need_sync - check a dma address needs sync
> + * @_vq: the struct virtqueue we're talking about.
> + * @addr: DMA address
> + *
> + * Check if the dma address mapped by the virtqueue_dma_map_* APIs needs to be
> + * synchronized
> + *
> + * return bool
> + */
> +bool virtqueue_dma_need_sync(struct virtqueue *_vq, dma_addr_t addr)
> +{
> +	struct vring_virtqueue *vq = to_vvq(_vq);
> +
> +	if (!vq->use_dma_api)
> +		return false;
> +
> +	return dma_need_sync(vring_dma_dev(vq), addr);
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_dma_need_sync);
> +
> +/**
> + * virtqueue_dma_sync_single_range_for_cpu - dma sync for cpu
> + * @_vq: the struct virtqueue we're talking about.
> + * @addr: DMA address
> + * @offset: DMA address offset
> + * @size: buf size for sync
> + * @dir: DMA direction
> + *
> + * Before calling this function, use virtqueue_dma_need_sync() to confirm that
> + * the DMA address really needs to be synchronized
> + *
> + */
> +void virtqueue_dma_sync_single_range_for_cpu(struct virtqueue *_vq,
> +					     dma_addr_t addr,
> +					     unsigned long offset, size_t size,
> +					     enum dma_data_direction dir)
> +{
> +	struct vring_virtqueue *vq = to_vvq(_vq);
> +	struct device *dev = vring_dma_dev(vq);
> +
> +	if (!vq->use_dma_api)
> +		return;
> +
> +	dma_sync_single_range_for_cpu(dev, addr, offset, size,
> +				      DMA_BIDIRECTIONAL);
> +}


Why did you use DMA_BIDIRECTIONAL here?
Why is "dir" ignored?


> +EXPORT_SYMBOL_GPL(virtqueue_dma_sync_single_range_for_cpu);
> +
> +/**
> + * virtqueue_dma_sync_single_range_for_device - dma sync for device
> + * @_vq: the struct virtqueue we're talking about.
> + * @addr: DMA address
> + * @offset: DMA address offset
> + * @size: buf size for sync
> + * @dir: DMA direction
> + *
> + * Before calling this function, use virtqueue_dma_need_sync() to confirm that
> + * the DMA address really needs to be synchronized
> + */
> +void virtqueue_dma_sync_single_range_for_device(struct virtqueue *_vq,
> +						dma_addr_t addr,
> +						unsigned long offset, size_t size,
> +						enum dma_data_direction dir)
> +{
> +	struct vring_virtqueue *vq = to_vvq(_vq);
> +	struct device *dev = vring_dma_dev(vq);
> +
> +	if (!vq->use_dma_api)
> +		return;
> +
> +	dma_sync_single_range_for_device(dev, addr, offset, size,
> +					 DMA_BIDIRECTIONAL);
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_dma_sync_single_range_for_device);
> +
>  MODULE_LICENSE("GPL");

same question here.

> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 79e3c74391e0..1311a7fbe675 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -220,4 +220,12 @@ void virtqueue_dma_unmap_single_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);
> +void virtqueue_dma_sync_single_range_for_cpu(struct virtqueue *_vq, dma_addr_t addr,
> +					     unsigned long offset, size_t size,
> +					     enum dma_data_direction dir);
> +void virtqueue_dma_sync_single_range_for_device(struct virtqueue *_vq, dma_addr_t addr,
> +						unsigned long offset, size_t size,
> +						enum dma_data_direction dir);
>  #endif /* _LINUX_VIRTIO_H */
> -- 
> 2.32.0.3.g01195cf9f

Re: [PATCH vhost v13 11/12] virtio_ring: introduce dma sync api for virtqueue

[Xuan Zhuo]

On Thu, 30 Nov 2023 04:45:58 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Aug 10, 2023 at 08:30:56PM +0800, Xuan Zhuo wrote:
> > These API has been introduced:
> >
> > * virtqueue_dma_need_sync
> > * virtqueue_dma_sync_single_range_for_cpu
> > * virtqueue_dma_sync_single_range_for_device
> >
> > These APIs can be used together with the premapped mechanism to sync the
> > DMA address.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  drivers/virtio/virtio_ring.c | 76 ++++++++++++++++++++++++++++++++++++
> >  include/linux/virtio.h       |  8 ++++
> >  2 files changed, 84 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 916479c9c72c..81ecb29c88f1 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -3175,4 +3175,80 @@ int virtqueue_dma_mapping_error(struct virtqueue *_vq, dma_addr_t addr)
> >  }
> >  EXPORT_SYMBOL_GPL(virtqueue_dma_mapping_error);
> >
> > +/**
> > + * virtqueue_dma_need_sync - check a dma address needs sync
> > + * @_vq: the struct virtqueue we're talking about.
> > + * @addr: DMA address
> > + *
> > + * Check if the dma address mapped by the virtqueue_dma_map_* APIs needs to be
> > + * synchronized
> > + *
> > + * return bool
> > + */
> > +bool virtqueue_dma_need_sync(struct virtqueue *_vq, dma_addr_t addr)
> > +{
> > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > +
> > +	if (!vq->use_dma_api)
> > +		return false;
> > +
> > +	return dma_need_sync(vring_dma_dev(vq), addr);
> > +}
> > +EXPORT_SYMBOL_GPL(virtqueue_dma_need_sync);
> > +
> > +/**
> > + * virtqueue_dma_sync_single_range_for_cpu - dma sync for cpu
> > + * @_vq: the struct virtqueue we're talking about.
> > + * @addr: DMA address
> > + * @offset: DMA address offset
> > + * @size: buf size for sync
> > + * @dir: DMA direction
> > + *
> > + * Before calling this function, use virtqueue_dma_need_sync() to confirm that
> > + * the DMA address really needs to be synchronized
> > + *
> > + */
> > +void virtqueue_dma_sync_single_range_for_cpu(struct virtqueue *_vq,
> > +					     dma_addr_t addr,
> > +					     unsigned long offset, size_t size,
> > +					     enum dma_data_direction dir)
> > +{
> > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > +	struct device *dev = vring_dma_dev(vq);
> > +
> > +	if (!vq->use_dma_api)
> > +		return;
> > +
> > +	dma_sync_single_range_for_cpu(dev, addr, offset, size,
> > +				      DMA_BIDIRECTIONAL);
> > +}
>
>
> Why did you use DMA_BIDIRECTIONAL here?
> Why is "dir" ignored?

This is a mistake.

I see Jason has a fix patch.

How can I help?

Thanks.


>
>
> > +EXPORT_SYMBOL_GPL(virtqueue_dma_sync_single_range_for_cpu);
> > +
> > +/**
> > + * virtqueue_dma_sync_single_range_for_device - dma sync for device
> > + * @_vq: the struct virtqueue we're talking about.
> > + * @addr: DMA address
> > + * @offset: DMA address offset
> > + * @size: buf size for sync
> > + * @dir: DMA direction
> > + *
> > + * Before calling this function, use virtqueue_dma_need_sync() to confirm that
> > + * the DMA address really needs to be synchronized
> > + */
> > +void virtqueue_dma_sync_single_range_for_device(struct virtqueue *_vq,
> > +						dma_addr_t addr,
> > +						unsigned long offset, size_t size,
> > +						enum dma_data_direction dir)
> > +{
> > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > +	struct device *dev = vring_dma_dev(vq);
> > +
> > +	if (!vq->use_dma_api)
> > +		return;
> > +
> > +	dma_sync_single_range_for_device(dev, addr, offset, size,
> > +					 DMA_BIDIRECTIONAL);
> > +}
> > +EXPORT_SYMBOL_GPL(virtqueue_dma_sync_single_range_for_device);
> > +
> >  MODULE_LICENSE("GPL");
>
> same question here.
>
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index 79e3c74391e0..1311a7fbe675 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -220,4 +220,12 @@ void virtqueue_dma_unmap_single_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);
> > +void virtqueue_dma_sync_single_range_for_cpu(struct virtqueue *_vq, dma_addr_t addr,
> > +					     unsigned long offset, size_t size,
> > +					     enum dma_data_direction dir);
> > +void virtqueue_dma_sync_single_range_for_device(struct virtqueue *_vq, dma_addr_t addr,
> > +						unsigned long offset, size_t size,
> > +						enum dma_data_direction dir);
> >  #endif /* _LINUX_VIRTIO_H */
> > --
> > 2.32.0.3.g01195cf9f
>
>

Re: [PATCH vhost v13 11/12] virtio_ring: introduce dma sync api for virtqueue

[Michael S. Tsirkin]

On Thu, Nov 30, 2023 at 05:49:38PM +0800, Xuan Zhuo wrote:
> On Thu, 30 Nov 2023 04:45:58 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Aug 10, 2023 at 08:30:56PM +0800, Xuan Zhuo wrote:
> > > These API has been introduced:
> > >
> > > * virtqueue_dma_need_sync
> > > * virtqueue_dma_sync_single_range_for_cpu
> > > * virtqueue_dma_sync_single_range_for_device
> > >
> > > These APIs can be used together with the premapped mechanism to sync the
> > > DMA address.
> > >
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >  drivers/virtio/virtio_ring.c | 76 ++++++++++++++++++++++++++++++++++++
> > >  include/linux/virtio.h       |  8 ++++
> > >  2 files changed, 84 insertions(+)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 916479c9c72c..81ecb29c88f1 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -3175,4 +3175,80 @@ int virtqueue_dma_mapping_error(struct virtqueue *_vq, dma_addr_t addr)
> > >  }
> > >  EXPORT_SYMBOL_GPL(virtqueue_dma_mapping_error);
> > >
> > > +/**
> > > + * virtqueue_dma_need_sync - check a dma address needs sync
> > > + * @_vq: the struct virtqueue we're talking about.
> > > + * @addr: DMA address
> > > + *
> > > + * Check if the dma address mapped by the virtqueue_dma_map_* APIs needs to be
> > > + * synchronized
> > > + *
> > > + * return bool
> > > + */
> > > +bool virtqueue_dma_need_sync(struct virtqueue *_vq, dma_addr_t addr)
> > > +{
> > > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > > +
> > > +	if (!vq->use_dma_api)
> > > +		return false;
> > > +
> > > +	return dma_need_sync(vring_dma_dev(vq), addr);
> > > +}
> > > +EXPORT_SYMBOL_GPL(virtqueue_dma_need_sync);
> > > +
> > > +/**
> > > + * virtqueue_dma_sync_single_range_for_cpu - dma sync for cpu
> > > + * @_vq: the struct virtqueue we're talking about.
> > > + * @addr: DMA address
> > > + * @offset: DMA address offset
> > > + * @size: buf size for sync
> > > + * @dir: DMA direction
> > > + *
> > > + * Before calling this function, use virtqueue_dma_need_sync() to confirm that
> > > + * the DMA address really needs to be synchronized
> > > + *
> > > + */
> > > +void virtqueue_dma_sync_single_range_for_cpu(struct virtqueue *_vq,
> > > +					     dma_addr_t addr,
> > > +					     unsigned long offset, size_t size,
> > > +					     enum dma_data_direction dir)
> > > +{
> > > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > > +	struct device *dev = vring_dma_dev(vq);
> > > +
> > > +	if (!vq->use_dma_api)
> > > +		return;
> > > +
> > > +	dma_sync_single_range_for_cpu(dev, addr, offset, size,
> > > +				      DMA_BIDIRECTIONAL);
> > > +}
> >
> >
> > Why did you use DMA_BIDIRECTIONAL here?
> > Why is "dir" ignored?
> 
> This is a mistake.
> 
> I see Jason has a fix patch.

the one he sent in response to the bug report? it's incomplete though.

> How can I help?
> 
> Thanks.

develop a full patch with commit log, explanation of what
the result of the bug is, Fixes tag etc, test
in some environment where dir makes a difference and post.


> 
> >
> >
> > > +EXPORT_SYMBOL_GPL(virtqueue_dma_sync_single_range_for_cpu);
> > > +
> > > +/**
> > > + * virtqueue_dma_sync_single_range_for_device - dma sync for device
> > > + * @_vq: the struct virtqueue we're talking about.
> > > + * @addr: DMA address
> > > + * @offset: DMA address offset
> > > + * @size: buf size for sync
> > > + * @dir: DMA direction
> > > + *
> > > + * Before calling this function, use virtqueue_dma_need_sync() to confirm that
> > > + * the DMA address really needs to be synchronized
> > > + */
> > > +void virtqueue_dma_sync_single_range_for_device(struct virtqueue *_vq,
> > > +						dma_addr_t addr,
> > > +						unsigned long offset, size_t size,
> > > +						enum dma_data_direction dir)
> > > +{
> > > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > > +	struct device *dev = vring_dma_dev(vq);
> > > +
> > > +	if (!vq->use_dma_api)
> > > +		return;
> > > +
> > > +	dma_sync_single_range_for_device(dev, addr, offset, size,
> > > +					 DMA_BIDIRECTIONAL);
> > > +}
> > > +EXPORT_SYMBOL_GPL(virtqueue_dma_sync_single_range_for_device);
> > > +
> > >  MODULE_LICENSE("GPL");
> >
> > same question here.
> >
> > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > index 79e3c74391e0..1311a7fbe675 100644
> > > --- a/include/linux/virtio.h
> > > +++ b/include/linux/virtio.h
> > > @@ -220,4 +220,12 @@ void virtqueue_dma_unmap_single_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);
> > > +void virtqueue_dma_sync_single_range_for_cpu(struct virtqueue *_vq, dma_addr_t addr,
> > > +					     unsigned long offset, size_t size,
> > > +					     enum dma_data_direction dir);
> > > +void virtqueue_dma_sync_single_range_for_device(struct virtqueue *_vq, dma_addr_t addr,
> > > +						unsigned long offset, size_t size,
> > > +						enum dma_data_direction dir);
> > >  #endif /* _LINUX_VIRTIO_H */
> > > --
> > > 2.32.0.3.g01195cf9f
> >
> >

Re: [PATCH vhost v13 11/12] virtio_ring: introduce dma sync api for virtqueue

[Xuan Zhuo]

On Thu, 30 Nov 2023 05:10:42 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Nov 30, 2023 at 05:49:38PM +0800, Xuan Zhuo wrote:
> > On Thu, 30 Nov 2023 04:45:58 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Thu, Aug 10, 2023 at 08:30:56PM +0800, Xuan Zhuo wrote:
> > > > These API has been introduced:
> > > >
> > > > * virtqueue_dma_need_sync
> > > > * virtqueue_dma_sync_single_range_for_cpu
> > > > * virtqueue_dma_sync_single_range_for_device
> > > >
> > > > These APIs can be used together with the premapped mechanism to sync the
> > > > DMA address.
> > > >
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > >  drivers/virtio/virtio_ring.c | 76 ++++++++++++++++++++++++++++++++++++
> > > >  include/linux/virtio.h       |  8 ++++
> > > >  2 files changed, 84 insertions(+)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 916479c9c72c..81ecb29c88f1 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -3175,4 +3175,80 @@ int virtqueue_dma_mapping_error(struct virtqueue *_vq, dma_addr_t addr)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(virtqueue_dma_mapping_error);
> > > >
> > > > +/**
> > > > + * virtqueue_dma_need_sync - check a dma address needs sync
> > > > + * @_vq: the struct virtqueue we're talking about.
> > > > + * @addr: DMA address
> > > > + *
> > > > + * Check if the dma address mapped by the virtqueue_dma_map_* APIs needs to be
> > > > + * synchronized
> > > > + *
> > > > + * return bool
> > > > + */
> > > > +bool virtqueue_dma_need_sync(struct virtqueue *_vq, dma_addr_t addr)
> > > > +{
> > > > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > > > +
> > > > +	if (!vq->use_dma_api)
> > > > +		return false;
> > > > +
> > > > +	return dma_need_sync(vring_dma_dev(vq), addr);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(virtqueue_dma_need_sync);
> > > > +
> > > > +/**
> > > > + * virtqueue_dma_sync_single_range_for_cpu - dma sync for cpu
> > > > + * @_vq: the struct virtqueue we're talking about.
> > > > + * @addr: DMA address
> > > > + * @offset: DMA address offset
> > > > + * @size: buf size for sync
> > > > + * @dir: DMA direction
> > > > + *
> > > > + * Before calling this function, use virtqueue_dma_need_sync() to confirm that
> > > > + * the DMA address really needs to be synchronized
> > > > + *
> > > > + */
> > > > +void virtqueue_dma_sync_single_range_for_cpu(struct virtqueue *_vq,
> > > > +					     dma_addr_t addr,
> > > > +					     unsigned long offset, size_t size,
> > > > +					     enum dma_data_direction dir)
> > > > +{
> > > > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > > > +	struct device *dev = vring_dma_dev(vq);
> > > > +
> > > > +	if (!vq->use_dma_api)
> > > > +		return;
> > > > +
> > > > +	dma_sync_single_range_for_cpu(dev, addr, offset, size,
> > > > +				      DMA_BIDIRECTIONAL);
> > > > +}
> > >
> > >
> > > Why did you use DMA_BIDIRECTIONAL here?
> > > Why is "dir" ignored?
> >
> > This is a mistake.
> >
> > I see Jason has a fix patch.
>
> the one he sent in response to the bug report? it's incomplete though.
>
> > How can I help?
> >
> > Thanks.
>
> develop a full patch with commit log, explanation of what
> the result of the bug is, Fixes tag etc, test
> in some environment where dir makes a difference and post.

I'm ok.

Thanks


>
>
> >
> > >
> > >
> > > > +EXPORT_SYMBOL_GPL(virtqueue_dma_sync_single_range_for_cpu);
> > > > +
> > > > +/**
> > > > + * virtqueue_dma_sync_single_range_for_device - dma sync for device
> > > > + * @_vq: the struct virtqueue we're talking about.
> > > > + * @addr: DMA address
> > > > + * @offset: DMA address offset
> > > > + * @size: buf size for sync
> > > > + * @dir: DMA direction
> > > > + *
> > > > + * Before calling this function, use virtqueue_dma_need_sync() to confirm that
> > > > + * the DMA address really needs to be synchronized
> > > > + */
> > > > +void virtqueue_dma_sync_single_range_for_device(struct virtqueue *_vq,
> > > > +						dma_addr_t addr,
> > > > +						unsigned long offset, size_t size,
> > > > +						enum dma_data_direction dir)
> > > > +{
> > > > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > > > +	struct device *dev = vring_dma_dev(vq);
> > > > +
> > > > +	if (!vq->use_dma_api)
> > > > +		return;
> > > > +
> > > > +	dma_sync_single_range_for_device(dev, addr, offset, size,
> > > > +					 DMA_BIDIRECTIONAL);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(virtqueue_dma_sync_single_range_for_device);
> > > > +
> > > >  MODULE_LICENSE("GPL");
> > >
> > > same question here.
> > >
> > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > > index 79e3c74391e0..1311a7fbe675 100644
> > > > --- a/include/linux/virtio.h
> > > > +++ b/include/linux/virtio.h
> > > > @@ -220,4 +220,12 @@ void virtqueue_dma_unmap_single_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);
> > > > +void virtqueue_dma_sync_single_range_for_cpu(struct virtqueue *_vq, dma_addr_t addr,
> > > > +					     unsigned long offset, size_t size,
> > > > +					     enum dma_data_direction dir);
> > > > +void virtqueue_dma_sync_single_range_for_device(struct virtqueue *_vq, dma_addr_t addr,
> > > > +						unsigned long offset, size_t size,
> > > > +						enum dma_data_direction dir);
> > > >  #endif /* _LINUX_VIRTIO_H */
> > > > --
> > > > 2.32.0.3.g01195cf9f
> > >
> > >
>

[PATCH vhost v13 12/12] virtio_net: merge dma operations when filling mergeable buffers

[Xuan Zhuo]

Currently, the virtio core will perform a dma operation for each
buffer. Although, the same page may be operated multiple times.

This patch, the driver does the dma operation and manages the dma
address based the feature premapped of virtio core.

This way, we can perform only one dma operation for the pages of the
alloc frag. This is beneficial for the iommu device.

kernel command line: intel_iommu=on iommu.passthrough=0

       |  strict=0  | strict=1
Before |  775496pps | 428614pps
After  | 1109316pps | 742853pps

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 228 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 202 insertions(+), 26 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 486b5849033d..16adb5ef18f8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -126,6 +126,14 @@ static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
 #define VIRTNET_SQ_STATS_LEN	ARRAY_SIZE(virtnet_sq_stats_desc)
 #define VIRTNET_RQ_STATS_LEN	ARRAY_SIZE(virtnet_rq_stats_desc)
 
+/* The dma information of pages allocated at a time. */
+struct virtnet_rq_dma {
+	dma_addr_t addr;
+	u32 ref;
+	u16 len;
+	u16 need_sync;
+};
+
 /* Internal representation of a send virtqueue */
 struct send_queue {
 	/* Virtqueue associated with this send _queue */
@@ -175,6 +183,12 @@ struct receive_queue {
 	char name[16];
 
 	struct xdp_rxq_info xdp_rxq;
+
+	/* Record the last dma info to free after new pages is allocated. */
+	struct virtnet_rq_dma *last_dma;
+
+	/* Do dma by self */
+	bool do_dma;
 };
 
 /* This structure can contain rss message with maximum settings for indirection table and keysize
@@ -549,6 +563,156 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
 	return skb;
 }
 
+static void virtnet_rq_unmap(struct receive_queue *rq, void *buf, u32 len)
+{
+	struct page *page = virt_to_head_page(buf);
+	struct virtnet_rq_dma *dma;
+	void *head;
+	int offset;
+
+	head = page_address(page);
+
+	dma = head;
+
+	--dma->ref;
+
+	if (dma->ref) {
+		if (dma->need_sync && len) {
+			offset = buf - (head + sizeof(*dma));
+
+			virtqueue_dma_sync_single_range_for_cpu(rq->vq, dma->addr, offset,
+								len, DMA_FROM_DEVICE);
+		}
+
+		return;
+	}
+
+	virtqueue_dma_unmap_single_attrs(rq->vq, dma->addr, dma->len,
+					 DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
+	put_page(page);
+}
+
+static void *virtnet_rq_get_buf(struct receive_queue *rq, u32 *len, void **ctx)
+{
+	void *buf;
+
+	buf = virtqueue_get_buf_ctx(rq->vq, len, ctx);
+	if (buf && rq->do_dma)
+		virtnet_rq_unmap(rq, buf, *len);
+
+	return buf;
+}
+
+static void *virtnet_rq_detach_unused_buf(struct receive_queue *rq)
+{
+	void *buf;
+
+	buf = virtqueue_detach_unused_buf(rq->vq);
+	if (buf && rq->do_dma)
+		virtnet_rq_unmap(rq, buf, 0);
+
+	return buf;
+}
+
+static void virtnet_rq_init_one_sg(struct receive_queue *rq, void *buf, u32 len)
+{
+	struct virtnet_rq_dma *dma;
+	dma_addr_t addr;
+	u32 offset;
+	void *head;
+
+	if (!rq->do_dma) {
+		sg_init_one(rq->sg, buf, len);
+		return;
+	}
+
+	head = page_address(rq->alloc_frag.page);
+
+	offset = buf - head;
+
+	dma = head;
+
+	addr = dma->addr - sizeof(*dma) + offset;
+
+	sg_init_table(rq->sg, 1);
+	rq->sg[0].dma_address = addr;
+	rq->sg[0].length = len;
+}
+
+static void *virtnet_rq_alloc(struct receive_queue *rq, u32 size, gfp_t gfp)
+{
+	struct page_frag *alloc_frag = &rq->alloc_frag;
+	struct virtnet_rq_dma *dma;
+	void *buf, *head;
+	dma_addr_t addr;
+
+	if (unlikely(!skb_page_frag_refill(size, alloc_frag, gfp)))
+		return NULL;
+
+	head = page_address(alloc_frag->page);
+
+	if (rq->do_dma) {
+		dma = head;
+
+		/* new pages */
+		if (!alloc_frag->offset) {
+			if (rq->last_dma) {
+				/* Now, the new page is allocated, the last dma
+				 * will not be used. So the dma can be unmapped
+				 * if the ref is 0.
+				 */
+				virtnet_rq_unmap(rq, rq->last_dma, 0);
+				rq->last_dma = NULL;
+			}
+
+			dma->len = alloc_frag->size - sizeof(*dma);
+
+			addr = virtqueue_dma_map_single_attrs(rq->vq, dma + 1,
+							      dma->len, DMA_FROM_DEVICE, 0);
+			if (virtqueue_dma_mapping_error(rq->vq, addr))
+				return NULL;
+
+			dma->addr = addr;
+			dma->need_sync = virtqueue_dma_need_sync(rq->vq, addr);
+
+			/* Add a reference to dma to prevent the entire dma from
+			 * being released during error handling. This reference
+			 * will be freed after the pages are no longer used.
+			 */
+			get_page(alloc_frag->page);
+			dma->ref = 1;
+			alloc_frag->offset = sizeof(*dma);
+
+			rq->last_dma = dma;
+		}
+
+		++dma->ref;
+	}
+
+	buf = head + alloc_frag->offset;
+
+	get_page(alloc_frag->page);
+	alloc_frag->offset += size;
+
+	return buf;
+}
+
+static void virtnet_rq_set_premapped(struct virtnet_info *vi)
+{
+	int i;
+
+	/* disable for big mode */
+	if (!vi->mergeable_rx_bufs && vi->big_packets)
+		return;
+
+	for (i = 0; i < vi->max_queue_pairs; i++) {
+		if (virtqueue_set_dma_premapped(vi->rq[i].vq))
+			continue;
+
+		vi->rq[i].do_dma = true;
+	}
+}
+
 static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
 {
 	unsigned int len;
@@ -835,7 +999,7 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
 		void *buf;
 		int off;
 
-		buf = virtqueue_get_buf(rq->vq, &buflen);
+		buf = virtnet_rq_get_buf(rq, &buflen, NULL);
 		if (unlikely(!buf))
 			goto err_buf;
 
@@ -1126,7 +1290,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
 		return -EINVAL;
 
 	while (--*num_buf > 0) {
-		buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx);
+		buf = virtnet_rq_get_buf(rq, &len, &ctx);
 		if (unlikely(!buf)) {
 			pr_debug("%s: rx error: %d buffers out of %d missing\n",
 				 dev->name, *num_buf,
@@ -1351,7 +1515,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 	while (--num_buf) {
 		int num_skb_frags;
 
-		buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx);
+		buf = virtnet_rq_get_buf(rq, &len, &ctx);
 		if (unlikely(!buf)) {
 			pr_debug("%s: rx error: %d buffers out of %d missing\n",
 				 dev->name, num_buf,
@@ -1414,7 +1578,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 err_skb:
 	put_page(page);
 	while (num_buf-- > 1) {
-		buf = virtqueue_get_buf(rq->vq, &len);
+		buf = virtnet_rq_get_buf(rq, &len, NULL);
 		if (unlikely(!buf)) {
 			pr_debug("%s: rx error: %d buffers missing\n",
 				 dev->name, num_buf);
@@ -1524,7 +1688,6 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
 			     gfp_t gfp)
 {
-	struct page_frag *alloc_frag = &rq->alloc_frag;
 	char *buf;
 	unsigned int xdp_headroom = virtnet_get_headroom(vi);
 	void *ctx = (void *)(unsigned long)xdp_headroom;
@@ -1533,17 +1696,21 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
 
 	len = SKB_DATA_ALIGN(len) +
 	      SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
-	if (unlikely(!skb_page_frag_refill(len, alloc_frag, gfp)))
+
+	buf = virtnet_rq_alloc(rq, len, gfp);
+	if (unlikely(!buf))
 		return -ENOMEM;
 
-	buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
-	get_page(alloc_frag->page);
-	alloc_frag->offset += len;
-	sg_init_one(rq->sg, buf + VIRTNET_RX_PAD + xdp_headroom,
-		    vi->hdr_len + GOOD_PACKET_LEN);
+	virtnet_rq_init_one_sg(rq, buf + VIRTNET_RX_PAD + xdp_headroom,
+			       vi->hdr_len + GOOD_PACKET_LEN);
+
 	err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
-	if (err < 0)
+	if (err < 0) {
+		if (rq->do_dma)
+			virtnet_rq_unmap(rq, buf, 0);
 		put_page(virt_to_head_page(buf));
+	}
+
 	return err;
 }
 
@@ -1620,23 +1787,22 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
 	unsigned int headroom = virtnet_get_headroom(vi);
 	unsigned int tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
 	unsigned int room = SKB_DATA_ALIGN(headroom + tailroom);
-	char *buf;
+	unsigned int len, hole;
 	void *ctx;
+	char *buf;
 	int err;
-	unsigned int len, hole;
 
 	/* Extra tailroom is needed to satisfy XDP's assumption. This
 	 * means rx frags coalescing won't work, but consider we've
 	 * disabled GSO for XDP, it won't be a big issue.
 	 */
 	len = get_mergeable_buf_len(rq, &rq->mrg_avg_pkt_len, room);
-	if (unlikely(!skb_page_frag_refill(len + room, alloc_frag, gfp)))
+
+	buf = virtnet_rq_alloc(rq, len + room, gfp);
+	if (unlikely(!buf))
 		return -ENOMEM;
 
-	buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
 	buf += headroom; /* advance address leaving hole at front of pkt */
-	get_page(alloc_frag->page);
-	alloc_frag->offset += len + room;
 	hole = alloc_frag->size - alloc_frag->offset;
 	if (hole < len + room) {
 		/* To avoid internal fragmentation, if there is very likely not
@@ -1650,11 +1816,15 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
 		alloc_frag->offset += hole;
 	}
 
-	sg_init_one(rq->sg, buf, len);
+	virtnet_rq_init_one_sg(rq, buf, len);
+
 	ctx = mergeable_len_to_ctx(len + room, headroom);
 	err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
-	if (err < 0)
+	if (err < 0) {
+		if (rq->do_dma)
+			virtnet_rq_unmap(rq, buf, 0);
 		put_page(virt_to_head_page(buf));
+	}
 
 	return err;
 }
@@ -1775,13 +1945,13 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
 		void *ctx;
 
 		while (stats.packets < budget &&
-		       (buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx))) {
+		       (buf = virtnet_rq_get_buf(rq, &len, &ctx))) {
 			receive_buf(vi, rq, buf, len, ctx, xdp_xmit, &stats);
 			stats.packets++;
 		}
 	} else {
 		while (stats.packets < budget &&
-		       (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
+		       (buf = virtnet_rq_get_buf(rq, &len, NULL)) != NULL) {
 			receive_buf(vi, rq, buf, len, NULL, xdp_xmit, &stats);
 			stats.packets++;
 		}
@@ -3553,8 +3723,11 @@ static void free_receive_page_frags(struct virtnet_info *vi)
 {
 	int i;
 	for (i = 0; i < vi->max_queue_pairs; i++)
-		if (vi->rq[i].alloc_frag.page)
+		if (vi->rq[i].alloc_frag.page) {
+			if (vi->rq[i].do_dma && vi->rq[i].last_dma)
+				virtnet_rq_unmap(&vi->rq[i], vi->rq[i].last_dma, 0);
 			put_page(vi->rq[i].alloc_frag.page);
+		}
 }
 
 static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf)
@@ -3591,9 +3764,10 @@ static void free_unused_bufs(struct virtnet_info *vi)
 	}
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {
-		struct virtqueue *vq = vi->rq[i].vq;
-		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
-			virtnet_rq_free_unused_buf(vq, buf);
+		struct receive_queue *rq = &vi->rq[i];
+
+		while ((buf = virtnet_rq_detach_unused_buf(rq)) != NULL)
+			virtnet_rq_free_unused_buf(rq->vq, buf);
 		cond_resched();
 	}
 }
@@ -3767,6 +3941,8 @@ static int init_vqs(struct virtnet_info *vi)
 	if (ret)
 		goto err_free;
 
+	virtnet_rq_set_premapped(vi);
+
 	cpus_read_lock();
 	virtnet_set_affinity(vi);
 	cpus_read_unlock();
-- 
2.32.0.3.g01195cf9f

Re: [PATCH vhost v13 12/12] virtio_net: merge dma operations when filling mergeable buffers

[Michael S. Tsirkin]

On Thu, Aug 10, 2023 at 08:30:57PM +0800, Xuan Zhuo wrote:
> Currently, the virtio core will perform a dma operation for each
> buffer. Although, the same page may be operated multiple times.
> 
> This patch, the driver does the dma operation and manages the dma
> address based the feature premapped of virtio core.
> 
> This way, we can perform only one dma operation for the pages of the
> alloc frag. This is beneficial for the iommu device.
> 
> kernel command line: intel_iommu=on iommu.passthrough=0
> 
>        |  strict=0  | strict=1
> Before |  775496pps | 428614pps
> After  | 1109316pps | 742853pps
> 
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Hi Xuan Zhuo,
looks like this patch is causing regressions. Do you have time to debug
or should I revert?

> ---
>  drivers/net/virtio_net.c | 228 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 202 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 486b5849033d..16adb5ef18f8 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -126,6 +126,14 @@ static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
>  #define VIRTNET_SQ_STATS_LEN	ARRAY_SIZE(virtnet_sq_stats_desc)
>  #define VIRTNET_RQ_STATS_LEN	ARRAY_SIZE(virtnet_rq_stats_desc)
>  
> +/* The dma information of pages allocated at a time. */
> +struct virtnet_rq_dma {
> +	dma_addr_t addr;
> +	u32 ref;
> +	u16 len;
> +	u16 need_sync;
> +};
> +
>  /* Internal representation of a send virtqueue */
>  struct send_queue {
>  	/* Virtqueue associated with this send _queue */
> @@ -175,6 +183,12 @@ struct receive_queue {
>  	char name[16];
>  
>  	struct xdp_rxq_info xdp_rxq;
> +
> +	/* Record the last dma info to free after new pages is allocated. */
> +	struct virtnet_rq_dma *last_dma;
> +
> +	/* Do dma by self */
> +	bool do_dma;
>  };
>  
>  /* This structure can contain rss message with maximum settings for indirection table and keysize
> @@ -549,6 +563,156 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>  	return skb;
>  }
>  
> +static void virtnet_rq_unmap(struct receive_queue *rq, void *buf, u32 len)
> +{
> +	struct page *page = virt_to_head_page(buf);
> +	struct virtnet_rq_dma *dma;
> +	void *head;
> +	int offset;
> +
> +	head = page_address(page);
> +
> +	dma = head;
> +
> +	--dma->ref;
> +
> +	if (dma->ref) {
> +		if (dma->need_sync && len) {
> +			offset = buf - (head + sizeof(*dma));
> +
> +			virtqueue_dma_sync_single_range_for_cpu(rq->vq, dma->addr, offset,
> +								len, DMA_FROM_DEVICE);
> +		}
> +
> +		return;
> +	}
> +
> +	virtqueue_dma_unmap_single_attrs(rq->vq, dma->addr, dma->len,
> +					 DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> +	put_page(page);
> +}
> +
> +static void *virtnet_rq_get_buf(struct receive_queue *rq, u32 *len, void **ctx)
> +{
> +	void *buf;
> +
> +	buf = virtqueue_get_buf_ctx(rq->vq, len, ctx);
> +	if (buf && rq->do_dma)
> +		virtnet_rq_unmap(rq, buf, *len);
> +
> +	return buf;
> +}
> +
> +static void *virtnet_rq_detach_unused_buf(struct receive_queue *rq)
> +{
> +	void *buf;
> +
> +	buf = virtqueue_detach_unused_buf(rq->vq);
> +	if (buf && rq->do_dma)
> +		virtnet_rq_unmap(rq, buf, 0);
> +
> +	return buf;
> +}
> +
> +static void virtnet_rq_init_one_sg(struct receive_queue *rq, void *buf, u32 len)
> +{
> +	struct virtnet_rq_dma *dma;
> +	dma_addr_t addr;
> +	u32 offset;
> +	void *head;
> +
> +	if (!rq->do_dma) {
> +		sg_init_one(rq->sg, buf, len);
> +		return;
> +	}
> +
> +	head = page_address(rq->alloc_frag.page);
> +
> +	offset = buf - head;
> +
> +	dma = head;
> +
> +	addr = dma->addr - sizeof(*dma) + offset;
> +
> +	sg_init_table(rq->sg, 1);
> +	rq->sg[0].dma_address = addr;
> +	rq->sg[0].length = len;
> +}
> +
> +static void *virtnet_rq_alloc(struct receive_queue *rq, u32 size, gfp_t gfp)
> +{
> +	struct page_frag *alloc_frag = &rq->alloc_frag;
> +	struct virtnet_rq_dma *dma;
> +	void *buf, *head;
> +	dma_addr_t addr;
> +
> +	if (unlikely(!skb_page_frag_refill(size, alloc_frag, gfp)))
> +		return NULL;
> +
> +	head = page_address(alloc_frag->page);
> +
> +	if (rq->do_dma) {
> +		dma = head;
> +
> +		/* new pages */
> +		if (!alloc_frag->offset) {
> +			if (rq->last_dma) {
> +				/* Now, the new page is allocated, the last dma
> +				 * will not be used. So the dma can be unmapped
> +				 * if the ref is 0.
> +				 */
> +				virtnet_rq_unmap(rq, rq->last_dma, 0);
> +				rq->last_dma = NULL;
> +			}
> +
> +			dma->len = alloc_frag->size - sizeof(*dma);
> +
> +			addr = virtqueue_dma_map_single_attrs(rq->vq, dma + 1,
> +							      dma->len, DMA_FROM_DEVICE, 0);
> +			if (virtqueue_dma_mapping_error(rq->vq, addr))
> +				return NULL;
> +
> +			dma->addr = addr;
> +			dma->need_sync = virtqueue_dma_need_sync(rq->vq, addr);
> +
> +			/* Add a reference to dma to prevent the entire dma from
> +			 * being released during error handling. This reference
> +			 * will be freed after the pages are no longer used.
> +			 */
> +			get_page(alloc_frag->page);
> +			dma->ref = 1;
> +			alloc_frag->offset = sizeof(*dma);
> +
> +			rq->last_dma = dma;
> +		}
> +
> +		++dma->ref;
> +	}
> +
> +	buf = head + alloc_frag->offset;
> +
> +	get_page(alloc_frag->page);
> +	alloc_frag->offset += size;
> +
> +	return buf;
> +}
> +
> +static void virtnet_rq_set_premapped(struct virtnet_info *vi)
> +{
> +	int i;
> +
> +	/* disable for big mode */
> +	if (!vi->mergeable_rx_bufs && vi->big_packets)
> +		return;
> +
> +	for (i = 0; i < vi->max_queue_pairs; i++) {
> +		if (virtqueue_set_dma_premapped(vi->rq[i].vq))
> +			continue;
> +
> +		vi->rq[i].do_dma = true;
> +	}
> +}
> +
>  static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
>  {
>  	unsigned int len;
> @@ -835,7 +999,7 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
>  		void *buf;
>  		int off;
>  
> -		buf = virtqueue_get_buf(rq->vq, &buflen);
> +		buf = virtnet_rq_get_buf(rq, &buflen, NULL);
>  		if (unlikely(!buf))
>  			goto err_buf;
>  
> @@ -1126,7 +1290,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
>  		return -EINVAL;
>  
>  	while (--*num_buf > 0) {
> -		buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx);
> +		buf = virtnet_rq_get_buf(rq, &len, &ctx);
>  		if (unlikely(!buf)) {
>  			pr_debug("%s: rx error: %d buffers out of %d missing\n",
>  				 dev->name, *num_buf,
> @@ -1351,7 +1515,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  	while (--num_buf) {
>  		int num_skb_frags;
>  
> -		buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx);
> +		buf = virtnet_rq_get_buf(rq, &len, &ctx);
>  		if (unlikely(!buf)) {
>  			pr_debug("%s: rx error: %d buffers out of %d missing\n",
>  				 dev->name, num_buf,
> @@ -1414,7 +1578,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  err_skb:
>  	put_page(page);
>  	while (num_buf-- > 1) {
> -		buf = virtqueue_get_buf(rq->vq, &len);
> +		buf = virtnet_rq_get_buf(rq, &len, NULL);
>  		if (unlikely(!buf)) {
>  			pr_debug("%s: rx error: %d buffers missing\n",
>  				 dev->name, num_buf);
> @@ -1524,7 +1688,6 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>  static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
>  			     gfp_t gfp)
>  {
> -	struct page_frag *alloc_frag = &rq->alloc_frag;
>  	char *buf;
>  	unsigned int xdp_headroom = virtnet_get_headroom(vi);
>  	void *ctx = (void *)(unsigned long)xdp_headroom;
> @@ -1533,17 +1696,21 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
>  
>  	len = SKB_DATA_ALIGN(len) +
>  	      SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> -	if (unlikely(!skb_page_frag_refill(len, alloc_frag, gfp)))
> +
> +	buf = virtnet_rq_alloc(rq, len, gfp);
> +	if (unlikely(!buf))
>  		return -ENOMEM;
>  
> -	buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> -	get_page(alloc_frag->page);
> -	alloc_frag->offset += len;
> -	sg_init_one(rq->sg, buf + VIRTNET_RX_PAD + xdp_headroom,
> -		    vi->hdr_len + GOOD_PACKET_LEN);
> +	virtnet_rq_init_one_sg(rq, buf + VIRTNET_RX_PAD + xdp_headroom,
> +			       vi->hdr_len + GOOD_PACKET_LEN);
> +
>  	err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
> -	if (err < 0)
> +	if (err < 0) {
> +		if (rq->do_dma)
> +			virtnet_rq_unmap(rq, buf, 0);
>  		put_page(virt_to_head_page(buf));
> +	}
> +
>  	return err;
>  }
>  
> @@ -1620,23 +1787,22 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
>  	unsigned int headroom = virtnet_get_headroom(vi);
>  	unsigned int tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
>  	unsigned int room = SKB_DATA_ALIGN(headroom + tailroom);
> -	char *buf;
> +	unsigned int len, hole;
>  	void *ctx;
> +	char *buf;
>  	int err;
> -	unsigned int len, hole;
>  
>  	/* Extra tailroom is needed to satisfy XDP's assumption. This
>  	 * means rx frags coalescing won't work, but consider we've
>  	 * disabled GSO for XDP, it won't be a big issue.
>  	 */
>  	len = get_mergeable_buf_len(rq, &rq->mrg_avg_pkt_len, room);
> -	if (unlikely(!skb_page_frag_refill(len + room, alloc_frag, gfp)))
> +
> +	buf = virtnet_rq_alloc(rq, len + room, gfp);
> +	if (unlikely(!buf))
>  		return -ENOMEM;
>  
> -	buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
>  	buf += headroom; /* advance address leaving hole at front of pkt */
> -	get_page(alloc_frag->page);
> -	alloc_frag->offset += len + room;
>  	hole = alloc_frag->size - alloc_frag->offset;
>  	if (hole < len + room) {
>  		/* To avoid internal fragmentation, if there is very likely not
> @@ -1650,11 +1816,15 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
>  		alloc_frag->offset += hole;
>  	}
>  
> -	sg_init_one(rq->sg, buf, len);
> +	virtnet_rq_init_one_sg(rq, buf, len);
> +
>  	ctx = mergeable_len_to_ctx(len + room, headroom);
>  	err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
> -	if (err < 0)
> +	if (err < 0) {
> +		if (rq->do_dma)
> +			virtnet_rq_unmap(rq, buf, 0);
>  		put_page(virt_to_head_page(buf));
> +	}
>  
>  	return err;
>  }
> @@ -1775,13 +1945,13 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>  		void *ctx;
>  
>  		while (stats.packets < budget &&
> -		       (buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx))) {
> +		       (buf = virtnet_rq_get_buf(rq, &len, &ctx))) {
>  			receive_buf(vi, rq, buf, len, ctx, xdp_xmit, &stats);
>  			stats.packets++;
>  		}
>  	} else {
>  		while (stats.packets < budget &&
> -		       (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
> +		       (buf = virtnet_rq_get_buf(rq, &len, NULL)) != NULL) {
>  			receive_buf(vi, rq, buf, len, NULL, xdp_xmit, &stats);
>  			stats.packets++;
>  		}
> @@ -3553,8 +3723,11 @@ static void free_receive_page_frags(struct virtnet_info *vi)
>  {
>  	int i;
>  	for (i = 0; i < vi->max_queue_pairs; i++)
> -		if (vi->rq[i].alloc_frag.page)
> +		if (vi->rq[i].alloc_frag.page) {
> +			if (vi->rq[i].do_dma && vi->rq[i].last_dma)
> +				virtnet_rq_unmap(&vi->rq[i], vi->rq[i].last_dma, 0);
>  			put_page(vi->rq[i].alloc_frag.page);
> +		}
>  }
>  
>  static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf)
> @@ -3591,9 +3764,10 @@ static void free_unused_bufs(struct virtnet_info *vi)
>  	}
>  
>  	for (i = 0; i < vi->max_queue_pairs; i++) {
> -		struct virtqueue *vq = vi->rq[i].vq;
> -		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
> -			virtnet_rq_free_unused_buf(vq, buf);
> +		struct receive_queue *rq = &vi->rq[i];
> +
> +		while ((buf = virtnet_rq_detach_unused_buf(rq)) != NULL)
> +			virtnet_rq_free_unused_buf(rq->vq, buf);
>  		cond_resched();
>  	}
>  }
> @@ -3767,6 +3941,8 @@ static int init_vqs(struct virtnet_info *vi)
>  	if (ret)
>  		goto err_free;
>  
> +	virtnet_rq_set_premapped(vi);
> +
>  	cpus_read_lock();
>  	virtnet_set_affinity(vi);
>  	cpus_read_unlock();
> -- 
> 2.32.0.3.g01195cf9f

Re: [PATCH vhost v13 12/12] virtio_net: merge dma operations when filling mergeable buffers

[Xuan Zhuo]

On Tue, 26 Sep 2023 12:01:34 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Aug 10, 2023 at 08:30:57PM +0800, Xuan Zhuo wrote:
> > Currently, the virtio core will perform a dma operation for each
> > buffer. Although, the same page may be operated multiple times.
> >
> > This patch, the driver does the dma operation and manages the dma
> > address based the feature premapped of virtio core.
> >
> > This way, we can perform only one dma operation for the pages of the
> > alloc frag. This is beneficial for the iommu device.
> >
> > kernel command line: intel_iommu=on iommu.passthrough=0
> >
> >        |  strict=0  | strict=1
> > Before |  775496pps | 428614pps
> > After  | 1109316pps | 742853pps
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> Hi Xuan Zhuo,
> looks like this patch is causing regressions. Do you have time to debug
> or should I revert?


I have sended a fix to Michael Roth.

I will wait his response. If that is ok, I will post that fix commit.

Thanks.


>
> > ---
> >  drivers/net/virtio_net.c | 228 ++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 202 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 486b5849033d..16adb5ef18f8 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -126,6 +126,14 @@ static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
> >  #define VIRTNET_SQ_STATS_LEN	ARRAY_SIZE(virtnet_sq_stats_desc)
> >  #define VIRTNET_RQ_STATS_LEN	ARRAY_SIZE(virtnet_rq_stats_desc)
> >
> > +/* The dma information of pages allocated at a time. */
> > +struct virtnet_rq_dma {
> > +	dma_addr_t addr;
> > +	u32 ref;
> > +	u16 len;
> > +	u16 need_sync;
> > +};
> > +
> >  /* Internal representation of a send virtqueue */
> >  struct send_queue {
> >  	/* Virtqueue associated with this send _queue */
> > @@ -175,6 +183,12 @@ struct receive_queue {
> >  	char name[16];
> >
> >  	struct xdp_rxq_info xdp_rxq;
> > +
> > +	/* Record the last dma info to free after new pages is allocated. */
> > +	struct virtnet_rq_dma *last_dma;
> > +
> > +	/* Do dma by self */
> > +	bool do_dma;
> >  };
> >
> >  /* This structure can contain rss message with maximum settings for indirection table and keysize
> > @@ -549,6 +563,156 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >  	return skb;
> >  }
> >
> > +static void virtnet_rq_unmap(struct receive_queue *rq, void *buf, u32 len)
> > +{
> > +	struct page *page = virt_to_head_page(buf);
> > +	struct virtnet_rq_dma *dma;
> > +	void *head;
> > +	int offset;
> > +
> > +	head = page_address(page);
> > +
> > +	dma = head;
> > +
> > +	--dma->ref;
> > +
> > +	if (dma->ref) {
> > +		if (dma->need_sync && len) {
> > +			offset = buf - (head + sizeof(*dma));
> > +
> > +			virtqueue_dma_sync_single_range_for_cpu(rq->vq, dma->addr, offset,
> > +								len, DMA_FROM_DEVICE);
> > +		}
> > +
> > +		return;
> > +	}
> > +
> > +	virtqueue_dma_unmap_single_attrs(rq->vq, dma->addr, dma->len,
> > +					 DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
> > +	put_page(page);
> > +}
> > +
> > +static void *virtnet_rq_get_buf(struct receive_queue *rq, u32 *len, void **ctx)
> > +{
> > +	void *buf;
> > +
> > +	buf = virtqueue_get_buf_ctx(rq->vq, len, ctx);
> > +	if (buf && rq->do_dma)
> > +		virtnet_rq_unmap(rq, buf, *len);
> > +
> > +	return buf;
> > +}
> > +
> > +static void *virtnet_rq_detach_unused_buf(struct receive_queue *rq)
> > +{
> > +	void *buf;
> > +
> > +	buf = virtqueue_detach_unused_buf(rq->vq);
> > +	if (buf && rq->do_dma)
> > +		virtnet_rq_unmap(rq, buf, 0);
> > +
> > +	return buf;
> > +}
> > +
> > +static void virtnet_rq_init_one_sg(struct receive_queue *rq, void *buf, u32 len)
> > +{
> > +	struct virtnet_rq_dma *dma;
> > +	dma_addr_t addr;
> > +	u32 offset;
> > +	void *head;
> > +
> > +	if (!rq->do_dma) {
> > +		sg_init_one(rq->sg, buf, len);
> > +		return;
> > +	}
> > +
> > +	head = page_address(rq->alloc_frag.page);
> > +
> > +	offset = buf - head;
> > +
> > +	dma = head;
> > +
> > +	addr = dma->addr - sizeof(*dma) + offset;
> > +
> > +	sg_init_table(rq->sg, 1);
> > +	rq->sg[0].dma_address = addr;
> > +	rq->sg[0].length = len;
> > +}
> > +
> > +static void *virtnet_rq_alloc(struct receive_queue *rq, u32 size, gfp_t gfp)
> > +{
> > +	struct page_frag *alloc_frag = &rq->alloc_frag;
> > +	struct virtnet_rq_dma *dma;
> > +	void *buf, *head;
> > +	dma_addr_t addr;
> > +
> > +	if (unlikely(!skb_page_frag_refill(size, alloc_frag, gfp)))
> > +		return NULL;
> > +
> > +	head = page_address(alloc_frag->page);
> > +
> > +	if (rq->do_dma) {
> > +		dma = head;
> > +
> > +		/* new pages */
> > +		if (!alloc_frag->offset) {
> > +			if (rq->last_dma) {
> > +				/* Now, the new page is allocated, the last dma
> > +				 * will not be used. So the dma can be unmapped
> > +				 * if the ref is 0.
> > +				 */
> > +				virtnet_rq_unmap(rq, rq->last_dma, 0);
> > +				rq->last_dma = NULL;
> > +			}
> > +
> > +			dma->len = alloc_frag->size - sizeof(*dma);
> > +
> > +			addr = virtqueue_dma_map_single_attrs(rq->vq, dma + 1,
> > +							      dma->len, DMA_FROM_DEVICE, 0);
> > +			if (virtqueue_dma_mapping_error(rq->vq, addr))
> > +				return NULL;
> > +
> > +			dma->addr = addr;
> > +			dma->need_sync = virtqueue_dma_need_sync(rq->vq, addr);
> > +
> > +			/* Add a reference to dma to prevent the entire dma from
> > +			 * being released during error handling. This reference
> > +			 * will be freed after the pages are no longer used.
> > +			 */
> > +			get_page(alloc_frag->page);
> > +			dma->ref = 1;
> > +			alloc_frag->offset = sizeof(*dma);
> > +
> > +			rq->last_dma = dma;
> > +		}
> > +
> > +		++dma->ref;
> > +	}
> > +
> > +	buf = head + alloc_frag->offset;
> > +
> > +	get_page(alloc_frag->page);
> > +	alloc_frag->offset += size;
> > +
> > +	return buf;
> > +}
> > +
> > +static void virtnet_rq_set_premapped(struct virtnet_info *vi)
> > +{
> > +	int i;
> > +
> > +	/* disable for big mode */
> > +	if (!vi->mergeable_rx_bufs && vi->big_packets)
> > +		return;
> > +
> > +	for (i = 0; i < vi->max_queue_pairs; i++) {
> > +		if (virtqueue_set_dma_premapped(vi->rq[i].vq))
> > +			continue;
> > +
> > +		vi->rq[i].do_dma = true;
> > +	}
> > +}
> > +
> >  static void free_old_xmit_skbs(struct send_queue *sq, bool in_napi)
> >  {
> >  	unsigned int len;
> > @@ -835,7 +999,7 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
> >  		void *buf;
> >  		int off;
> >
> > -		buf = virtqueue_get_buf(rq->vq, &buflen);
> > +		buf = virtnet_rq_get_buf(rq, &buflen, NULL);
> >  		if (unlikely(!buf))
> >  			goto err_buf;
> >
> > @@ -1126,7 +1290,7 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
> >  		return -EINVAL;
> >
> >  	while (--*num_buf > 0) {
> > -		buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx);
> > +		buf = virtnet_rq_get_buf(rq, &len, &ctx);
> >  		if (unlikely(!buf)) {
> >  			pr_debug("%s: rx error: %d buffers out of %d missing\n",
> >  				 dev->name, *num_buf,
> > @@ -1351,7 +1515,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >  	while (--num_buf) {
> >  		int num_skb_frags;
> >
> > -		buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx);
> > +		buf = virtnet_rq_get_buf(rq, &len, &ctx);
> >  		if (unlikely(!buf)) {
> >  			pr_debug("%s: rx error: %d buffers out of %d missing\n",
> >  				 dev->name, num_buf,
> > @@ -1414,7 +1578,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> >  err_skb:
> >  	put_page(page);
> >  	while (num_buf-- > 1) {
> > -		buf = virtqueue_get_buf(rq->vq, &len);
> > +		buf = virtnet_rq_get_buf(rq, &len, NULL);
> >  		if (unlikely(!buf)) {
> >  			pr_debug("%s: rx error: %d buffers missing\n",
> >  				 dev->name, num_buf);
> > @@ -1524,7 +1688,6 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >  static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
> >  			     gfp_t gfp)
> >  {
> > -	struct page_frag *alloc_frag = &rq->alloc_frag;
> >  	char *buf;
> >  	unsigned int xdp_headroom = virtnet_get_headroom(vi);
> >  	void *ctx = (void *)(unsigned long)xdp_headroom;
> > @@ -1533,17 +1696,21 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
> >
> >  	len = SKB_DATA_ALIGN(len) +
> >  	      SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > -	if (unlikely(!skb_page_frag_refill(len, alloc_frag, gfp)))
> > +
> > +	buf = virtnet_rq_alloc(rq, len, gfp);
> > +	if (unlikely(!buf))
> >  		return -ENOMEM;
> >
> > -	buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> > -	get_page(alloc_frag->page);
> > -	alloc_frag->offset += len;
> > -	sg_init_one(rq->sg, buf + VIRTNET_RX_PAD + xdp_headroom,
> > -		    vi->hdr_len + GOOD_PACKET_LEN);
> > +	virtnet_rq_init_one_sg(rq, buf + VIRTNET_RX_PAD + xdp_headroom,
> > +			       vi->hdr_len + GOOD_PACKET_LEN);
> > +
> >  	err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
> > -	if (err < 0)
> > +	if (err < 0) {
> > +		if (rq->do_dma)
> > +			virtnet_rq_unmap(rq, buf, 0);
> >  		put_page(virt_to_head_page(buf));
> > +	}
> > +
> >  	return err;
> >  }
> >
> > @@ -1620,23 +1787,22 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
> >  	unsigned int headroom = virtnet_get_headroom(vi);
> >  	unsigned int tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
> >  	unsigned int room = SKB_DATA_ALIGN(headroom + tailroom);
> > -	char *buf;
> > +	unsigned int len, hole;
> >  	void *ctx;
> > +	char *buf;
> >  	int err;
> > -	unsigned int len, hole;
> >
> >  	/* Extra tailroom is needed to satisfy XDP's assumption. This
> >  	 * means rx frags coalescing won't work, but consider we've
> >  	 * disabled GSO for XDP, it won't be a big issue.
> >  	 */
> >  	len = get_mergeable_buf_len(rq, &rq->mrg_avg_pkt_len, room);
> > -	if (unlikely(!skb_page_frag_refill(len + room, alloc_frag, gfp)))
> > +
> > +	buf = virtnet_rq_alloc(rq, len + room, gfp);
> > +	if (unlikely(!buf))
> >  		return -ENOMEM;
> >
> > -	buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> >  	buf += headroom; /* advance address leaving hole at front of pkt */
> > -	get_page(alloc_frag->page);
> > -	alloc_frag->offset += len + room;
> >  	hole = alloc_frag->size - alloc_frag->offset;
> >  	if (hole < len + room) {
> >  		/* To avoid internal fragmentation, if there is very likely not
> > @@ -1650,11 +1816,15 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
> >  		alloc_frag->offset += hole;
> >  	}
> >
> > -	sg_init_one(rq->sg, buf, len);
> > +	virtnet_rq_init_one_sg(rq, buf, len);
> > +
> >  	ctx = mergeable_len_to_ctx(len + room, headroom);
> >  	err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
> > -	if (err < 0)
> > +	if (err < 0) {
> > +		if (rq->do_dma)
> > +			virtnet_rq_unmap(rq, buf, 0);
> >  		put_page(virt_to_head_page(buf));
> > +	}
> >
> >  	return err;
> >  }
> > @@ -1775,13 +1945,13 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> >  		void *ctx;
> >
> >  		while (stats.packets < budget &&
> > -		       (buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx))) {
> > +		       (buf = virtnet_rq_get_buf(rq, &len, &ctx))) {
> >  			receive_buf(vi, rq, buf, len, ctx, xdp_xmit, &stats);
> >  			stats.packets++;
> >  		}
> >  	} else {
> >  		while (stats.packets < budget &&
> > -		       (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
> > +		       (buf = virtnet_rq_get_buf(rq, &len, NULL)) != NULL) {
> >  			receive_buf(vi, rq, buf, len, NULL, xdp_xmit, &stats);
> >  			stats.packets++;
> >  		}
> > @@ -3553,8 +3723,11 @@ static void free_receive_page_frags(struct virtnet_info *vi)
> >  {
> >  	int i;
> >  	for (i = 0; i < vi->max_queue_pairs; i++)
> > -		if (vi->rq[i].alloc_frag.page)
> > +		if (vi->rq[i].alloc_frag.page) {
> > +			if (vi->rq[i].do_dma && vi->rq[i].last_dma)
> > +				virtnet_rq_unmap(&vi->rq[i], vi->rq[i].last_dma, 0);
> >  			put_page(vi->rq[i].alloc_frag.page);
> > +		}
> >  }
> >
> >  static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf)
> > @@ -3591,9 +3764,10 @@ static void free_unused_bufs(struct virtnet_info *vi)
> >  	}
> >
> >  	for (i = 0; i < vi->max_queue_pairs; i++) {
> > -		struct virtqueue *vq = vi->rq[i].vq;
> > -		while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
> > -			virtnet_rq_free_unused_buf(vq, buf);
> > +		struct receive_queue *rq = &vi->rq[i];
> > +
> > +		while ((buf = virtnet_rq_detach_unused_buf(rq)) != NULL)
> > +			virtnet_rq_free_unused_buf(rq->vq, buf);
> >  		cond_resched();
> >  	}
> >  }
> > @@ -3767,6 +3941,8 @@ static int init_vqs(struct virtnet_info *vi)
> >  	if (ret)
> >  		goto err_free;
> >
> > +	virtnet_rq_set_premapped(vi);
> > +
> >  	cpus_read_lock();
> >  	virtnet_set_affinity(vi);
> >  	cpus_read_unlock();
> > --
> > 2.32.0.3.g01195cf9f
>