vhost: exclude VM hugepages from coredumps

PATCH Superseded Mike Pattrick 5 participants 19 messages Dec 6, 2022 – Mar 6, 2023

Share & tools

Mike Pattrick

author

2022-12-06 15:05 UTC #0 lore raw

PATCH v2 1/1 vhost: exclude VM hugepages from coredumps

Currently if an application wants to include shared hugepages in coredumps in conjunction with the vhost library, the coredump will be larger than expected and include unneeded virtual machine memory.

This patch will mark all vhost huge pages as DONTDUMP, except for some select pages used by DPDK.

Signed-off-by Mike Pattrick <[email protected]>

---
v2:
* Removed warning on unsupported platforms

---
 lib/vhost/iotlb.c      |  5 +++++
 lib/vhost/vhost.h      | 12 ++++++++++++
 lib/vhost/vhost_user.c | 10 ++++++++++
 3 files changed, 27 insertions(+)

lib/vhost/iotlb.c +5 -0

index 6a729e8804..2f89f88817 100644
@@ -149,6 +149,7 @@ vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq)
 	rte_rwlock_write_lock(&vq->iotlb_lock);
 
 	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
+		mem_set_dump((void *)node->uaddr, node->size, true);
 		TAILQ_REMOVE(&vq->iotlb_list, node, next);
 		vhost_user_iotlb_pool_put(vq, node);
 	}

@@ -170,6 +171,7 @@ vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq)
 
 	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
 		if (!entry_idx) {
+			mem_set_dump((void *)node->uaddr, node->size, true);
 			TAILQ_REMOVE(&vq->iotlb_list, node, next);
 			vhost_user_iotlb_pool_put(vq, node);
 			vq->iotlb_cache_nr--;

@@ -222,12 +224,14 @@ vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq
 			vhost_user_iotlb_pool_put(vq, new_node);
 			goto unlock;
 		} else if (node->iova > new_node->iova) {
+			mem_set_dump((void *)node->uaddr, node->size, true);
 			TAILQ_INSERT_BEFORE(node, new_node, next);
 			vq->iotlb_cache_nr++;
 			goto unlock;
 		}
 	}
 
+	mem_set_dump((void *)node->uaddr, node->size, true);
 	TAILQ_INSERT_TAIL(&vq->iotlb_list, new_node, next);
 	vq->iotlb_cache_nr++;

@@ -255,6 +259,7 @@ vhost_user_iotlb_cache_remove(struct vhost_virtqueue *vq,
 			break;
 
 		if (iova < node->iova + node->size) {
+			mem_set_dump((void *)node->uaddr, node->size, true);
 			TAILQ_REMOVE(&vq->iotlb_list, node, next);
 			vhost_user_iotlb_pool_put(vq, node);
 			vq->iotlb_cache_nr--;

lib/vhost/vhost.h +12 -0

index ef211ed519..1f913803f6 100644
@@ -13,6 +13,7 @@
 #include <linux/virtio_net.h>
 #include <sys/socket.h>
 #include <linux/if.h>
+#include <sys/mman.h>
 
 #include <rte_log.h>
 #include <rte_ether.h>

@@ -987,4 +988,15 @@ mbuf_is_consumed(struct rte_mbuf *m)
 
 	return true;
 }
+
+static __rte_always_inline void
+mem_set_dump(__rte_unused void *ptr, __rte_unused size_t size, __rte_unused bool enable)
+{
+#ifdef MADV_DONTDUMP
+	if (madvise(ptr, size, enable ? MADV_DODUMP : MADV_DONTDUMP) == -1) {
+		rte_log(RTE_LOG_INFO, vhost_config_log_level,
+			"VHOST_CONFIG: could not set coredump preference (%s).\n", strerror(errno));
+	}
+#endif
+}
 #endif /* _VHOST_NET_CDEV_H_ */

lib/vhost/vhost_user.c +10 -0

index 9902ae9944..8f33d5f4d9 100644
@@ -793,6 +793,9 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
 			return;
 		}
 
+		mem_set_dump(vq->desc_packed, len, true);
+		mem_set_dump(vq->driver_event, len, true);
+		mem_set_dump(vq->device_event, len, true);
 		vq->access_ok = true;
 		return;
 	}

@@ -846,6 +849,9 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
 			"some packets maybe resent for Tx and dropped for Rx\n");
 	}
 
+	mem_set_dump(vq->desc, len, true);
+	mem_set_dump(vq->avail, len, true);
+	mem_set_dump(vq->used, len, true);
 	vq->access_ok = true;
 
 	VHOST_LOG_CONFIG(dev->ifname, DEBUG, "mapped address desc: %p\n", vq->desc);

@@ -1224,6 +1230,7 @@ vhost_user_mmap_region(struct virtio_net *dev,
 	region->mmap_addr = mmap_addr;
 	region->mmap_size = mmap_size;
 	region->host_user_addr = (uint64_t)(uintptr_t)mmap_addr + mmap_offset;
+	mem_set_dump(mmap_addr, mmap_size, false);
 
 	if (dev->async_copy) {
 		if (add_guest_pages(dev, region, alignment) < 0) {

@@ -1528,6 +1535,7 @@ inflight_mem_alloc(struct virtio_net *dev, const char *name, size_t size, int *f
 		return NULL;
 	}
 
+	mem_set_dump(ptr, size, false);
 	*fd = mfd;
 	return ptr;
 }

@@ -1736,6 +1744,7 @@ vhost_user_set_inflight_fd(struct virtio_net **pdev,
 		dev->inflight_info->fd = -1;
 	}
 
+	mem_set_dump(addr, mmap_size, false);
 	dev->inflight_info->fd = fd;
 	dev->inflight_info->addr = addr;
 	dev->inflight_info->size = mmap_size;

@@ -2283,6 +2292,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
 	dev->log_addr = (uint64_t)(uintptr_t)addr;
 	dev->log_base = dev->log_addr + off;
 	dev->log_size = size;
+	mem_set_dump(addr, size, false);
 
 	for (i = 0; i < dev->nr_vring; i++) {
 		struct vhost_virtqueue *vq = dev->virtqueue[i];

Signature · 1 line

2.31.1

Mike Pattrick

author

2022-12-07 16:54 UTC #1 lore raw

PATCH v3 1/1 vhost: exclude VM hugepages from coredumps

This patch will mark all vhost huge pages as DONTDUMP, except for some select pages used by DPDK.

Signed-off-by Mike Pattrick <[email protected]>

---
v2:
* Removed warning on unsupported platforms

v3:
* Removed pointer warning on 32bit platforms

---
 lib/vhost/iotlb.c      |  5 +++++
 lib/vhost/vhost.h      | 12 ++++++++++++
 lib/vhost/vhost_user.c | 10 ++++++++++
 3 files changed, 27 insertions(+)

lib/vhost/iotlb.c +5 -0

index 6a729e8804..a0b8fd7302 100644
@@ -149,6 +149,7 @@ vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq)
 	rte_rwlock_write_lock(&vq->iotlb_lock);
 
 	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
+		mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
 		TAILQ_REMOVE(&vq->iotlb_list, node, next);
 		vhost_user_iotlb_pool_put(vq, node);
 	}

@@ -170,6 +171,7 @@ vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq)
 
 	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
 		if (!entry_idx) {
+			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
 			TAILQ_REMOVE(&vq->iotlb_list, node, next);
 			vhost_user_iotlb_pool_put(vq, node);
 			vq->iotlb_cache_nr--;

@@ -222,12 +224,14 @@ vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq
 			vhost_user_iotlb_pool_put(vq, new_node);
 			goto unlock;
 		} else if (node->iova > new_node->iova) {
+			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
 			TAILQ_INSERT_BEFORE(node, new_node, next);
 			vq->iotlb_cache_nr++;
 			goto unlock;
 		}
 	}
 
+	mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
 	TAILQ_INSERT_TAIL(&vq->iotlb_list, new_node, next);
 	vq->iotlb_cache_nr++;

@@ -255,6 +259,7 @@ vhost_user_iotlb_cache_remove(struct vhost_virtqueue *vq,
 			break;
 
 		if (iova < node->iova + node->size) {
+			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
 			TAILQ_REMOVE(&vq->iotlb_list, node, next);
 			vhost_user_iotlb_pool_put(vq, node);
 			vq->iotlb_cache_nr--;

lib/vhost/vhost.h +12 -0

index ef211ed519..1f913803f6 100644
@@ -13,6 +13,7 @@
 #include <linux/virtio_net.h>
 #include <sys/socket.h>
 #include <linux/if.h>
+#include <sys/mman.h>
 
 #include <rte_log.h>
 #include <rte_ether.h>

@@ -987,4 +988,15 @@ mbuf_is_consumed(struct rte_mbuf *m)
 
 	return true;
 }
+
+static __rte_always_inline void
+mem_set_dump(__rte_unused void *ptr, __rte_unused size_t size, __rte_unused bool enable)
+{
+#ifdef MADV_DONTDUMP
+	if (madvise(ptr, size, enable ? MADV_DODUMP : MADV_DONTDUMP) == -1) {
+		rte_log(RTE_LOG_INFO, vhost_config_log_level,
+			"VHOST_CONFIG: could not set coredump preference (%s).\n", strerror(errno));
+	}
+#endif
+}
 #endif /* _VHOST_NET_CDEV_H_ */

lib/vhost/vhost_user.c +10 -0

index 9902ae9944..8f33d5f4d9 100644
@@ -793,6 +793,9 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
 			return;
 		}
 
+		mem_set_dump(vq->desc_packed, len, true);
+		mem_set_dump(vq->driver_event, len, true);
+		mem_set_dump(vq->device_event, len, true);
 		vq->access_ok = true;
 		return;
 	}

@@ -846,6 +849,9 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
 			"some packets maybe resent for Tx and dropped for Rx\n");
 	}
 
+	mem_set_dump(vq->desc, len, true);
+	mem_set_dump(vq->avail, len, true);
+	mem_set_dump(vq->used, len, true);
 	vq->access_ok = true;
 
 	VHOST_LOG_CONFIG(dev->ifname, DEBUG, "mapped address desc: %p\n", vq->desc);

