On Mon, 26 Feb 2024 07:01:11 -0500, "Michael S. Tsirkin" [off-list ref] wrote:

On Mon, Feb 26, 2024 at 07:00:09AM -0500, Michael S. Tsirkin wrote:

quoted

On Mon, Feb 26, 2024 at 07:41:20PM +0800, Xuan Zhuo wrote:

quoted

On Mon, 26 Feb 2024 06:39:51 -0500, "Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Mon, Feb 26, 2024 at 05:24:11PM +0800, Xuan Zhuo wrote:

quoted

On Sun, 25 Feb 2024 03:38:48 -0500, "Michael S. Tsirkin" [off-list ref] wrote:

quoted

On Fri, Feb 23, 2024 at 04:27:26PM +0800, Xuan Zhuo wrote:

quoted

If the xsk is enabling, the xsk tx will share the send queue.
But the xsk requires that the send queue use the premapped mode.
So the send queue must support premapped mode.

cmd:
    sh samples/pktgen/pktgen_sample01_simple.sh -i eth0 \
        -s 16 -d 10.0.0.128 -m 00:16:3e:2c:c8:2e -n 0 -p 100
CPU:
    Intel(R) Xeon(R) Platinum 8369B CPU @ 2.70GHz

Machine:
    ecs.g7.2xlarge(Aliyun)

before:              1600010.00
after(no-premapped): 1599966.00
after(premapped):    1600014.00

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

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7715bb7032ec..b83ef6afc4fb 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -146,6 +146,25 @@ struct virtnet_rq_dma {
 	u16 need_sync;
 };

+struct virtnet_sq_dma {
+	union {
+		struct virtnet_sq_dma *next;
+		void *data;
+	};
+
+	u32 num;
+
+	dma_addr_t addr[MAX_SKB_FRAGS + 2];
+	u32 len[MAX_SKB_FRAGS + 2];
+};
+
+struct virtnet_sq_dma_head {
+	/* record for kfree */
+	void *p;
+
+	struct virtnet_sq_dma *free;
+};
+
 /* Internal representation of a send virtqueue */
 struct send_queue {
 	/* Virtqueue associated with this send _queue */

@@ -165,6 +184,8 @@ struct send_queue {

 	/* Record whether sq is in reset state. */
 	bool reset;
+
+	struct virtnet_sq_dma_head dmainfo;
 };

 /* Internal representation of a receive virtqueue */

@@ -368,6 +389,95 @@ static struct xdp_frame *ptr_to_xdp(void *ptr)
 	return (struct xdp_frame *)((unsigned long)ptr & ~VIRTIO_XDP_FLAG);
 }

+static struct virtnet_sq_dma *virtnet_sq_unmap(struct send_queue *sq, void **data)
+{
+	struct virtnet_sq_dma *d;
+	int i;
+
+	d = *data;
+	*data = d->data;
+
+	for (i = 0; i < d->num; ++i)
+		virtqueue_dma_unmap_page_attrs(sq->vq, d->addr[i], d->len[i],
+					       DMA_TO_DEVICE, 0);
+
+	d->next = sq->dmainfo.free;
+	sq->dmainfo.free = d;
+
+	return d;
+}
+
+static struct virtnet_sq_dma *virtnet_sq_map_sg(struct send_queue *sq,
+						int nents, void *data)
+{
+	struct virtnet_sq_dma *d;
+	struct scatterlist *sg;
+	int i;
+
+	if (!sq->dmainfo.free)
+		return NULL;
+
+	d = sq->dmainfo.free;
+	sq->dmainfo.free = d->next;
+
+	for_each_sg(sq->sg, sg, nents, i) {
+		if (virtqueue_dma_map_sg_attrs(sq->vq, sg, DMA_TO_DEVICE, 0))
+			goto err;
+
+		d->addr[i] = sg->dma_address;
+		d->len[i] = sg->length;
+	}
+
+	d->data = data;
+	d->num = i;
+	return d;
+
+err:
+	d->num = i;
+	virtnet_sq_unmap(sq, (void **)&d);
+	return NULL;
+}

Do I see a reimplementation of linux/llist.h here?

quoted

+
+static int virtnet_add_outbuf(struct send_queue *sq, u32 num, void *data)
+{
+	int ret;
+
+	if (sq->vq->premapped) {
+		data = virtnet_sq_map_sg(sq, num, data);
+		if (!data)
+			return -ENOMEM;
+	}
+
+	ret = virtqueue_add_outbuf(sq->vq, sq->sg, num, data, GFP_ATOMIC);
+	if (ret && sq->vq->premapped)
+		virtnet_sq_unmap(sq, &data);
+
+	return ret;
+}
+
+static int virtnet_sq_init_dma_mate(struct send_queue *sq)

Mate? The popular south african drink?

quoted

+{
+	struct virtnet_sq_dma *d;
+	int num, i;
+
+	num = virtqueue_get_vring_size(sq->vq);
+
+	sq->dmainfo.free = kcalloc(num, sizeof(*sq->dmainfo.free), GFP_KERNEL);
+	if (!sq->dmainfo.free)
+		return -ENOMEM;

This could be quite a bit of memory for a large queue.  And for a bunch
of common cases where unmap is a nop (e.g. iommu pt) this does nothing
useful at all.  And also, this does nothing useful if PLATFORM_ACCESS is off
which is super common.

A while ago I proposed:
- extend DMA APIs so one can query whether unmap is a nop

We may have trouble for this.

dma_addr_t dma_map_page_attrs(struct device *dev, struct page *page,
		size_t offset, size_t size, enum dma_data_direction dir,
		unsigned long attrs)
{
	const struct dma_map_ops *ops = get_dma_ops(dev);
	dma_addr_t addr;

	BUG_ON(!valid_dma_direction(dir));

	if (WARN_ON_ONCE(!dev->dma_mask))
		return DMA_MAPPING_ERROR;

	if (dma_map_direct(dev, ops) ||
	    arch_dma_map_page_direct(dev, page_to_phys(page) + offset + size))
		addr = dma_direct_map_page(dev, page, offset, size, dir, attrs);
	else
		addr = ops->map_page(dev, page, offset, size, dir, attrs);
	kmsan_handle_dma(page, offset, size, dir);
	debug_dma_map_page(dev, page, offset, size, dir, addr, attrs);

	return addr;
}

arch_dma_map_page_direct will check the dma address.
So we can not judge by the API in advance.

Thanks.

So if dma_map_direct is false we'll still waste some memory.
So be it.

arch_dma_map_page_direct default is marco (false), just for powerpc
it is a function. So I think we can skip it.

If the dma_map_direct is false, I think should save the dma info.

Thanks.

Would already be an improvement.
But can we have better names?

I'd prefer:

dma_can_skip_unmap
dma_can_skip_sync

Because we do not know for sure if it's direct unless
we have the page.

we might need to add these callbacks to dma ops too, I think
with iommu pt that is the case and it is pretty common,
right?

I agree.

But this patch set has 19 commits, I hope to do this on a new patchset on the
top of this.

If we need to check the iommu pt, I need to study the iommu first.

Thanks.

quoted

--
MST