Re: [dpdk-dev] [PATCH v3 13/19] vhost: register new regions with userfaultfd

[Maxime Coquelin]

On 10/05/2018 02:34 PM, Ilya Maximets wrote:

On 04.10.2018 11:13, Maxime Coquelin wrote:

Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
---
  lib/librte_vhost/vhost_user.c | 33 ++++++++++++++++++++++++++++++++-
  1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index ffb3ea60a..d61be2da6 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -968,6 +968,32 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct 
VhostUserMsg *msg,
                        mmap_size,
                        alignment,
                        mmap_offset);
+
+               if (dev->postcopy_listening) {
+#ifdef RTE_LIBRTE_VHOST_POSTCOPY
+                       struct uffdio_register reg_struct;
+
+                       reg_struct.range.start = (uint64_t)(uintptr_t)mmap_addr;
+                       reg_struct.range.len = mmap_size;
+                       reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
+
+                       if (ioctl(dev->postcopy_ufd, UFFDIO_REGISTER,
+                                               &reg_struct)) {
+                               RTE_LOG(ERR, VHOST_CONFIG,
+                                               "Failed to register ufd for region 
%d: (ufd = %d) %s\n",
+                                               i, dev->postcopy_ufd,
+                                               strerror(errno));
+                               goto err_ufd;
+                       }
+                       RTE_LOG(INFO, VHOST_CONFIG,
+                                       "\t userfaultfd registered for range : %llx 
- %llx\n",
+                                       reg_struct.range.start,
+                                       reg_struct.range.start +
+                                       reg_struct.range.len - 1);
+#else
+                       goto err_ufd;
+#endif
+               }
        }

for (i = 0; i < dev->nr_vring; i++) {

@@ -983,7 +1009,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct 
VhostUserMsg *msg,

dev = translate_ring_addresses(dev, i);

                        if (!dev)
-                               goto err_mmap;
+                               goto err_ufd;

*pdev = dev;

@@ -994,6 +1020,11 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct 
VhostUserMsg *msg,

return VH_RESULT_OK; +err_ufd:

+       if (dev->postcopy_ufd >= 0) {
+               close(dev->postcopy_ufd);
+               dev->postcopy_ufd = -1;
+       }

It's unclear why we need to close postcopy_ufd here because this
handler doesn't allocate it and postcopy_ufd should exist before
calling this function.
So, IMHO, we need to close it in all the VH_RESULT_ERR cases or
leave it for later vhost_backend_cleanup().

Right, I agree this is vhost_backend_cleanup()'s job. I will remove it
from here.

  err_mmap:
        free_mem_region(dev);
        rte_free(dev->mem);