@@ -1224,6 +1230,7 @@ vhost_user_mmap_region(struct virtio_net *dev,
 	region->mmap_addr = mmap_addr;
 	region->mmap_size = mmap_size;
 	region->host_user_addr = (uint64_t)(uintptr_t)mmap_addr + mmap_offset;
+	mem_set_dump(mmap_addr, mmap_size, false);
 
 	if (dev->async_copy) {
 		if (add_guest_pages(dev, region, alignment) < 0) {

@@ -1528,6 +1535,7 @@ inflight_mem_alloc(struct virtio_net *dev, const char *name, size_t size, int *f
 		return NULL;
 	}
 
+	mem_set_dump(ptr, size, false);
 	*fd = mfd;
 	return ptr;
 }

@@ -1736,6 +1744,7 @@ vhost_user_set_inflight_fd(struct virtio_net **pdev,
 		dev->inflight_info->fd = -1;
 	}
 
+	mem_set_dump(addr, mmap_size, false);
 	dev->inflight_info->fd = fd;
 	dev->inflight_info->addr = addr;
 	dev->inflight_info->size = mmap_size;

@@ -2283,6 +2292,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
 	dev->log_addr = (uint64_t)(uintptr_t)addr;
 	dev->log_base = dev->log_addr + off;
 	dev->log_size = size;
+	mem_set_dump(addr, size, false);
 
 	for (i = 0; i < dev->nr_vring; i++) {
 		struct vhost_virtqueue *vq = dev->virtqueue[i];

Signature · 1 line

2.31.1

Maxime Coquelin

Reviewed-by on v3 1/1

2022-12-20 12:43 UTC #2 lore raw

Hi Mike,

Quoted 13 lines · Mike Pattrick

Currently if an application wants to include shared hugepages in coredumps in conjunction with the vhost library, the coredump will be larger than expected and include unneeded virtual machine memory.
This patch will mark all vhost huge pages as DONTDUMP, except for some select pages used by DPDK.
Signed-off-by Mike Pattrick <[email protected]>
---
v2:
* Removed warning on unsupported platforms

Quoted 9 lines

v3:
* Removed pointer warning on 32bit platforms

---
  lib/vhost/iotlb.c      |  5 +++++
  lib/vhost/vhost.h      | 12 ++++++++++++
  lib/vhost/vhost_user.c | 10 ++++++++++
  3 files changed, 27 insertions(+)

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Maxime Coquelin

Applied

2023-02-03 14:50 UTC #3 lore raw

In reply to Mike Pattrick

Quoted 13 lines · Mike Pattrick

Currently if an application wants to include shared hugepages in coredumps in conjunction with the vhost library, the coredump will be larger than expected and include unneeded virtual machine memory.
This patch will mark all vhost huge pages as DONTDUMP, except for some select pages used by DPDK.
Signed-off-by Mike Pattrick <[email protected]>
---
v2:
* Removed warning on unsupported platforms

Quoted 9 lines

v3:
* Removed pointer warning on 32bit platforms

---
  lib/vhost/iotlb.c      |  5 +++++
  lib/vhost/vhost.h      | 12 ++++++++++++
  lib/vhost/vhost_user.c | 10 ++++++++++
  3 files changed, 27 insertions(+)

Applied to dpdk-next-virtio/main.

Thanks,
Maxime

David Marchand

2023-02-10 15:53 UTC #4 lore raw

In reply to Mike Pattrick

Hello Mike,

Quoted 9 lines · Mike Pattrick

Currently if an application wants to include shared hugepages in coredumps in conjunction with the vhost library, the coredump will be larger than expected and include unneeded virtual machine memory.
This patch will mark all vhost huge pages as DONTDUMP, except for some select pages used by DPDK.
Signed-off-by Mike Pattrick <[email protected]>

I noticed the following warnings today on my f37 kernel, while running a vhost-user/virtio-user testpmd setup on next-virtio branch. Linux dmarchan 6.1.9-200.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Feb 2 00:21:48 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
My system has 2M hugepages, only.

$ rm vhost-net; strace -e trace=madvise -f
./build-clang/app/dpdk-testpmd --in-memory --no-pci
--vdev=net_vhost0,iface=./vhost-net,client=1 -- -i

$ ./build-clang/app/dpdk-testpmd --in-memory --single-file-segment
--no-pci --vdev
'net_virtio_user0,mac=00:01:02:03:04:05,path=./vhost-net,server=1'  --
-i

Then, on the "vhost side" testpmd:
...
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_BASE
VHOST_CONFIG: (./vhost-net) vring base idx:0 last_used_idx:0 last_avail_idx:0.
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_ADDR
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_KICK
VHOST_CONFIG: (./vhost-net) vring kick idx:0 file:391
[pid 59565] madvise(0x7fa6d8da4000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument). [pid 59565] madvise(0x7fa6d8da5000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument). [pid 59565] madvise(0x7fa6d8da6000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument).
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_BASE

Looking at the whole trace, only madvise calls with MADV_DODUMP (with all of them for a 2052 size) fail.

I did not investigate further.
Could you have a look please?

Signature · 1 line

David Marchand

Maxime Coquelin

2023-02-10 15:59 UTC #5 lore raw

Thanks David for the report.

Quoted 11 lines · David Marchand

Hello Mike,
Quoted 8 lines · Mike Pattrick
Currently if an application wants to include shared hugepages in coredumps in conjunction with the vhost library, the coredump will be larger than expected and include unneeded virtual machine memory.
This patch will mark all vhost huge pages as DONTDUMP, except for some select pages used by DPDK.

Quoted 7 lines

Quoted 1 line
Signed-off-by Mike Pattrick <[email protected]>
I noticed the following warnings today on my f37 kernel, while running a vhost-user/virtio-user testpmd setup on next-virtio branch. Linux dmarchan 6.1.9-200.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Feb 2 00:21:48 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
My system has 2M hugepages, only.

FYI, I don't see it on my system which is using 2MB hugepages too:
Linux max-t490s 6.1.9-100.fc36.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Feb 2 03:27:13 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Almost the same Kernel BTW.

Quoted 30 lines

$ rm vhost-net; strace -e trace=madvise -f
./build-clang/app/dpdk-testpmd --in-memory --no-pci
--vdev=net_vhost0,iface=./vhost-net,client=1 -- -i
$ ./build-clang/app/dpdk-testpmd --in-memory --single-file-segment
--no-pci --vdev
'net_virtio_user0,mac=00:01:02:03:04:05,path=./vhost-net,server=1'  --
-i
Then, on the "vhost side" testpmd:
...
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_BASE
VHOST_CONFIG: (./vhost-net) vring base idx:0 last_used_idx:0 last_avail_idx:0.
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_ADDR
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_KICK
VHOST_CONFIG: (./vhost-net) vring kick idx:0 file:391
[pid 59565] madvise(0x7fa6d8da4000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument). [pid 59565] madvise(0x7fa6d8da5000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument). [pid 59565] madvise(0x7fa6d8da6000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument).
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_BASE

Quoted 7 lines

Looking at the whole trace, only madvise calls with MADV_DODUMP (with all of them for a 2052 size) fail.
I did not investigate further.
Could you have a look please?

Maxime

Mike Pattrick

author

2023-02-10 21:13 UTC #6 lore raw

In reply to David Marchand

Quoted 50 lines · David Marchand

Hello Mike,
Quoted 9 lines · Mike Pattrick
Currently if an application wants to include shared hugepages in coredumps in conjunction with the vhost library, the coredump will be larger than expected and include unneeded virtual machine memory.
This patch will mark all vhost huge pages as DONTDUMP, except for some select pages used by DPDK.
Signed-off-by Mike Pattrick <[email protected]>
I noticed the following warnings today on my f37 kernel, while running a vhost-user/virtio-user testpmd setup on next-virtio branch. Linux dmarchan 6.1.9-200.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Feb 2 00:21:48 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
My system has 2M hugepages, only.
$ rm vhost-net; strace -e trace=madvise -f
./build-clang/app/dpdk-testpmd --in-memory --no-pci
--vdev=net_vhost0,iface=./vhost-net,client=1 -- -i
$ ./build-clang/app/dpdk-testpmd --in-memory --single-file-segment
--no-pci --vdev
'net_virtio_user0,mac=00:01:02:03:04:05,path=./vhost-net,server=1'  --
-i
Then, on the "vhost side" testpmd:
...
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_BASE
VHOST_CONFIG: (./vhost-net) vring base idx:0 last_used_idx:0 last_avail_idx:0.
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_ADDR
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_KICK
VHOST_CONFIG: (./vhost-net) vring kick idx:0 file:391
[pid 59565] madvise(0x7fa6d8da4000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument). [pid 59565] madvise(0x7fa6d8da5000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument). [pid 59565] madvise(0x7fa6d8da6000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument).
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_BASE

Quoted 6 lines

Looking at the whole trace, only madvise calls with MADV_DODUMP (with all of them for a 2052 size) fail.
I did not investigate further.
Could you have a look please?

I tried it on that exact kernel and also ran into this issue. I'll check it out in more depth.

-M

Quoted 4 lines

--
David Marchand

Maxime Coquelin

2023-02-21 16:26 UTC #7 lore raw

Hi Mike,

Quoted 55 lines · Mike Pattrick

Quoted 53 lines · David Marchand
Hello Mike,
Quoted 9 lines · Mike Pattrick
Currently if an application wants to include shared hugepages in coredumps in conjunction with the vhost library, the coredump will be larger than expected and include unneeded virtual machine memory.
This patch will mark all vhost huge pages as DONTDUMP, except for some select pages used by DPDK.
Signed-off-by Mike Pattrick <[email protected]>
I noticed the following warnings today on my f37 kernel, while running a vhost-user/virtio-user testpmd setup on next-virtio branch. Linux dmarchan 6.1.9-200.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Feb 2 00:21:48 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
My system has 2M hugepages, only.
$ rm vhost-net; strace -e trace=madvise -f
./build-clang/app/dpdk-testpmd --in-memory --no-pci
--vdev=net_vhost0,iface=./vhost-net,client=1 -- -i
$ ./build-clang/app/dpdk-testpmd --in-memory --single-file-segment
--no-pci --vdev
'net_virtio_user0,mac=00:01:02:03:04:05,path=./vhost-net,server=1'  --
-i
Then, on the "vhost side" testpmd:
...
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_BASE
VHOST_CONFIG: (./vhost-net) vring base idx:0 last_used_idx:0 last_avail_idx:0.
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_ADDR
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_KICK
VHOST_CONFIG: (./vhost-net) vring kick idx:0 file:391
[pid 59565] madvise(0x7fa6d8da4000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument). [pid 59565] madvise(0x7fa6d8da5000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument). [pid 59565] madvise(0x7fa6d8da6000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument).
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_BASE
Looking at the whole trace, only madvise calls with MADV_DODUMP (with all of them for a 2052 size) fail.

Quoted 6 lines

Quoted 3 lines
I did not investigate further.
Could you have a look please?
I tried it on that exact kernel and also ran into this issue. I'll check it out in more depth.

Gentle reminder, have you found the root cause for this issue?

Thanks,
Maxime

Quoted 7 lines

-M
Quoted 4 lines
--
David Marchand

Mike Pattrick

author

2023-02-22 14:00 UTC #8 lore raw

Quoted 59 lines · Maxime Coquelin

Hi Mike,
Quoted 55 lines · Mike Pattrick
Quoted 53 lines · David Marchand
Hello Mike,
Quoted 9 lines · Mike Pattrick
Currently if an application wants to include shared hugepages in coredumps in conjunction with the vhost library, the coredump will be larger than expected and include unneeded virtual machine memory.
This patch will mark all vhost huge pages as DONTDUMP, except for some select pages used by DPDK.
Signed-off-by Mike Pattrick <[email protected]>
I noticed the following warnings today on my f37 kernel, while running a vhost-user/virtio-user testpmd setup on next-virtio branch. Linux dmarchan 6.1.9-200.fc37.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Feb 2 00:21:48 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
My system has 2M hugepages, only.
$ rm vhost-net; strace -e trace=madvise -f
./build-clang/app/dpdk-testpmd --in-memory --no-pci
--vdev=net_vhost0,iface=./vhost-net,client=1 -- -i
$ ./build-clang/app/dpdk-testpmd --in-memory --single-file-segment
--no-pci --vdev
'net_virtio_user0,mac=00:01:02:03:04:05,path=./vhost-net,server=1'  --
-i
Then, on the "vhost side" testpmd:
...
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_BASE
VHOST_CONFIG: (./vhost-net) vring base idx:0 last_used_idx:0 last_avail_idx:0.
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_ADDR
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_KICK
VHOST_CONFIG: (./vhost-net) vring kick idx:0 file:391
[pid 59565] madvise(0x7fa6d8da4000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument). [pid 59565] madvise(0x7fa6d8da5000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument). [pid 59565] madvise(0x7fa6d8da6000, 2052, MADV_DODUMP) = -1 EINVAL (Invalid argument)
VHOST_CONFIG: could not set coredump preference (Invalid argument).
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: (./vhost-net) read message VHOST_USER_SET_VRING_BASE
Looking at the whole trace, only madvise calls with MADV_DODUMP (with all of them for a 2052 size) fail.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
I did not investigate further.
Could you have a look please?
I tried it on that exact kernel and also ran into this issue. I'll check it out in more depth.
Gentle reminder, have you found the root cause for this issue?

Yes, thanks for the ping. I should have a patch soon.

Cheers,
M

Quoted 12 lines

Thanks,
Maxime
Quoted 7 lines
-M
Quoted 4 lines
--
David Marchand

Mike Pattrick

author

2023-02-22 21:44 UTC #9 lore raw

PATCH 1/1 vhost: fix madvise arguments alignment

In reply to Maxime Coquelin

The arguments passed to madvise should be aligned to the alignment of the backing memory. Now we keep track of each regions alignment and use then when setting coredump preferences. To facilitate this, a new member was added to rte_vhost_mem_region. A new function was added to easily translate memory address back to region alignment. Unneeded calls to madvise were reduced, as the cache removal case should already be covered by the cache insertion case. The previously inline function mem_set_dump was removed from a header file and made not inline.

Fixes 338ad77c9ed3 ("vhost: exclude VM hugepages from coredumps")

Signed-off-by Mike Pattrick <[email protected]>

---
 lib/vhost/iotlb.c      |  9 +++---
 lib/vhost/rte_vhost.h  |  1 +
 lib/vhost/vhost.h      | 12 ++------
 lib/vhost/vhost_user.c | 63 +++++++++++++++++++++++++++++++++++-------
 4 files changed, 60 insertions(+), 25 deletions(-)

lib/vhost/iotlb.c +4 -5

index a0b8fd7302..aa5ba3ba6d 100644
@@ -149,7 +149,6 @@ vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq)
 	rte_rwlock_write_lock(&vq->iotlb_lock);
 
 	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
-		mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
 		TAILQ_REMOVE(&vq->iotlb_list, node, next);
 		vhost_user_iotlb_pool_put(vq, node);
 	}

@@ -171,7 +170,6 @@ vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq)
 
 	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
 		if (!entry_idx) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
 			TAILQ_REMOVE(&vq->iotlb_list, node, next);
 			vhost_user_iotlb_pool_put(vq, node);
 			vq->iotlb_cache_nr--;

