| Message ID | 1471268075-3425-1-git-send-email-mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ack. You beat me to the patch by a few minutes :) Prerna On 15/08/16 7:05 pm, "Michael S. Tsirkin" <mst@redhat.com> wrote: >This reverts commit 28ed5ef16384f12500abd3647973ee21b03cbe23. > >I still think it's the right thing to do, but >tests have been failing sporadically. > >Revert for now, and hope to fix it before the release. > >Cc: Prerna Saxena <prerna.saxena@nutanix.com> >Cc: Peter Maydell <peter.maydell@linaro.org> >Cc: Marc-André Lureau <mlureau@redhat.com> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >--- > hw/virtio/vhost-user.c | 127 +++++++++++++++++++++++-------------------------- > 1 file changed, 60 insertions(+), 67 deletions(-) > >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c >index 1a7d53c..b57454a 100644 >--- a/hw/virtio/vhost-user.c >+++ b/hw/virtio/vhost-user.c >@@ -263,6 +263,66 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, > return 0; > } > >+static int vhost_user_set_mem_table(struct vhost_dev *dev, >+ struct vhost_memory *mem) >+{ >+ int fds[VHOST_MEMORY_MAX_NREGIONS]; >+ int i, fd; >+ size_t fd_num = 0; >+ bool reply_supported = virtio_has_feature(dev->protocol_features, >+ VHOST_USER_PROTOCOL_F_REPLY_ACK); >+ >+ VhostUserMsg msg = { >+ .request = VHOST_USER_SET_MEM_TABLE, >+ .flags = VHOST_USER_VERSION, >+ }; >+ >+ if (reply_supported) { >+ msg.flags |= VHOST_USER_NEED_REPLY_MASK; >+ } >+ >+ for (i = 0; i < dev->mem->nregions; ++i) { >+ struct vhost_memory_region *reg = dev->mem->regions + i; >+ ram_addr_t offset; >+ MemoryRegion *mr; >+ >+ assert((uintptr_t)reg->userspace_addr == reg->userspace_addr); >+ mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr, >+ &offset); >+ fd = memory_region_get_fd(mr); >+ if (fd > 0) { >+ msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr; >+ msg.payload.memory.regions[fd_num].memory_size = reg->memory_size; >+ msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr; >+ msg.payload.memory.regions[fd_num].mmap_offset = offset; >+ assert(fd_num < VHOST_MEMORY_MAX_NREGIONS); >+ fds[fd_num++] = fd; >+ } >+ } >+ >+ msg.payload.memory.nregions = fd_num; >+ >+ if (!fd_num) { >+ error_report("Failed initializing vhost-user memory map, " >+ "consider using -object memory-backend-file share=on"); >+ return -1; >+ } >+ >+ msg.size = sizeof(msg.payload.memory.nregions); >+ msg.size += sizeof(msg.payload.memory.padding); >+ msg.size += fd_num * sizeof(VhostUserMemoryRegion); >+ >+ if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { >+ return -1; >+ } >+ >+ if (reply_supported) { >+ return process_message_reply(dev, msg.request); >+ } >+ >+ return 0; >+} >+ > static int vhost_user_set_vring_addr(struct vhost_dev *dev, > struct vhost_vring_addr *addr) > { >@@ -477,73 +537,6 @@ static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features) > return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features); > } > >-static int vhost_user_set_mem_table(struct vhost_dev *dev, >- struct vhost_memory *mem) >-{ >- int fds[VHOST_MEMORY_MAX_NREGIONS]; >- int i, fd; >- size_t fd_num = 0; >- uint64_t features; >- bool reply_supported = virtio_has_feature(dev->protocol_features, >- VHOST_USER_PROTOCOL_F_REPLY_ACK); >- >- VhostUserMsg msg = { >- .request = VHOST_USER_SET_MEM_TABLE, >- .flags = VHOST_USER_VERSION, >- }; >- >- if (reply_supported) { >- msg.flags |= VHOST_USER_NEED_REPLY_MASK; >- } >- >- for (i = 0; i < dev->mem->nregions; ++i) { >- struct vhost_memory_region *reg = dev->mem->regions + i; >- ram_addr_t offset; >- MemoryRegion *mr; >- >- assert((uintptr_t)reg->userspace_addr == reg->userspace_addr); >- mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr, >- &offset); >- fd = memory_region_get_fd(mr); >- if (fd > 0) { >- msg.payload.memory.regions[fd_num].userspace_addr >- = reg->userspace_addr; >- msg.payload.memory.regions[fd_num].memory_size = reg->memory_size; >- msg.payload.memory.regions[fd_num].guest_phys_addr >- = reg->guest_phys_addr; >- msg.payload.memory.regions[fd_num].mmap_offset = offset; >- assert(fd_num < VHOST_MEMORY_MAX_NREGIONS); >- fds[fd_num++] = fd; >- } >- } >- >- msg.payload.memory.nregions = fd_num; >- >- if (!fd_num) { >- error_report("Failed initializing vhost-user memory map, " >- "consider using -object memory-backend-file share=on"); >- return -1; >- } >- >- msg.size = sizeof(msg.payload.memory.nregions); >- msg.size += sizeof(msg.payload.memory.padding); >- msg.size += fd_num * sizeof(VhostUserMemoryRegion); >- >- vhost_user_write(dev, &msg, fds, fd_num); >- >- if (reply_supported) { >- return process_message_reply(dev, msg.request); >- } else { >- /* Note: It is (yet) unknown when the client application has finished >- * remapping the GPA. >- * Attempt to prevent a race by sending a command that requires a reply. >- */ >- vhost_user_get_features(dev, &features); >- } >- >- return 0; >-} >- > static int vhost_user_set_owner(struct vhost_dev *dev) > { > VhostUserMsg msg = { >-- >MST
On 15 August 2016 at 14:35, Michael S. Tsirkin <mst@redhat.com> wrote: > This reverts commit 28ed5ef16384f12500abd3647973ee21b03cbe23. > > I still think it's the right thing to do, but > tests have been failing sporadically. > > Revert for now, and hope to fix it before the release. > > Cc: Prerna Saxena <prerna.saxena@nutanix.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Marc-André Lureau <mlureau@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- Applied, thanks. I found my clang-on-x86-64 Linux Ubuntu xenial build would hang in vhost-user/read-guest-mem after 10 or so iterations, but with this revert applied it seems fine, so I think this commit was definitely the culprit. -- PMM
On Mon, Aug 15, 2016 at 04:15:08PM +0100, Peter Maydell wrote: > On 15 August 2016 at 14:35, Michael S. Tsirkin <mst@redhat.com> wrote: > > This reverts commit 28ed5ef16384f12500abd3647973ee21b03cbe23. > > > > I still think it's the right thing to do, but > > tests have been failing sporadically. > > > > Revert for now, and hope to fix it before the release. > > > > Cc: Prerna Saxena <prerna.saxena@nutanix.com> > > Cc: Peter Maydell <peter.maydell@linaro.org> > > Cc: Marc-André Lureau <mlureau@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > Applied, thanks. I found my clang-on-x86-64 Linux Ubuntu xenial > build would hang in vhost-user/read-guest-mem after 10 or > so iterations, but with this revert applied it seems fine, > so I think this commit was definitely the culprit. > > -- PMM That's nice for the RC, but I think we do want to have the underlying issue fixed down the road. Prerna, Marc André - any chance you could try reproducing on an ubuntu guest? In particular to make sure the issue whatever it is will not tigger once clients negotiate the new feature bit.
On 16/08/16 2:39 am, "Michael S. Tsirkin" <mst@redhat.com> wrote: >On Mon, Aug 15, 2016 at 04:15:08PM +0100, Peter Maydell wrote: >> On 15 August 2016 at 14:35, Michael S. Tsirkin <mst@redhat.com> wrote: >> > This reverts commit 28ed5ef16384f12500abd3647973ee21b03cbe23. >> > >> > I still think it's the right thing to do, but >> > tests have been failing sporadically. >> > >> > Revert for now, and hope to fix it before the release. >> > >> > Cc: Prerna Saxena <prerna.saxena@nutanix.com> >> > Cc: Peter Maydell <peter.maydell@linaro.org> >> > Cc: Marc-André Lureau <mlureau@redhat.com> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> > --- >> >> Applied, thanks. I found my clang-on-x86-64 Linux Ubuntu xenial >> build would hang in vhost-user/read-guest-mem after 10 or >> so iterations, but with this revert applied it seems fine, >> so I think this commit was definitely the culprit. >> >> -- PMM > >That's nice for the RC, but I think we do want to >have the underlying issue fixed down the road. >Prerna, Marc André - any chance you could try >reproducing on an ubuntu guest? > >In particular to make sure the issue whatever it is >will not tigger once clients negotiate the >new feature bit. Sure, I’ll give it a try. Prerna
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 1a7d53c..b57454a 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -263,6 +263,66 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base, return 0; } +static int vhost_user_set_mem_table(struct vhost_dev *dev, + struct vhost_memory *mem) +{ + int fds[VHOST_MEMORY_MAX_NREGIONS]; + int i, fd; + size_t fd_num = 0; + bool reply_supported = virtio_has_feature(dev->protocol_features, + VHOST_USER_PROTOCOL_F_REPLY_ACK); + + VhostUserMsg msg = { + .request = VHOST_USER_SET_MEM_TABLE, + .flags = VHOST_USER_VERSION, + }; + + if (reply_supported) { + msg.flags |= VHOST_USER_NEED_REPLY_MASK; + } + + for (i = 0; i < dev->mem->nregions; ++i) { + struct vhost_memory_region *reg = dev->mem->regions + i; + ram_addr_t offset; + MemoryRegion *mr; + + assert((uintptr_t)reg->userspace_addr == reg->userspace_addr); + mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr, + &offset); + fd = memory_region_get_fd(mr); + if (fd > 0) { + msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr; + msg.payload.memory.regions[fd_num].memory_size = reg->memory_size; + msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr; + msg.payload.memory.regions[fd_num].mmap_offset = offset; + assert(fd_num < VHOST_MEMORY_MAX_NREGIONS); + fds[fd_num++] = fd; + } + } + + msg.payload.memory.nregions = fd_num; + + if (!fd_num) { + error_report("Failed initializing vhost-user memory map, " + "consider using -object memory-backend-file share=on"); + return -1; + } + + msg.size = sizeof(msg.payload.memory.nregions); + msg.size += sizeof(msg.payload.memory.padding); + msg.size += fd_num * sizeof(VhostUserMemoryRegion); + + if (vhost_user_write(dev, &msg, fds, fd_num) < 0) { + return -1; + } + + if (reply_supported) { + return process_message_reply(dev, msg.request); + } + + return 0; +} + static int vhost_user_set_vring_addr(struct vhost_dev *dev, struct vhost_vring_addr *addr) { @@ -477,73 +537,6 @@ static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features) return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features); } -static int vhost_user_set_mem_table(struct vhost_dev *dev, - struct vhost_memory *mem) -{ - int fds[VHOST_MEMORY_MAX_NREGIONS]; - int i, fd; - size_t fd_num = 0; - uint64_t features; - bool reply_supported = virtio_has_feature(dev->protocol_features, - VHOST_USER_PROTOCOL_F_REPLY_ACK); - - VhostUserMsg msg = { - .request = VHOST_USER_SET_MEM_TABLE, - .flags = VHOST_USER_VERSION, - }; - - if (reply_supported) { - msg.flags |= VHOST_USER_NEED_REPLY_MASK; - } - - for (i = 0; i < dev->mem->nregions; ++i) { - struct vhost_memory_region *reg = dev->mem->regions + i; - ram_addr_t offset; - MemoryRegion *mr; - - assert((uintptr_t)reg->userspace_addr == reg->userspace_addr); - mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr, - &offset); - fd = memory_region_get_fd(mr); - if (fd > 0) { - msg.payload.memory.regions[fd_num].userspace_addr - = reg->userspace_addr; - msg.payload.memory.regions[fd_num].memory_size = reg->memory_size; - msg.payload.memory.regions[fd_num].guest_phys_addr - = reg->guest_phys_addr; - msg.payload.memory.regions[fd_num].mmap_offset = offset; - assert(fd_num < VHOST_MEMORY_MAX_NREGIONS); - fds[fd_num++] = fd; - } - } - - msg.payload.memory.nregions = fd_num; - - if (!fd_num) { - error_report("Failed initializing vhost-user memory map, " - "consider using -object memory-backend-file share=on"); - return -1; - } - - msg.size = sizeof(msg.payload.memory.nregions); - msg.size += sizeof(msg.payload.memory.padding); - msg.size += fd_num * sizeof(VhostUserMemoryRegion); - - vhost_user_write(dev, &msg, fds, fd_num); - - if (reply_supported) { - return process_message_reply(dev, msg.request); - } else { - /* Note: It is (yet) unknown when the client application has finished - * remapping the GPA. - * Attempt to prevent a race by sending a command that requires a reply. - */ - vhost_user_get_features(dev, &features); - } - - return 0; -} - static int vhost_user_set_owner(struct vhost_dev *dev) { VhostUserMsg msg = {
This reverts commit 28ed5ef16384f12500abd3647973ee21b03cbe23. I still think it's the right thing to do, but tests have been failing sporadically. Revert for now, and hope to fix it before the release. Cc: Prerna Saxena <prerna.saxena@nutanix.com> Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Marc-André Lureau <mlureau@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/virtio/vhost-user.c | 127 +++++++++++++++++++++++-------------------------- 1 file changed, 60 insertions(+), 67 deletions(-)