Thread (4 messages) 4 messages, 3 authors, 2017-09-25

Re: [PATCH] vhost: fix vhost_user_set_mem_table error

From: Tan, Jianfeng <hidden>
Date: 2017-09-22 05:28:52

quoted hunk ↗ jump to hunk
-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yi Yang
Sent: Wednesday, September 20, 2017 4:32 PM
To: yliu@fridaylinux.org
Cc: dev@dpdk.org; Yang, Yi Y
Subject: [dpdk-dev] [PATCH] vhost: fix vhost_user_set_mem_table error

Usually vhost_user message VHOST_USER_SET_MEM_TABLE
is only sent out during initialization and only sent
once, but it isn't so for memory hotplug and hotunplug
, for that case, vhost_user message VHOST_USER_SET_MEM_TABLE
will be resent with the old memory regions (not hotunplugged)
and the new memory regions (hotplugged), so current
vhost_user_set_mem_table implementation is wrong for
memory hotplug and hotunplug case, it will free current
memory regions and unmap hugepages no matter if they
are be using or not and if they are memory regions which
have been initialized and mapped in the previous vhost_user
message VHOST_USER_SET_MEM_TABLE or not.

This commit fixed this issue very well, it will keep them
intact for those old memory region it will unmap those
memroy regions if they have been hotunplugged, it will
initialize the new hotplugged memory regions and map
hugepages if they are hotplugged.

vhost_user message VHOST_USER_SET_MEM_TABLE will include
all the current effective memory regions, the hotunplugged
memory regions won't be included in VHOST_USER_SET_MEM_TABLE,
the hotplugged memroy regions will be included in
VHOST_USER_SET_MEM_TABLE.

This has been verified in OVS DPDK by memory hotplug and
hotunplug in qemu.

Signed-off-by: Yi Yang <redacted>
---
 lib/librte_vhost/vhost_user.c | 72
++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 7 deletions(-)
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index ad2e8d3..1c475d1 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -509,17 +509,43 @@ vhost_user_set_mem_table(struct virtio_net *dev,
struct VhostUserMsg *pmsg)
 	uint64_t mmap_size;
 	uint64_t mmap_offset;
 	uint64_t alignment;
-	uint32_t i;
+	uint32_t i, j;
 	int fd;

-	if (dev->mem) {
-		free_mem_region(dev);
-		rte_free(dev->mem);
-		dev->mem = NULL;
+	/* Handle hot unplug case */
+	if (dev->mem && dev->mem->nregions) {
+		i = 0;
+		j = 0;
+		while ((i < memory.nregions) && (j < dev->mem->nregions))
{
+			reg = &dev->mem->regions[j];
+			/* munmap this region if it is hot unplugged */
+			if (reg->guest_user_addr
+				!= memory.regions[i].userspace_addr) {
+				if (reg->host_user_addr) {
+					munmap(reg->mmap_addr, reg-
quoted
mmap_size);
+					close(reg->fd);
+				}
+				reg->guest_user_addr = 0;
+				j++;
+			} else {
+				i++;
+				j++;
+			}
+		}
The algorithm here could be problematic for hot plug. For example,

T0: we have two regions with address 1, 3.
T1: we add another region, 2, which makes the regions as 1, 2, 3.

1st iteration, 1 matches 1, i++, j++;
2nd iteration, 2 does not match 3, unmap 3, j++.
+
+		/* munmap these regions because they have been hot
unplugged */
+		for (; j < dev->mem->nregions; j++) {
+			reg = &dev->mem->regions[j];
+			if (reg->host_user_addr) {
+				munmap(reg->mmap_addr, reg-
quoted
mmap_size);
+				close(reg->fd);
+			}
+			reg->guest_user_addr = 0;
+		}
 	}

-	dev->nr_guest_pages = 0;
 	if (!dev->guest_pages) {
+		dev->nr_guest_pages = 0;
We cannot just move this line here, which results in a wrong nr_guest_pages if we do hot plug/unplug instead of initialization.
quoted hunk ↗ jump to hunk
 		dev->max_guest_pages = 8;
 		dev->guest_pages = malloc(dev->max_guest_pages *
 						sizeof(struct guest_page));
@@ -532,7 +558,7 @@ vhost_user_set_mem_table(struct virtio_net *dev,
struct VhostUserMsg *pmsg)
 		}
 	}

-	dev->mem = rte_zmalloc("vhost-mem-table", sizeof(struct
rte_vhost_memory) +
+	dev->mem = rte_realloc(dev->mem, sizeof(struct
rte_vhost_memory) +
 		sizeof(struct rte_vhost_mem_region) * memory.nregions, 0);
 	if (dev->mem == NULL) {
 		RTE_LOG(ERR, VHOST_CONFIG,
@@ -546,6 +572,38 @@ vhost_user_set_mem_table(struct virtio_net *dev,
struct VhostUserMsg *pmsg)
 		fd  = pmsg->fds[i];
 		reg = &dev->mem->regions[i];

+		/* This region should be skipped if it is initialized before */
+		if (reg->guest_user_addr ==
memory.regions[i].userspace_addr) {
Also problematic for hot unplug. For example,
T0: we have three regions, 1, 2, 3.
T1: remove region 2, with only 1, 3 left.

1st iteration, 1 matches 1, skip old region 1.
2nd iteration, 3 does not match 2, do the mmap.

+			alignment = get_blk_size(fd);
+			if (alignment == (uint64_t)-1) {
+				RTE_LOG(ERR, VHOST_CONFIG,
+					"couldn't get hugepage size "
+					"through fstat\n");
+				goto err_mmap;
+			}
+			RTE_LOG(INFO, VHOST_CONFIG,
+				"guest memory region(old)"
+				" %u, size: 0x%" PRIx64 "\n"
+				"\t guest physical addr: 0x%" PRIx64 "\n"
+				"\t guest virtual  addr: 0x%" PRIx64 "\n"
+				"\t host  virtual  addr: 0x%" PRIx64 "\n"
+				"\t mmap addr : 0x%" PRIx64 "\n"
+				"\t mmap size : 0x%" PRIx64 "\n"
+				"\t mmap align: 0x%" PRIx64 "\n"
+				"\t mmap off  : 0x%" PRIx64 "\n",
+				i, reg->size,
+				reg->guest_phys_addr,
+				reg->guest_user_addr,
+				reg->host_user_addr,
+				(uint64_t)(uintptr_t)reg->mmap_addr,
+				reg->mmap_size,
+				alignment,
+				(uint64_t)(uintptr_t)reg->host_user_addr -
+					(uint64_t)(uintptr_t)reg-
quoted
mmap_addr
+				);
+				continue;
+		}
+
 		reg->guest_phys_addr =
memory.regions[i].guest_phys_addr;
 		reg->guest_user_addr = memory.regions[i].userspace_addr;
 		reg->size            = memory.regions[i].memory_size;
--
2.1.0
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help