@@ -224,14 +222,16 @@ vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq
 			vhost_user_iotlb_pool_put(vq, new_node);
 			goto unlock;
 		} else if (node->iova > new_node->iova) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+			mem_set_dump((void *)(uintptr_t)new_node->uaddr, new_node->size, true,
+				hua_to_alignment(dev->mem, (void *)node->uaddr));
 			TAILQ_INSERT_BEFORE(node, new_node, next);
 			vq->iotlb_cache_nr++;
 			goto unlock;
 		}
 	}
 
-	mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+	mem_set_dump((void *)(uintptr_t)new_node->uaddr, new_node->size, true,
+		hua_to_alignment(dev->mem, (void *)(uintptr_t)new_node->uaddr));
 	TAILQ_INSERT_TAIL(&vq->iotlb_list, new_node, next);
 	vq->iotlb_cache_nr++;

@@ -259,7 +259,6 @@ vhost_user_iotlb_cache_remove(struct vhost_virtqueue *vq,
 			break;
 
 		if (iova < node->iova + node->size) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
 			TAILQ_REMOVE(&vq->iotlb_list, node, next);
 			vhost_user_iotlb_pool_put(vq, node);
 			vq->iotlb_cache_nr--;

lib/vhost/rte_vhost.h +1 -0

index a395843fe9..c5c97ea67e 100644
@@ -136,6 +136,7 @@ struct rte_vhost_mem_region {
 	void	 *mmap_addr;
 	uint64_t mmap_size;
 	int fd;
+	uint64_t alignment;
 };
 
 /**

lib/vhost/vhost.h +2 ~~-10~~

index 5750f0c005..a2467ba509 100644
@@ -1009,14 +1009,6 @@ mbuf_is_consumed(struct rte_mbuf *m)
 	return true;
 }
 
-static __rte_always_inline void
-mem_set_dump(__rte_unused void *ptr, __rte_unused size_t size, __rte_unused bool enable)
-{
-#ifdef MADV_DONTDUMP
-	if (madvise(ptr, size, enable ? MADV_DODUMP : MADV_DONTDUMP) == -1) {
-		rte_log(RTE_LOG_INFO, vhost_config_log_level,
-			"VHOST_CONFIG: could not set coredump preference (%s).\n", strerror(errno));
-	}
-#endif
-}
+uint64_t hua_to_alignment(struct rte_vhost_memory *mem, void *ptr);
+void mem_set_dump(void *ptr, size_t size, bool enable, uint64_t alignment);
 #endif /* _VHOST_NET_CDEV_H_ */

lib/vhost/vhost_user.c +53 ~~-10~~

162 lines not on this page · raw message

Signature · 1 line

2.31.1

Mike Pattrick

author

2023-02-23 04:35 UTC #10 lore raw

PATCH v2 1/1 vhost: fix madvise arguments alignment

Fixes 338ad77c9ed3 ("vhost: exclude VM hugepages from coredumps")

Signed-off-by Mike Pattrick <[email protected]>

---
Since v1:
- Corrected a cast for 32bit compiles

---
 lib/vhost/iotlb.c      |  9 +++---
 lib/vhost/rte_vhost.h  |  1 +
 lib/vhost/vhost.h      | 12 ++------
 lib/vhost/vhost_user.c | 63 +++++++++++++++++++++++++++++++++++-------
 4 files changed, 60 insertions(+), 25 deletions(-)

lib/vhost/iotlb.c +4 -5

index a0b8fd7302..5293507b63 100644
@@ -149,7 +149,6 @@ vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq)
 	rte_rwlock_write_lock(&vq->iotlb_lock);
 
 	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
-		mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
 		TAILQ_REMOVE(&vq->iotlb_list, node, next);
 		vhost_user_iotlb_pool_put(vq, node);
 	}

@@ -171,7 +170,6 @@ vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq)
 
 	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
 		if (!entry_idx) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
 			TAILQ_REMOVE(&vq->iotlb_list, node, next);
 			vhost_user_iotlb_pool_put(vq, node);
 			vq->iotlb_cache_nr--;

@@ -224,14 +222,16 @@ vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq
 			vhost_user_iotlb_pool_put(vq, new_node);
 			goto unlock;
 		} else if (node->iova > new_node->iova) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+			mem_set_dump((void *)(uintptr_t)new_node->uaddr, new_node->size, true,
+				hua_to_alignment(dev->mem, (void *)(uintptr_t)node->uaddr));
 			TAILQ_INSERT_BEFORE(node, new_node, next);
 			vq->iotlb_cache_nr++;
 			goto unlock;
 		}
 	}
 
-	mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+	mem_set_dump((void *)(uintptr_t)new_node->uaddr, new_node->size, true,
+		hua_to_alignment(dev->mem, (void *)(uintptr_t)new_node->uaddr));
 	TAILQ_INSERT_TAIL(&vq->iotlb_list, new_node, next);
 	vq->iotlb_cache_nr++;

@@ -259,7 +259,6 @@ vhost_user_iotlb_cache_remove(struct vhost_virtqueue *vq,
 			break;
 
 		if (iova < node->iova + node->size) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
 			TAILQ_REMOVE(&vq->iotlb_list, node, next);
 			vhost_user_iotlb_pool_put(vq, node);
 			vq->iotlb_cache_nr--;

lib/vhost/rte_vhost.h +1 -0

index a395843fe9..c5c97ea67e 100644
@@ -136,6 +136,7 @@ struct rte_vhost_mem_region {
 	void	 *mmap_addr;
 	uint64_t mmap_size;
 	int fd;
+	uint64_t alignment;
 };
 
 /**

lib/vhost/vhost.h +2 ~~-10~~

index 5750f0c005..a2467ba509 100644
@@ -1009,14 +1009,6 @@ mbuf_is_consumed(struct rte_mbuf *m)
 	return true;
 }
 
-static __rte_always_inline void
-mem_set_dump(__rte_unused void *ptr, __rte_unused size_t size, __rte_unused bool enable)
-{
-#ifdef MADV_DONTDUMP
-	if (madvise(ptr, size, enable ? MADV_DODUMP : MADV_DONTDUMP) == -1) {
-		rte_log(RTE_LOG_INFO, vhost_config_log_level,
-			"VHOST_CONFIG: could not set coredump preference (%s).\n", strerror(errno));
-	}
-#endif
-}
+uint64_t hua_to_alignment(struct rte_vhost_memory *mem, void *ptr);
+void mem_set_dump(void *ptr, size_t size, bool enable, uint64_t alignment);
 #endif /* _VHOST_NET_CDEV_H_ */

lib/vhost/vhost_user.c +53 ~~-10~~

162 lines not on this page · raw message

Signature · 1 line

2.31.1

Maxime Coquelin

2023-02-23 16:12 UTC #11 lore raw

Re: [PATCH v2] vhost: fix madvise arguments alignment

Hi Mike,

Thanks for looking into this issue.

Quoted 27 lines · Mike Pattrick

The arguments passed to madvise should be aligned to the alignment of the backing memory. Now we keep track of each regions alignment and use then when setting coredump preferences. To facilitate this, a new member was added to rte_vhost_mem_region. A new function was added to easily translate memory address back to region alignment. Unneeded calls to madvise were reduced, as the cache removal case should already be covered by the cache insertion case. The previously inline function mem_set_dump was removed from a header file and made not inline.
Fixes 338ad77c9ed3 ("vhost: exclude VM hugepages from coredumps")
Signed-off-by Mike Pattrick <[email protected]>
---
Since v1:
- Corrected a cast for 32bit compiles
---
  lib/vhost/iotlb.c      |  9 +++---
  lib/vhost/rte_vhost.h  |  1 +
  lib/vhost/vhost.h      | 12 ++------
  lib/vhost/vhost_user.c | 63 +++++++++++++++++++++++++++++++++++-------
  4 files changed, 60 insertions(+), 25 deletions(-)
lib/vhost/iotlb.c +0 -0
index a0b8fd7302..5293507b63 100644

lib/vhost/iotlb.c vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq) · line 152

Quoted 4 lines

lib/vhost/iotlb.c +0 -1

@@ -149,7 +149,6 @@ vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq)
  	rte_rwlock_write_lock(&vq->iotlb_lock);
 
  	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
-		mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);

Hmm, it should have been called with enable=false here since we are removing the entry from the IOTLB cache. It should be kept in order to "DONTDUMP" pages evicted from the cache.

lib/vhost/iotlb.c vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq) · line 174

Quoted 8 lines

lib/vhost/iotlb.c +0 -1

  		TAILQ_REMOVE(&vq->iotlb_list, node, next);
  		vhost_user_iotlb_pool_put(vq, node);
  	}

@@ -171,7 +170,6 @@ vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq)
 
  	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
  		if (!entry_idx) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);

Same here.

Quoted 35 lines

lib/vhost/iotlb.c +4 -3

  			TAILQ_REMOVE(&vq->iotlb_list, node, next);
  			vhost_user_iotlb_pool_put(vq, node);
  			vq->iotlb_cache_nr--;

@@ -224,14 +222,16 @@ vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq
  			vhost_user_iotlb_pool_put(vq, new_node);
  			goto unlock;
  		} else if (node->iova > new_node->iova) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+			mem_set_dump((void *)(uintptr_t)new_node->uaddr, new_node->size, true,
+				hua_to_alignment(dev->mem, (void *)(uintptr_t)node->uaddr));
  			TAILQ_INSERT_BEFORE(node, new_node, next);
  			vq->iotlb_cache_nr++;
  			goto unlock;
  		}
  	}
 
-	mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+	mem_set_dump((void *)(uintptr_t)new_node->uaddr, new_node->size, true,
+		hua_to_alignment(dev->mem, (void *)(uintptr_t)new_node->uaddr));
  	TAILQ_INSERT_TAIL(&vq->iotlb_list, new_node, next);
  	vq->iotlb_cache_nr++;

@@ -259,7 +259,6 @@ vhost_user_iotlb_cache_remove(struct vhost_virtqueue *vq,
  			break;
 
  		if (iova < node->iova + node->size) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
  			TAILQ_REMOVE(&vq->iotlb_list, node, next);
  			vhost_user_iotlb_pool_put(vq, node);
  			vq->iotlb_cache_nr--;

lib/vhost/rte_vhost.h +0 -0

index a395843fe9..c5c97ea67e 100644

lib/vhost/rte_vhost.h struct rte_vhost_mem_region { · line 139

Quoted 4 lines

lib/vhost/rte_vhost.h +1 -0

@@ -136,6 +136,7 @@ struct rte_vhost_mem_region {
  	void	 *mmap_addr;
  	uint64_t mmap_size;
  	int fd;
+	uint64_t alignment;

This is not possible to do this as it breaks the ABI. You have to store the information somewhere else, or simply call get_blk_size() in hua_to_alignment() since the fd is not closed.

Quoted 206 lines

lib/vhost/rte_vhost.h +0 -0

  };
 
  /**

lib/vhost/vhost.h +2 ~~-10~~

index 5750f0c005..a2467ba509 100644
@@ -1009,14 +1009,6 @@ mbuf_is_consumed(struct rte_mbuf *m)
  	return true;
  }
 
-static __rte_always_inline void
-mem_set_dump(__rte_unused void *ptr, __rte_unused size_t size, __rte_unused bool enable)
-{
-#ifdef MADV_DONTDUMP
-	if (madvise(ptr, size, enable ? MADV_DODUMP : MADV_DONTDUMP) == -1) {
-		rte_log(RTE_LOG_INFO, vhost_config_log_level,
-			"VHOST_CONFIG: could not set coredump preference (%s).\n", strerror(errno));
-	}
-#endif
-}
+uint64_t hua_to_alignment(struct rte_vhost_memory *mem, void *ptr);
+void mem_set_dump(void *ptr, size_t size, bool enable, uint64_t alignment);
  #endif /* _VHOST_NET_CDEV_H_ */

lib/vhost/vhost_user.c +53 ~~-10~~

index d702d082dd..6d09597fbe 100644
@@ -737,6 +737,40 @@ log_addr_to_gpa(struct virtio_net *dev, struct vhost_virtqueue *vq)
  	return log_gpa;
  }
 
+uint64_t
+hua_to_alignment(struct rte_vhost_memory *mem, void *ptr)
+{
+	struct rte_vhost_mem_region *r;
+	uint32_t i;
+	uintptr_t hua = (uintptr_t)ptr;
+
+	for (i = 0; i < mem->nregions; i++) {
+		r = &mem->regions[i];
+		if (hua >= r->host_user_addr &&
+			hua < r->host_user_addr + r->size) {
+			return r->alignment;
+		}
+	}
+
+	/* If region isn't found, don't align at all */
+	return 1;
+}
+
+void
+mem_set_dump(void *ptr, size_t size, bool enable, uint64_t pagesz)
+{
+#ifdef MADV_DONTDUMP
+	void *start = RTE_PTR_ALIGN_FLOOR(ptr, pagesz);
+	uintptr_t end = RTE_ALIGN_CEIL((uintptr_t)ptr + size, pagesz);
+	size_t len = end - (uintptr_t)start;
+
+	if (madvise(start, len, enable ? MADV_DODUMP : MADV_DONTDUMP) == -1) {
+		rte_log(RTE_LOG_INFO, vhost_config_log_level,
+			"VHOST_CONFIG: could not set coredump preference (%s).\n", strerror(errno));
+	}
+#endif
+}
+
  static void
  translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
  {

@@ -767,6 +801,8 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
  			return;
  		}
 
+		mem_set_dump(vq->desc_packed, len, true,
+			hua_to_alignment(dev->mem, vq->desc_packed));
  		numa_realloc(&dev, &vq);
  		*pdev = dev;
  		*pvq = vq;

@@ -782,6 +818,8 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
  			return;
  		}
 
+		mem_set_dump(vq->driver_event, len, true,
+			hua_to_alignment(dev->mem, vq->driver_event));
  		len = sizeof(struct vring_packed_desc_event);
  		vq->device_event = (struct vring_packed_desc_event *)
  					(uintptr_t)ring_addr_to_vva(dev,

@@ -793,9 +831,8 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
  			return;
  		}
 
-		mem_set_dump(vq->desc_packed, len, true);
-		mem_set_dump(vq->driver_event, len, true);
-		mem_set_dump(vq->device_event, len, true);
+		mem_set_dump(vq->device_event, len, true,
+			hua_to_alignment(dev->mem, vq->device_event));
  		vq->access_ok = true;
  		return;
  	}

@@ -812,6 +849,7 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
  		return;
  	}
 
+	mem_set_dump(vq->desc, len, true, hua_to_alignment(dev->mem, vq->desc));
  	numa_realloc(&dev, &vq);
  	*pdev = dev;
  	*pvq = vq;

@@ -827,6 +865,7 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
  		return;
  	}
 
+	mem_set_dump(vq->avail, len, true, hua_to_alignment(dev->mem, vq->avail));
  	len = sizeof(struct vring_used) +
  		sizeof(struct vring_used_elem) * vq->size;
  	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX))

@@ -839,6 +878,8 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
  		return;
  	}
 
+	mem_set_dump(vq->used, len, true, hua_to_alignment(dev->mem, vq->used));
+
  	if (vq->last_used_idx != vq->used->idx) {
  		VHOST_LOG_CONFIG(dev->ifname, WARNING,
  			"last_used_idx (%u) and vq->used->idx (%u) mismatches;\n",

@@ -849,9 +890,6 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
  			"some packets maybe resent for Tx and dropped for Rx\n");
  	}
 
-	mem_set_dump(vq->desc, len, true);
-	mem_set_dump(vq->avail, len, true);
-	mem_set_dump(vq->used, len, true);
  	vq->access_ok = true;
 
  	VHOST_LOG_CONFIG(dev->ifname, DEBUG, "mapped address desc: %p\n", vq->desc);

@@ -1230,7 +1268,8 @@ vhost_user_mmap_region(struct virtio_net *dev,
  	region->mmap_addr = mmap_addr;
  	region->mmap_size = mmap_size;
  	region->host_user_addr = (uint64_t)(uintptr_t)mmap_addr + mmap_offset;
-	mem_set_dump(mmap_addr, mmap_size, false);
+	region->alignment = alignment;
+	mem_set_dump(mmap_addr, mmap_size, false, alignment);
 
  	if (dev->async_copy) {
  		if (add_guest_pages(dev, region, alignment) < 0) {

@@ -1535,7 +1574,6 @@ inflight_mem_alloc(struct virtio_net *dev, const char *name, size_t size, int *f
  		return NULL;
  	}
 
-	mem_set_dump(ptr, size, false);
  	*fd = mfd;
  	return ptr;
  }

@@ -1566,6 +1604,7 @@ vhost_user_get_inflight_fd(struct virtio_net **pdev,
  	uint64_t pervq_inflight_size, mmap_size;
  	uint16_t num_queues, queue_size;
  	struct virtio_net *dev = *pdev;
+	uint64_t alignment;
  	int fd, i, j;
  	int numa_node = SOCKET_ID_ANY;
  	void *addr;

@@ -1628,6 +1667,8 @@ vhost_user_get_inflight_fd(struct virtio_net **pdev,
  		dev->inflight_info->fd = -1;
  	}
 
+	alignment = get_blk_size(fd);
+	mem_set_dump(addr, mmap_size, false, alignment);
  	dev->inflight_info->addr = addr;
  	dev->inflight_info->size = ctx->msg.payload.inflight.mmap_size = mmap_size;
  	dev->inflight_info->fd = ctx->fds[0] = fd;

@@ -1744,10 +1785,10 @@ vhost_user_set_inflight_fd(struct virtio_net **pdev,
  		dev->inflight_info->fd = -1;
  	}
 
-	mem_set_dump(addr, mmap_size, false);
  	dev->inflight_info->fd = fd;
  	dev->inflight_info->addr = addr;
  	dev->inflight_info->size = mmap_size;
+	mem_set_dump(addr, mmap_size, false, get_blk_size(fd));
 
  	for (i = 0; i < num_queues; i++) {
  		vq = dev->virtqueue[i];

@@ -2242,6 +2283,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
  	struct virtio_net *dev = *pdev;
  	int fd = ctx->fds[0];
  	uint64_t size, off;
+	uint64_t alignment;
  	void *addr;
  	uint32_t i;

@@ -2280,6 +2322,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
  	 * fail when offset is not page size aligned.
  	 */
  	addr = mmap(0, size + off, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+	alignment = get_blk_size(fd);
  	close(fd);
  	if (addr == MAP_FAILED) {
  		VHOST_LOG_CONFIG(dev->ifname, ERR, "mmap log base failed!\n");

@@ -2296,7 +2339,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
  	dev->log_addr = (uint64_t)(uintptr_t)addr;
  	dev->log_base = dev->log_addr + off;
  	dev->log_size = size;
-	mem_set_dump(addr, size, false);
+	mem_set_dump(addr, size + off, false, alignment);
 
  	for (i = 0; i < dev->nr_vring; i++) {
  		struct vhost_virtqueue *vq = dev->virtqueue[i];

Mike Pattrick

author

2023-02-23 16:57 UTC #12 lore raw

Re: [PATCH v2] vhost: fix madvise arguments alignment

Quoted 35 lines · Maxime Coquelin

Hi Mike,
Thanks for looking into this issue.
Quoted 29 lines · Mike Pattrick
The arguments passed to madvise should be aligned to the alignment of the backing memory. Now we keep track of each regions alignment and use then when setting coredump preferences. To facilitate this, a new member was added to rte_vhost_mem_region. A new function was added to easily translate memory address back to region alignment. Unneeded calls to madvise were reduced, as the cache removal case should already be covered by the cache insertion case. The previously inline function mem_set_dump was removed from a header file and made not inline.
Fixes 338ad77c9ed3 ("vhost: exclude VM hugepages from coredumps")
Signed-off-by Mike Pattrick <[email protected]>
---
Since v1:
- Corrected a cast for 32bit compiles
---
  lib/vhost/iotlb.c      |  9 +++---
  lib/vhost/rte_vhost.h  |  1 +
  lib/vhost/vhost.h      | 12 ++------
  lib/vhost/vhost_user.c | 63 +++++++++++++++++++++++++++++++++++-------
  4 files changed, 60 insertions(+), 25 deletions(-)
lib/vhost/iotlb.c +0 -0
index a0b8fd7302..5293507b63 100644
@@ -149,7 +149,6 @@ vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq)
      rte_rwlock_write_lock(&vq->iotlb_lock);
 

Quoted 6 lines

lib/vhost/iotlb.c vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq) · line 152
Quoted 2 lines
lib/vhost/iotlb.c +0 -1
      RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
-             mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
Hmm, it should have been called with enable=false here since we are removing the entry from the IOTLB cache. It should be kept in order to "DONTDUMP" pages evicted from the cache.

Here I was thinking that if we add an entry and then remove a different entry, they could be in the same page. But on I should have kept an enable=false in remove_all().

And now that I think about it again, I could just check if there are any active cache entries in the page on every evict/remove, they're sorted so that should be an easy check. Unless there are any objections I'll go forward with that.

Quoted 47 lines

lib/vhost/iotlb.c vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq) · line 174

Quoted 9 lines

lib/vhost/iotlb.c +0 -1

              TAILQ_REMOVE(&vq->iotlb_list, node, next);
              vhost_user_iotlb_pool_put(vq, node);
      }

@@ -171,7 +170,6 @@ vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq)
 
      RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
              if (!entry_idx) {
-                     mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);

Same here.

Quoted 35 lines

lib/vhost/iotlb.c +4 -3

                      TAILQ_REMOVE(&vq->iotlb_list, node, next);
                      vhost_user_iotlb_pool_put(vq, node);
                      vq->iotlb_cache_nr--;

@@ -224,14 +222,16 @@ vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq
                      vhost_user_iotlb_pool_put(vq, new_node);
                      goto unlock;
              } else if (node->iova > new_node->iova) {
-                     mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+                     mem_set_dump((void *)(uintptr_t)new_node->uaddr, new_node->size, true,
+                             hua_to_alignment(dev->mem, (void *)(uintptr_t)node->uaddr));
                      TAILQ_INSERT_BEFORE(node, new_node, next);
                      vq->iotlb_cache_nr++;
                      goto unlock;
              }
      }
 
-     mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+     mem_set_dump((void *)(uintptr_t)new_node->uaddr, new_node->size, true,
+             hua_to_alignment(dev->mem, (void *)(uintptr_t)new_node->uaddr));
      TAILQ_INSERT_TAIL(&vq->iotlb_list, new_node, next);
      vq->iotlb_cache_nr++;

@@ -259,7 +259,6 @@ vhost_user_iotlb_cache_remove(struct vhost_virtqueue *vq,
                      break;
 
              if (iova < node->iova + node->size) {
-                     mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
                      TAILQ_REMOVE(&vq->iotlb_list, node, next);
                      vhost_user_iotlb_pool_put(vq, node);
                      vq->iotlb_cache_nr--;

lib/vhost/rte_vhost.h +0 -0

index a395843fe9..c5c97ea67e 100644
@@ -136,6 +136,7 @@ struct rte_vhost_mem_region {

Quoted 9 lines

lib/vhost/rte_vhost.h struct rte_vhost_mem_region { · line 139
Quoted 4 lines
lib/vhost/rte_vhost.h +1 -0
      void     *mmap_addr;
      uint64_t mmap_size;
      int fd;
+     uint64_t alignment;
This is not possible to do this as it breaks the ABI. You have to store the information somewhere else, or simply call get_blk_size() in hua_to_alignment() since the fd is not closed.

Sorry about that! You're right, checking the fd per operation should be easy enough.

Thanks for the review,

Quoted 207 lines

Quoted 206 lines

lib/vhost/rte_vhost.h +0 -0

  };
 
  /**

lib/vhost/vhost.h +2 ~~-10~~

index 5750f0c005..a2467ba509 100644
@@ -1009,14 +1009,6 @@ mbuf_is_consumed(struct rte_mbuf *m)
      return true;
  }
 
-static __rte_always_inline void
-mem_set_dump(__rte_unused void *ptr, __rte_unused size_t size, __rte_unused bool enable)
-{
-#ifdef MADV_DONTDUMP
-     if (madvise(ptr, size, enable ? MADV_DODUMP : MADV_DONTDUMP) == -1) {
-             rte_log(RTE_LOG_INFO, vhost_config_log_level,
-                     "VHOST_CONFIG: could not set coredump preference (%s).\n", strerror(errno));
-     }
-#endif
-}
+uint64_t hua_to_alignment(struct rte_vhost_memory *mem, void *ptr);
+void mem_set_dump(void *ptr, size_t size, bool enable, uint64_t alignment);
  #endif /* _VHOST_NET_CDEV_H_ */

lib/vhost/vhost_user.c +53 ~~-10~~

index d702d082dd..6d09597fbe 100644
@@ -737,6 +737,40 @@ log_addr_to_gpa(struct virtio_net *dev, struct vhost_virtqueue *vq)
      return log_gpa;
  }
 
+uint64_t
+hua_to_alignment(struct rte_vhost_memory *mem, void *ptr)
+{
+     struct rte_vhost_mem_region *r;
+     uint32_t i;
+     uintptr_t hua = (uintptr_t)ptr;
+
+     for (i = 0; i < mem->nregions; i++) {
+             r = &mem->regions[i];
+             if (hua >= r->host_user_addr &&
+                     hua < r->host_user_addr + r->size) {
+                     return r->alignment;
+             }
+     }
+
+     /* If region isn't found, don't align at all */
+     return 1;
+}
+
+void
+mem_set_dump(void *ptr, size_t size, bool enable, uint64_t pagesz)
+{
+#ifdef MADV_DONTDUMP
+     void *start = RTE_PTR_ALIGN_FLOOR(ptr, pagesz);
+     uintptr_t end = RTE_ALIGN_CEIL((uintptr_t)ptr + size, pagesz);
+     size_t len = end - (uintptr_t)start;
+
+     if (madvise(start, len, enable ? MADV_DODUMP : MADV_DONTDUMP) == -1) {
+             rte_log(RTE_LOG_INFO, vhost_config_log_level,
+                     "VHOST_CONFIG: could not set coredump preference (%s).\n", strerror(errno));
+     }
+#endif
+}
+
  static void
  translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
  {

@@ -767,6 +801,8 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
                      return;
              }
 
+             mem_set_dump(vq->desc_packed, len, true,
+                     hua_to_alignment(dev->mem, vq->desc_packed));
              numa_realloc(&dev, &vq);
              *pdev = dev;
              *pvq = vq;

@@ -782,6 +818,8 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
                      return;
              }
 
+             mem_set_dump(vq->driver_event, len, true,
+                     hua_to_alignment(dev->mem, vq->driver_event));
              len = sizeof(struct vring_packed_desc_event);
              vq->device_event = (struct vring_packed_desc_event *)
                                      (uintptr_t)ring_addr_to_vva(dev,

@@ -793,9 +831,8 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
                      return;
              }
 
-             mem_set_dump(vq->desc_packed, len, true);
-             mem_set_dump(vq->driver_event, len, true);
-             mem_set_dump(vq->device_event, len, true);
+             mem_set_dump(vq->device_event, len, true,
+                     hua_to_alignment(dev->mem, vq->device_event));
              vq->access_ok = true;
              return;
      }

@@ -812,6 +849,7 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
              return;
      }
 
+     mem_set_dump(vq->desc, len, true, hua_to_alignment(dev->mem, vq->desc));
      numa_realloc(&dev, &vq);
      *pdev = dev;
      *pvq = vq;

@@ -827,6 +865,7 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
              return;
      }
 
+     mem_set_dump(vq->avail, len, true, hua_to_alignment(dev->mem, vq->avail));
      len = sizeof(struct vring_used) +
              sizeof(struct vring_used_elem) * vq->size;
      if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX))

@@ -839,6 +878,8 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
              return;
      }
 
+     mem_set_dump(vq->used, len, true, hua_to_alignment(dev->mem, vq->used));
+
      if (vq->last_used_idx != vq->used->idx) {
              VHOST_LOG_CONFIG(dev->ifname, WARNING,
                      "last_used_idx (%u) and vq->used->idx (%u) mismatches;\n",

@@ -849,9 +890,6 @@ translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
                      "some packets maybe resent for Tx and dropped for Rx\n");
      }
 
-     mem_set_dump(vq->desc, len, true);
-     mem_set_dump(vq->avail, len, true);
-     mem_set_dump(vq->used, len, true);
      vq->access_ok = true;
 
      VHOST_LOG_CONFIG(dev->ifname, DEBUG, "mapped address desc: %p\n", vq->desc);

@@ -1230,7 +1268,8 @@ vhost_user_mmap_region(struct virtio_net *dev,
      region->mmap_addr = mmap_addr;
      region->mmap_size = mmap_size;
      region->host_user_addr = (uint64_t)(uintptr_t)mmap_addr + mmap_offset;
-     mem_set_dump(mmap_addr, mmap_size, false);
+     region->alignment = alignment;
+     mem_set_dump(mmap_addr, mmap_size, false, alignment);
 
      if (dev->async_copy) {
              if (add_guest_pages(dev, region, alignment) < 0) {

@@ -1535,7 +1574,6 @@ inflight_mem_alloc(struct virtio_net *dev, const char *name, size_t size, int *f
              return NULL;
      }
 
-     mem_set_dump(ptr, size, false);
      *fd = mfd;
      return ptr;
  }

@@ -1566,6 +1604,7 @@ vhost_user_get_inflight_fd(struct virtio_net **pdev,
      uint64_t pervq_inflight_size, mmap_size;
      uint16_t num_queues, queue_size;
      struct virtio_net *dev = *pdev;
+     uint64_t alignment;
      int fd, i, j;
      int numa_node = SOCKET_ID_ANY;
      void *addr;

@@ -1628,6 +1667,8 @@ vhost_user_get_inflight_fd(struct virtio_net **pdev,
              dev->inflight_info->fd = -1;
      }
 
+     alignment = get_blk_size(fd);
+     mem_set_dump(addr, mmap_size, false, alignment);
      dev->inflight_info->addr = addr;
      dev->inflight_info->size = ctx->msg.payload.inflight.mmap_size = mmap_size;
      dev->inflight_info->fd = ctx->fds[0] = fd;

@@ -1744,10 +1785,10 @@ vhost_user_set_inflight_fd(struct virtio_net **pdev,
              dev->inflight_info->fd = -1;
      }
 
-     mem_set_dump(addr, mmap_size, false);
      dev->inflight_info->fd = fd;
      dev->inflight_info->addr = addr;
      dev->inflight_info->size = mmap_size;
+     mem_set_dump(addr, mmap_size, false, get_blk_size(fd));
 
      for (i = 0; i < num_queues; i++) {
              vq = dev->virtqueue[i];

@@ -2242,6 +2283,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
      struct virtio_net *dev = *pdev;
      int fd = ctx->fds[0];
      uint64_t size, off;
+     uint64_t alignment;
      void *addr;
      uint32_t i;

@@ -2280,6 +2322,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
       * fail when offset is not page size aligned.
       */
      addr = mmap(0, size + off, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+     alignment = get_blk_size(fd);
      close(fd);
      if (addr == MAP_FAILED) {
              VHOST_LOG_CONFIG(dev->ifname, ERR, "mmap log base failed!\n");

@@ -2296,7 +2339,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
      dev->log_addr = (uint64_t)(uintptr_t)addr;
      dev->log_base = dev->log_addr + off;
      dev->log_size = size;
-     mem_set_dump(addr, size, false);
+     mem_set_dump(addr, size + off, false, alignment);
 
      for (i = 0; i < dev->nr_vring; i++) {
              struct vhost_virtqueue *vq = dev->virtqueue[i];

Patrick Robb

2023-02-24 15:06 UTC #13 lore raw

Re: [PATCH v2] vhost: fix madvise arguments alignment

UNH CI reported an ABI failure for this patch which did not report due to a bug on our end, so I'm manually reporting it now. I see Maxime you already predicted the issue though!

*07:58:32* 1 function with some indirect sub-type change:*07:58:32*
*07:58:32* [C] 'function int rte_vhost_get_mem_table(int,
rte_vhost_memory**)' at vhost.c:922:1 has some indirect sub-type
changes:*07:58:32* parameter 2 of type 'rte_vhost_memory**' has
sub-type changes:*07:58:32* in pointed to type
'rte_vhost_memory*':*07:58:32* in pointed to type 'struct
rte_vhost_memory' at rte_vhost.h:145:1:*07:58:32* type size
hasn't changed*07:58:32* 1 data member change:*07:58:32*
type of 'rte_vhost_mem_region regions[]' changed:*07:58:32*
array element type 'struct rte_vhost_mem_region'
changed:*07:58:32* type size changed from 448 to 512
(in bits)*07:58:32* 1 data member
insertion:*07:58:32* 'uint64_t alignment', at
offset 448 (in bits) at rte_vhost.h:139:1*07:58:32*
type size hasn't changed*07:58:32* *07:58:32* Error: ABI issue reported for abidiff --suppr dpdk/devtools/libabigail.abignore --no-added-syms --headers-dir1 reference/include --headers-dir2 build_install/include reference/dump/librte_vhost.dump build_install/dump/librte_vhost.dump*07:58:32* ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue).

Quoted 369 lines · Mike Pattrick

Quoted 9 lines · Maxime Coquelin
Hi Mike,
Thanks for looking into this issue.
Quoted 3 lines · Mike Pattrick
The arguments passed to madvise should be aligned to the alignment of the backing memory. Now we keep track of each regions alignment and use then when setting coredump preferences. To facilitate this, a new
member
Quoted 8 lines
Quoted 8 lines
was added to rte_vhost_mem_region. A new function was added to easily translate memory address back to region alignment. Unneeded calls to madvise were reduced, as the cache removal case should already be covered by the cache insertion case. The previously inline function mem_set_dump was removed from a header file and made not inline.
Fixes 338ad77c9ed3 ("vhost: exclude VM hugepages from coredumps")
Quoted 9 lines
Quoted 9 lines
Signed-off-by Mike Pattrick <[email protected]>
---
Since v1:
- Corrected a cast for 32bit compiles
---
  lib/vhost/iotlb.c      |  9 +++---
  lib/vhost/rte_vhost.h  |  1 +
  lib/vhost/vhost.h      | 12 ++------
  lib/vhost/vhost_user.c | 63
+++++++++++++++++++++++++++++++++++-------
Quoted 7 lines
lib/vhost/iotlb.c vhost_user_iotlb_cache_remove_all(struct
Quoted 7 lines
  4 files changed, 60 insertions(+), 25 deletions(-)
lib/vhost/iotlb.c +0 -0
index a0b8fd7302..5293507b63 100644
@@ -149,7 +149,6 @@ vhost_user_iotlb_cache_remove_all(struct
vhost_virtqueue *vq)
Quoted 4 lines
lib/vhost/iotlb.c vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq)
Quoted 4 lines
lib/vhost/iotlb.c +0 -1
      rte_rwlock_write_lock(&vq->iotlb_lock);
 
      RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
-             mem_set_dump((void *)(uintptr_t)node->uaddr, node->size,
true);
Quoted 4 lines
Hmm, it should have been called with enable=false here since we are removing the entry from the IOTLB cache. It should be kept in order to "DONTDUMP" pages evicted from the cache.
Here I was thinking that if we add an entry and then remove a different entry, they could be in the same page. But on I should have kept an enable=false in remove_all().
And now that I think about it again, I could just check if there are any active cache entries in the page on every evict/remove, they're sorted so that should be an easy check. Unless there are any objections I'll go forward with that.
Quoted 5 lines
lib/vhost/iotlb.c vhost_user_iotlb_cache_random_evict(struct
Quoted 5 lines
lib/vhost/iotlb.c +0 -0
              TAILQ_REMOVE(&vq->iotlb_list, node, next);
              vhost_user_iotlb_pool_put(vq, node);
      }
@@ -171,7 +170,6 @@ vhost_user_iotlb_cache_random_evict(struct
vhost_virtqueue *vq)
Quoted 4 lines
lib/vhost/iotlb.c vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq)
Quoted 4 lines
lib/vhost/iotlb.c +0 -1
      RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
              if (!entry_idx) {
-                     mem_set_dump((void *)(uintptr_t)node->uaddr,
node->size, true);
Quoted 7 lines
Same here.
lib/vhost/iotlb.c vhost_user_iotlb_cache_insert(struct virtio_net
Quoted 4 lines
lib/vhost/iotlb.c +0 -0
                      TAILQ_REMOVE(&vq->iotlb_list, node, next);
                      vhost_user_iotlb_pool_put(vq, node);
                      vq->iotlb_cache_nr--;
@@ -224,14 +222,16 @@ vhost_user_iotlb_cache_insert(struct virtio_net
*dev, struct vhost_virtqueue *vq
Quoted 4 lines
lib/vhost/iotlb.c vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq
Quoted 4 lines
lib/vhost/iotlb.c +0 -1
                      vhost_user_iotlb_pool_put(vq, new_node);
                      goto unlock;
              } else if (node->iova > new_node->iova) {
-                     mem_set_dump((void *)(uintptr_t)node->uaddr,
node->size, true);
Quoted 1 line
lib/vhost/iotlb.c vhost_user_iotlb_cache_insert(struct virtio_net · line 225
Quoted 1 line
lib/vhost/iotlb.c +1 -0
+                     mem_set_dump((void *)(uintptr_t)new_node->uaddr,
new_node->size, true,
Quoted 1 line
lib/vhost/iotlb.c vhost_user_iotlb_cache_insert(struct virtio_net · line 226
Quoted 1 line
lib/vhost/iotlb.c +1 -0
+                             hua_to_alignment(dev->mem, (void
*)(uintptr_t)node->uaddr));
Quoted 8 lines
lib/vhost/iotlb.c vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq
Quoted 8 lines
lib/vhost/iotlb.c +1 -1
                      TAILQ_INSERT_BEFORE(node, new_node, next);
                      vq->iotlb_cache_nr++;
                      goto unlock;
              }
      }
 
-     mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+     mem_set_dump((void *)(uintptr_t)new_node->uaddr, new_node->size,
true,
Quoted 1 line
lib/vhost/iotlb.c vhost_user_iotlb_cache_insert(struct virtio_net · line 234
Quoted 1 line
lib/vhost/iotlb.c +1 -0
+             hua_to_alignment(dev->mem, (void
*)(uintptr_t)new_node->uaddr));
Quoted 4 lines
lib/vhost/iotlb.c vhost_user_iotlb_cache_remove(struct
Quoted 4 lines
lib/vhost/iotlb.c +0 -0
      TAILQ_INSERT_TAIL(&vq->iotlb_list, new_node, next);
      vq->iotlb_cache_nr++;
 
@@ -259,7 +259,6 @@ vhost_user_iotlb_cache_remove(struct
vhost_virtqueue *vq,
Quoted 4 lines
lib/vhost/iotlb.c vhost_user_iotlb_cache_remove(struct vhost_virtqueue *vq,
Quoted 4 lines
lib/vhost/iotlb.c +0 -1
                      break;
 
              if (iova < node->iova + node->size) {
-                     mem_set_dump((void *)(uintptr_t)node->uaddr,
node->size, true);
Quoted 8 lines
Quoted 8 lines
lib/vhost/iotlb.c +0 -0
                      TAILQ_REMOVE(&vq->iotlb_list, node, next);
                      vhost_user_iotlb_pool_put(vq, node);
                      vq->iotlb_cache_nr--;
lib/vhost/rte_vhost.h +0 -0
index a395843fe9..c5c97ea67e 100644
@@ -136,6 +136,7 @@ struct rte_vhost_mem_region {
Quoted 9 lines
lib/vhost/rte_vhost.h struct rte_vhost_mem_region { · line 139
Quoted 4 lines
lib/vhost/rte_vhost.h +1 -0
      void     *mmap_addr;
      uint64_t mmap_size;
      int fd;
+     uint64_t alignment;
This is not possible to do this as it breaks the ABI. You have to store the information somewhere else, or simply call get_blk_size() in hua_to_alignment() since the fd is not closed.
Sorry about that! You're right, checking the fd per operation should be easy enough.
Thanks for the review,
M
Quoted 11 lines
Quoted 11 lines
lib/vhost/rte_vhost.h +0 -0
  };
 
  /**
lib/vhost/vhost.h +0 -0
index 5750f0c005..a2467ba509 100644
@@ -1009,14 +1009,6 @@ mbuf_is_consumed(struct rte_mbuf *m)
      return true;
  }
 
Quoted 2 lines
lib/vhost/vhost.h mbuf_is_consumed(struct rte_mbuf *m) · line 1013
Quoted 2 lines
lib/vhost/vhost.h +0 -2
-static __rte_always_inline void
-mem_set_dump(__rte_unused void *ptr, __rte_unused size_t size,
__rte_unused bool enable)
Quoted 3 lines
lib/vhost/vhost.h mbuf_is_consumed(struct rte_mbuf *m)
Quoted 3 lines
lib/vhost/vhost.h +0 -3
-{
-#ifdef MADV_DONTDUMP
-     if (madvise(ptr, size, enable ? MADV_DODUMP : MADV_DONTDUMP) ==
-1) {
Quoted 2 lines
lib/vhost/vhost.h mbuf_is_consumed(struct rte_mbuf *m) · line 1018
Quoted 2 lines
lib/vhost/vhost.h +0 -2
-             rte_log(RTE_LOG_INFO, vhost_config_log_level,
-                     "VHOST_CONFIG: could not set coredump preference
(%s).\n", strerror(errno));
Quoted 5 lines
lib/vhost/vhost.h mbuf_is_consumed(struct rte_mbuf *m)
Quoted 5 lines
lib/vhost/vhost.h +2 -3
-     }
-#endif
-}
+uint64_t hua_to_alignment(struct rte_vhost_memory *mem, void *ptr);
+void mem_set_dump(void *ptr, size_t size, bool enable, uint64_t
alignment);
Quoted 6 lines
lib/vhost/vhost_user.c log_addr_to_gpa(struct virtio_net *dev, struct
Quoted 6 lines
lib/vhost/vhost.h +0 -0
  #endif /* _VHOST_NET_CDEV_H_ */
lib/vhost/vhost_user.c +0 -0
index d702d082dd..6d09597fbe 100644
@@ -737,6 +737,40 @@ log_addr_to_gpa(struct virtio_net *dev, struct
vhost_virtqueue *vq)
Quoted 23 lines
Quoted 23 lines
lib/vhost/vhost_user.c +20 -0
      return log_gpa;
  }
 
+uint64_t
+hua_to_alignment(struct rte_vhost_memory *mem, void *ptr)
+{
+     struct rte_vhost_mem_region *r;
+     uint32_t i;
+     uintptr_t hua = (uintptr_t)ptr;
+
+     for (i = 0; i < mem->nregions; i++) {
+             r = &mem->regions[i];
+             if (hua >= r->host_user_addr &&
+                     hua < r->host_user_addr + r->size) {
+                     return r->alignment;
+             }
+     }
+
+     /* If region isn't found, don't align at all */
+     return 1;
+}
+
+void
Quoted 8 lines
lib/vhost/vhost_user.c log_addr_to_gpa(struct virtio_net *dev, struct vhost_virtqueue *vq)
Quoted 8 lines
lib/vhost/vhost_user.c +8 -0
+mem_set_dump(void *ptr, size_t size, bool enable, uint64_t pagesz)
+{
+#ifdef MADV_DONTDUMP
+     void *start = RTE_PTR_ALIGN_FLOOR(ptr, pagesz);
+     uintptr_t end = RTE_ALIGN_CEIL((uintptr_t)ptr + size, pagesz);
+     size_t len = end - (uintptr_t)start;
+
+     if (madvise(start, len, enable ? MADV_DODUMP : MADV_DONTDUMP) ==
-1) {
Quoted 2 lines
lib/vhost/vhost_user.c log_addr_to_gpa(struct virtio_net *dev, struct · line 769
Quoted 2 lines
lib/vhost/vhost_user.c +2 -0
+             rte_log(RTE_LOG_INFO, vhost_config_log_level,
+                     "VHOST_CONFIG: could not set coredump preference
(%s).\n", strerror(errno));
Quoted 6 lines
lib/vhost/vhost_user.c log_addr_to_gpa(struct virtio_net *dev, struct vhost_virtqueue *vq)
Quoted 6 lines
lib/vhost/vhost_user.c +4 -0
+     }
+#endif
+}
+
  static void
  translate_ring_addresses(struct virtio_net **pdev, struct
vhost_virtqueue **pvq)
Quoted 2 lines
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev,
Quoted 2 lines
lib/vhost/vhost_user.c +0 -0
  {
@@ -767,6 +801,8 @@ translate_ring_addresses(struct virtio_net **pdev,
struct vhost_virtqueue **pvq)
Quoted 9 lines
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev,
Quoted 9 lines
lib/vhost/vhost_user.c +2 -0
                      return;
              }
 
+             mem_set_dump(vq->desc_packed, len, true,
+                     hua_to_alignment(dev->mem, vq->desc_packed));
              numa_realloc(&dev, &vq);
              *pdev = dev;
              *pvq = vq;
@@ -782,6 +818,8 @@ translate_ring_addresses(struct virtio_net **pdev,
struct vhost_virtqueue **pvq)
Quoted 9 lines
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev,
Quoted 9 lines
lib/vhost/vhost_user.c +2 -0
                      return;
              }
 
+             mem_set_dump(vq->driver_event, len, true,
+                     hua_to_alignment(dev->mem, vq->driver_event));
              len = sizeof(struct vring_packed_desc_event);
              vq->device_event = (struct vring_packed_desc_event *)
                                      (uintptr_t)ring_addr_to_vva(dev,
@@ -793,9 +831,8 @@ translate_ring_addresses(struct virtio_net **pdev,
struct vhost_virtqueue **pvq)
Quoted 12 lines
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev,
Quoted 12 lines
lib/vhost/vhost_user.c +2 -3
                      return;
              }
 
-             mem_set_dump(vq->desc_packed, len, true);
-             mem_set_dump(vq->driver_event, len, true);
-             mem_set_dump(vq->device_event, len, true);
+             mem_set_dump(vq->device_event, len, true,
+                     hua_to_alignment(dev->mem, vq->device_event));
              vq->access_ok = true;
              return;
      }
@@ -812,6 +849,7 @@ translate_ring_addresses(struct virtio_net **pdev,
struct vhost_virtqueue **pvq)
Quoted 4 lines
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev, · line 852
Quoted 4 lines
lib/vhost/vhost_user.c +1 -0
              return;
      }
 
+     mem_set_dump(vq->desc, len, true, hua_to_alignment(dev->mem,
vq->desc));
Quoted 4 lines
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev,
Quoted 4 lines
lib/vhost/vhost_user.c +0 -0
      numa_realloc(&dev, &vq);
      *pdev = dev;
      *pvq = vq;
@@ -827,6 +865,7 @@ translate_ring_addresses(struct virtio_net **pdev,
struct vhost_virtqueue **pvq)
Quoted 4 lines
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev, · line 868
Quoted 4 lines
lib/vhost/vhost_user.c +1 -0
              return;
      }
 
+     mem_set_dump(vq->avail, len, true, hua_to_alignment(dev->mem,
vq->avail));
Quoted 4 lines
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev,
Quoted 4 lines
lib/vhost/vhost_user.c +0 -0
      len = sizeof(struct vring_used) +
              sizeof(struct vring_used_elem) * vq->size;
      if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX))
@@ -839,6 +878,8 @@ translate_ring_addresses(struct virtio_net **pdev,
struct vhost_virtqueue **pvq)
Quoted 4 lines
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev, · line 881
Quoted 4 lines
lib/vhost/vhost_user.c +1 -0
              return;
      }
 
+     mem_set_dump(vq->used, len, true, hua_to_alignment(dev->mem,
vq->used));
Quoted 4 lines
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
Quoted 4 lines
lib/vhost/vhost_user.c +1 -0
+
      if (vq->last_used_idx != vq->used->idx) {
              VHOST_LOG_CONFIG(dev->ifname, WARNING,
                      "last_used_idx (%u) and vq->used->idx (%u)
mismatches;\n",
Quoted 1 line
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev,
Quoted 1 line
lib/vhost/vhost_user.c +0 -0
@@ -849,9 +890,6 @@ translate_ring_addresses(struct virtio_net **pdev,
struct vhost_virtqueue **pvq)
Quoted 1 line
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev, · line 890
Quoted 1 line
lib/vhost/vhost_user.c +0 -0
                      "some packets maybe resent for Tx and dropped
for Rx\n");
Quoted 8 lines
lib/vhost/vhost_user.c translate_ring_addresses(struct virtio_net **pdev, struct vhost_virtqueue **pvq)
Quoted 8 lines
lib/vhost/vhost_user.c +0 -3
      }
 
-     mem_set_dump(vq->desc, len, true);
-     mem_set_dump(vq->avail, len, true);
-     mem_set_dump(vq->used, len, true);
      vq->access_ok = true;
 
      VHOST_LOG_CONFIG(dev->ifname, DEBUG, "mapped address desc:
%p\n", vq->desc);
Quoted 4 lines
lib/vhost/vhost_user.c vhost_user_mmap_region(struct virtio_net *dev, · line 1270
Quoted 4 lines
lib/vhost/vhost_user.c +0 -0
@@ -1230,7 +1268,8 @@ vhost_user_mmap_region(struct virtio_net *dev,
      region->mmap_addr = mmap_addr;
      region->mmap_size = mmap_size;
      region->host_user_addr = (uint64_t)(uintptr_t)mmap_addr +
mmap_offset;
Quoted 7 lines
lib/vhost/vhost_user.c inflight_mem_alloc(struct virtio_net *dev, const
Quoted 7 lines
lib/vhost/vhost_user.c +2 -1
-     mem_set_dump(mmap_addr, mmap_size, false);
+     region->alignment = alignment;
+     mem_set_dump(mmap_addr, mmap_size, false, alignment);
 
      if (dev->async_copy) {
              if (add_guest_pages(dev, region, alignment) < 0) {
@@ -1535,7 +1574,6 @@ inflight_mem_alloc(struct virtio_net *dev, const
char *name, size_t size, int *f
Quoted 8 lines
lib/vhost/vhost_user.c vhost_user_get_inflight_fd(struct virtio_net
Quoted 8 lines
lib/vhost/vhost_user.c +0 -1
              return NULL;
      }
 
-     mem_set_dump(ptr, size, false);
      *fd = mfd;
      return ptr;
  }
@@ -1566,6 +1604,7 @@ vhost_user_get_inflight_fd(struct virtio_net
**pdev,
Quoted 8 lines
lib/vhost/vhost_user.c vhost_user_get_inflight_fd(struct virtio_net
Quoted 8 lines
lib/vhost/vhost_user.c +1 -0
      uint64_t pervq_inflight_size, mmap_size;
      uint16_t num_queues, queue_size;
      struct virtio_net *dev = *pdev;
+     uint64_t alignment;
      int fd, i, j;
      int numa_node = SOCKET_ID_ANY;
      void *addr;
@@ -1628,6 +1667,8 @@ vhost_user_get_inflight_fd(struct virtio_net
**pdev,
Quoted 7 lines
lib/vhost/vhost_user.c vhost_user_get_inflight_fd(struct virtio_net **pdev,
Quoted 7 lines
lib/vhost/vhost_user.c +2 -0
              dev->inflight_info->fd = -1;
      }
 
+     alignment = get_blk_size(fd);
+     mem_set_dump(addr, mmap_size, false, alignment);
      dev->inflight_info->addr = addr;
      dev->inflight_info->size = ctx->msg.payload.inflight.mmap_size =
mmap_size;
Quoted 2 lines
lib/vhost/vhost_user.c vhost_user_set_inflight_fd(struct virtio_net
Quoted 2 lines
lib/vhost/vhost_user.c +0 -0
      dev->inflight_info->fd = ctx->fds[0] = fd;
@@ -1744,10 +1785,10 @@ vhost_user_set_inflight_fd(struct virtio_net
**pdev,
Quoted 19 lines
Quoted 19 lines
lib/vhost/vhost_user.c +2 -1
              dev->inflight_info->fd = -1;
      }
 
-     mem_set_dump(addr, mmap_size, false);
      dev->inflight_info->fd = fd;
      dev->inflight_info->addr = addr;
      dev->inflight_info->size = mmap_size;
+     mem_set_dump(addr, mmap_size, false, get_blk_size(fd));
 
      for (i = 0; i < num_queues; i++) {
              vq = dev->virtqueue[i];
@@ -2242,6 +2283,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
      struct virtio_net *dev = *pdev;
      int fd = ctx->fds[0];
      uint64_t size, off;
+     uint64_t alignment;
      void *addr;
      uint32_t i;
 
Quoted 4 lines
lib/vhost/vhost_user.c vhost_user_set_log_base(struct virtio_net **pdev, · line 2324
Quoted 4 lines
lib/vhost/vhost_user.c +0 -0
@@ -2280,6 +2322,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
       * fail when offset is not page size aligned.
       */
      addr = mmap(0, size + off, PROT_READ | PROT_WRITE, MAP_SHARED,
fd, 0);
Quoted 4 lines
lib/vhost/vhost_user.c vhost_user_set_log_base(struct virtio_net **pdev,
Quoted 4 lines
lib/vhost/vhost_user.c +1 -0
+     alignment = get_blk_size(fd);
      close(fd);
      if (addr == MAP_FAILED) {
              VHOST_LOG_CONFIG(dev->ifname, ERR, "mmap log base
failed!\n");
Quoted 10 lines
Quoted 9 lines
lib/vhost/vhost_user.c +1 -1
@@ -2296,7 +2339,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
      dev->log_addr = (uint64_t)(uintptr_t)addr;
      dev->log_base = dev->log_addr + off;
      dev->log_size = size;
-     mem_set_dump(addr, size, false);
+     mem_set_dump(addr, size + off, false, alignment);
 
      for (i = 0; i < dev->nr_vring; i++) {
              struct vhost_virtqueue *vq = dev->virtqueue[i];

Signature · 9 lines

Patrick Robb

Technical Service Manager

UNH InterOperability Laboratory

21 Madbury Rd, Suite 100, Durham, NH 03824

www.iol.unh.edu

Mike Pattrick

author

2023-03-01 20:02 UTC #14 lore raw

PATCH v3 1/1 vhost: fix madvise arguments alignment

In reply to Mike Pattrick

Fixes 338ad77c9ed3 ("vhost: exclude VM hugepages from coredumps")

Signed-off-by Mike Pattrick <[email protected]>

---
Since v1:
- Corrected a cast for 32bit compiles
Since v2:
- Removed changes from rte_vhost.h
- Restored set_mem_dump in ioctl remove/evict functions

---
 lib/vhost/iotlb.c      | 68 +++++++++++++++++++++++++++++++-----------
 lib/vhost/iotlb.h      |  5 ++--
 lib/vhost/vhost.h      | 12 ++------
 lib/vhost/vhost_user.c | 68 ++++++++++++++++++++++++++++++++++--------
 4 files changed, 110 insertions(+), 43 deletions(-)

lib/vhost/iotlb.c +51 ~~-17~~

index a0b8fd7302..11785392ac 100644
@@ -46,7 +46,7 @@ vhost_user_iotlb_pool_put(struct vhost_virtqueue *vq,
 }
 
 static void
-vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq);
+vhost_user_iotlb_cache_random_evict(struct virtio_net *dev, struct vhost_virtqueue *vq);
 
 static void
 vhost_user_iotlb_pending_remove_all(struct vhost_virtqueue *vq)

@@ -98,7 +98,7 @@ vhost_user_iotlb_pending_insert(struct virtio_net *dev, struct vhost_virtqueue *
 		if (!TAILQ_EMPTY(&vq->iotlb_pending_list))
 			vhost_user_iotlb_pending_remove_all(vq);
 		else
-			vhost_user_iotlb_cache_random_evict(vq);
+			vhost_user_iotlb_cache_random_evict(dev, vq);
 		node = vhost_user_iotlb_pool_get(vq);
 		if (node == NULL) {
 			VHOST_LOG_CONFIG(dev->ifname, ERR,

@@ -142,14 +142,15 @@ vhost_user_iotlb_pending_remove(struct vhost_virtqueue *vq,
 }
 
 static void
-vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq)
+vhost_user_iotlb_cache_remove_all(struct virtio_net *dev, struct vhost_virtqueue *vq)
 {
 	struct vhost_iotlb_entry *node, *temp_node;
 
 	rte_rwlock_write_lock(&vq->iotlb_lock);
 
 	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
-		mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+		mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, false,
+			hua_to_alignment(dev->mem, (void *)(uintptr_t)node->uaddr));
 		TAILQ_REMOVE(&vq->iotlb_list, node, next);
 		vhost_user_iotlb_pool_put(vq, node);
 	}

@@ -160,9 +161,10 @@ vhost_user_iotlb_cache_remove_all(struct vhost_virtqueue *vq)
 }
 
 static void
-vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq)
+vhost_user_iotlb_cache_random_evict(struct virtio_net *dev, struct vhost_virtqueue *vq)
 {
-	struct vhost_iotlb_entry *node, *temp_node;
+	struct vhost_iotlb_entry *node, *temp_node, *prev_node = NULL;
+	uint64_t alignment, mask;
 	int entry_idx;
 
 	rte_rwlock_write_lock(&vq->iotlb_lock);

@@ -171,12 +173,26 @@ vhost_user_iotlb_cache_random_evict(struct vhost_virtqueue *vq)
 
 	RTE_TAILQ_FOREACH_SAFE(node, &vq->iotlb_list, next, temp_node) {
 		if (!entry_idx) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+			struct vhost_iotlb_entry *next_node;
+			alignment = hua_to_alignment(dev->mem, (void *)(uintptr_t)node->uaddr);
+			mask = ~(alignment - 1);
+
+			/* Don't disable coredump if the previous node is in the same page */
+			if (prev_node == NULL ||
+					(node->uaddr & mask) != (prev_node->uaddr & mask)) {
+				next_node = RTE_TAILQ_NEXT(node, next);
+				/* Don't disable coredump if the next node is in the same page */
+				if (next_node == NULL ||
+						(node->uaddr & mask) != (next_node->uaddr & mask))
+					mem_set_dump((void *)(uintptr_t)node->uaddr, node->size,
+							false, alignment);
+			}
 			TAILQ_REMOVE(&vq->iotlb_list, node, next);
 			vhost_user_iotlb_pool_put(vq, node);
 			vq->iotlb_cache_nr--;
 			break;
 		}
+		prev_node = node;
 		entry_idx--;
 	}

@@ -196,7 +212,7 @@ vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq
 			"IOTLB pool vq %"PRIu32" empty, clear entries for cache insertion\n",
 			vq->index);
 		if (!TAILQ_EMPTY(&vq->iotlb_list))
-			vhost_user_iotlb_cache_random_evict(vq);
+			vhost_user_iotlb_cache_random_evict(dev, vq);
 		else
 			vhost_user_iotlb_pending_remove_all(vq);
 		new_node = vhost_user_iotlb_pool_get(vq);

@@ -224,14 +240,16 @@ vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq
 			vhost_user_iotlb_pool_put(vq, new_node);
 			goto unlock;
 		} else if (node->iova > new_node->iova) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+			mem_set_dump((void *)(uintptr_t)new_node->uaddr, new_node->size, true,
+				hua_to_alignment(dev->mem, (void *)(uintptr_t)new_node->uaddr));
 			TAILQ_INSERT_BEFORE(node, new_node, next);
 			vq->iotlb_cache_nr++;
 			goto unlock;
 		}
 	}
 
-	mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+	mem_set_dump((void *)(uintptr_t)new_node->uaddr, new_node->size, true,
+		hua_to_alignment(dev->mem, (void *)(uintptr_t)new_node->uaddr));
 	TAILQ_INSERT_TAIL(&vq->iotlb_list, new_node, next);
 	vq->iotlb_cache_nr++;

@@ -243,10 +261,11 @@ vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq
 }
 
 void
-vhost_user_iotlb_cache_remove(struct vhost_virtqueue *vq,
+vhost_user_iotlb_cache_remove(struct virtio_net *dev, struct vhost_virtqueue *vq,
 					uint64_t iova, uint64_t size)
 {
-	struct vhost_iotlb_entry *node, *temp_node;
+	struct vhost_iotlb_entry *node, *temp_node, *prev_node = NULL;
+	uint64_t alignment, mask;
 
 	if (unlikely(!size))
 		return;

@@ -259,11 +278,26 @@ vhost_user_iotlb_cache_remove(struct vhost_virtqueue *vq,
 			break;
 
 		if (iova < node->iova + node->size) {
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, true);
+			struct vhost_iotlb_entry *next_node;
+			alignment = hua_to_alignment(dev->mem, (void *)(uintptr_t)node->uaddr);
+			mask = ~(alignment-1);
+
+			/* Don't disable coredump if the previous node is in the same page */
+			if (prev_node == NULL ||
+					(node->uaddr & mask) != (prev_node->uaddr & mask)) {
+				next_node = RTE_TAILQ_NEXT(node, next);
+				/* Don't disable coredump if the next node is in the same page */
+				if (next_node == NULL ||
+						(node->uaddr & mask) != (next_node->uaddr & mask))
+					mem_set_dump((void *)(uintptr_t)node->uaddr, node->size,
+							false, alignment);
+			}
+
 			TAILQ_REMOVE(&vq->iotlb_list, node, next);
 			vhost_user_iotlb_pool_put(vq, node);
 			vq->iotlb_cache_nr--;
-		}
+		} else
+			prev_node = node;
 	}
 
 	rte_rwlock_write_unlock(&vq->iotlb_lock);

@@ -312,9 +346,9 @@ vhost_user_iotlb_cache_find(struct vhost_virtqueue *vq, uint64_t iova,
 }
 
 void
-vhost_user_iotlb_flush_all(struct vhost_virtqueue *vq)
+vhost_user_iotlb_flush_all(struct virtio_net *dev, struct vhost_virtqueue *vq)
 {
-	vhost_user_iotlb_cache_remove_all(vq);
+	vhost_user_iotlb_cache_remove_all(dev, vq);
 	vhost_user_iotlb_pending_remove_all(vq);
 }

@@ -329,7 +363,7 @@ vhost_user_iotlb_init(struct virtio_net *dev, struct vhost_virtqueue *vq)
 		 * The cache has already been initialized,
 		 * just drop all cached and pending entries.
 		 */
-		vhost_user_iotlb_flush_all(vq);
+		vhost_user_iotlb_flush_all(dev, vq);
 		rte_free(vq->iotlb_pool);
 	}

lib/vhost/iotlb.h +2 -3

index be079a8aa7..73b5465b41 100644
@@ -40,7 +40,7 @@ vhost_user_iotlb_wr_unlock(struct vhost_virtqueue *vq)
 void vhost_user_iotlb_cache_insert(struct virtio_net *dev, struct vhost_virtqueue *vq,
 					uint64_t iova, uint64_t uaddr,
 					uint64_t size, uint8_t perm);
-void vhost_user_iotlb_cache_remove(struct vhost_virtqueue *vq,
+void vhost_user_iotlb_cache_remove(struct virtio_net *dev, struct vhost_virtqueue *vq,
 					uint64_t iova, uint64_t size);
 uint64_t vhost_user_iotlb_cache_find(struct vhost_virtqueue *vq, uint64_t iova,
 					uint64_t *size, uint8_t perm);

@@ -50,8 +50,7 @@ void vhost_user_iotlb_pending_insert(struct virtio_net *dev, struct vhost_virtqu
 						uint64_t iova, uint8_t perm);
 void vhost_user_iotlb_pending_remove(struct vhost_virtqueue *vq, uint64_t iova,
 						uint64_t size, uint8_t perm);
-void vhost_user_iotlb_flush_all(struct vhost_virtqueue *vq);
+void vhost_user_iotlb_flush_all(struct virtio_net *dev, struct vhost_virtqueue *vq);
 int vhost_user_iotlb_init(struct virtio_net *dev, struct vhost_virtqueue *vq);
 void vhost_user_iotlb_destroy(struct vhost_virtqueue *vq);
-
 #endif /* _VHOST_IOTLB_H_ */

lib/vhost/vhost.h +2 ~~-10~~

index 5750f0c005..a2467ba509 100644
@@ -1009,14 +1009,6 @@ mbuf_is_consumed(struct rte_mbuf *m)
 	return true;
 }
 
-static __rte_always_inline void
-mem_set_dump(__rte_unused void *ptr, __rte_unused size_t size, __rte_unused bool enable)
-{
-#ifdef MADV_DONTDUMP
-	if (madvise(ptr, size, enable ? MADV_DODUMP : MADV_DONTDUMP) == -1) {
-		rte_log(RTE_LOG_INFO, vhost_config_log_level,
-			"VHOST_CONFIG: could not set coredump preference (%s).\n", strerror(errno));
-	}
-#endif
-}
+uint64_t hua_to_alignment(struct rte_vhost_memory *mem, void *ptr);
+void mem_set_dump(void *ptr, size_t size, bool enable, uint64_t alignment);
 #endif /* _VHOST_NET_CDEV_H_ */

lib/vhost/vhost_user.c +55 ~~-13~~

176 lines not on this page · raw message

Signature · 1 line

2.31.1

Maxime Coquelin

Tested-by Reviewed-by on v3 1/1

2023-03-02 10:19 UTC #15 lore raw

Re: [PATCH v3] vhost: fix madvise arguments alignment

Hi Mike,

Quoted 4 lines · Mike Pattrick

The arguments passed to madvise should be aligned to the alignment of the backing memory. Now we keep track of each regions alignment and use then when setting coredump preferences. To facilitate this, a new member was added to rte_vhost_mem_region. A new function was added to easily

The above sentence is no more valid, I can rework it while applying.

Quoted 14 lines

translate memory address back to region alignment. Unneeded calls to madvise were reduced, as the cache removal case should already be covered by the cache insertion case. The previously inline function mem_set_dump was removed from a header file and made not inline.
Fixes 338ad77c9ed3 ("vhost: exclude VM hugepages from coredumps")
Signed-off-by Mike Pattrick <[email protected]>
---
Since v1:
- Corrected a cast for 32bit compiles
Since v2:
- Removed changes from rte_vhost.h
- Restored set_mem_dump in ioctl remove/evict functions

Quoted 7 lines

---
  lib/vhost/iotlb.c      | 68 +++++++++++++++++++++++++++++++-----------
  lib/vhost/iotlb.h      |  5 ++--
  lib/vhost/vhost.h      | 12 ++------
  lib/vhost/vhost_user.c | 68 ++++++++++++++++++++++++++++++++++--------
  4 files changed, 110 insertions(+), 43 deletions(-)

Tested-by Maxime Coquelin <[email protected]>

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Maxime Coquelin

Applied

2023-03-06 14:22 UTC #16 lore raw

Re: [PATCH v3] vhost: fix madvise arguments alignment

In reply to Mike Pattrick

Quoted 18 lines · Mike Pattrick

The arguments passed to madvise should be aligned to the alignment of the backing memory. Now we keep track of each regions alignment and use then when setting coredump preferences. To facilitate this, a new member was added to rte_vhost_mem_region. A new function was added to easily translate memory address back to region alignment. Unneeded calls to madvise were reduced, as the cache removal case should already be covered by the cache insertion case. The previously inline function mem_set_dump was removed from a header file and made not inline.
Fixes 338ad77c9ed3 ("vhost: exclude VM hugepages from coredumps")
Signed-off-by Mike Pattrick <[email protected]>
---
Since v1:
- Corrected a cast for 32bit compiles
Since v2:
- Removed changes from rte_vhost.h
- Restored set_mem_dump in ioctl remove/evict functions

Quoted 7 lines

---
  lib/vhost/iotlb.c      | 68 +++++++++++++++++++++++++++++++-----------
  lib/vhost/iotlb.h      |  5 ++--
  lib/vhost/vhost.h      | 12 ++------
  lib/vhost/vhost_user.c | 68 ++++++++++++++++++++++++++++++++++--------
  4 files changed, 110 insertions(+), 43 deletions(-)

Applied to dpdk-next-virtio/main.

Thanks,
Maxime

Stephen Hemminger

2023-02-03 17:45 UTC #17 lore raw

In reply to Mike Pattrick

lib/vhost/vhost.h mbuf_is_consumed(struct rte_mbuf *m) · line 1001

Quoted 11 lines · On Tue, 6 Dec 2022 10:05:09 -0500 Mike Pattrick <[email protected]> wrote:

lib/vhost/vhost.h +11 -0

+
+static __rte_always_inline void
+mem_set_dump(__rte_unused void *ptr, __rte_unused size_t size, __rte_unused bool enable)
+{
+#ifdef MADV_DONTDUMP
+	if (madvise(ptr, size, enable ? MADV_DODUMP : MADV_DONTDUMP) == -1) {
+		rte_log(RTE_LOG_INFO, vhost_config_log_level,
+			"VHOST_CONFIG: could not set coredump preference (%s).\n", strerror(errno));
+	}
+#endif
+}

Why is this inlined in vhost.h? Hardly a critical path function.

Mike Pattrick

author

2023-02-06 01:13 UTC #18 lore raw

Quoted 4 lines · Stephen Hemminger

On Tue, 6 Dec 2022 10:05:09 -0500
Mike Pattrick <[email protected]> wrote:

Quoted 14 lines

lib/vhost/vhost.h mbuf_is_consumed(struct rte_mbuf *m) · line 1001

Quoted 11 lines

lib/vhost/vhost.h +11 -0

+
+static __rte_always_inline void
+mem_set_dump(__rte_unused void *ptr, __rte_unused size_t size, __rte_unused bool enable)
+{
+#ifdef MADV_DONTDUMP
+     if (madvise(ptr, size, enable ? MADV_DODUMP : MADV_DONTDUMP) == -1) {
+             rte_log(RTE_LOG_INFO, vhost_config_log_level,
+                     "VHOST_CONFIG: could not set coredump preference (%s).\n", strerror(errno));
+     }
+#endif
+}

Why is this inlined in vhost.h? Hardly a critical path function.

Hello Steven,

I thought it was appropriate here due to the size and contents of that function.

Thanks,
M