| Message ID | 20240711131424.181615-1-ppandit@redhat.com |
|---|---|
| Headers | show |
| Series | Postcopy migration and vhost-user errors | expand |
On Thu, Jul 11, 2024 at 06:44:22PM +0530, Prasad Pandit wrote: > From: Prasad Pandit <pjp@fedoraproject.org> > > Hello, > > * virsh(1) offers multiple options to initiate Postcopy migration: > > 1) virsh migrate --postcopy --postcopy-after-precopy > 2) virsh migrate --postcopy + virsh migrate-postcopy > 3) virsh migrate --postcopy --timeout <N> --timeout-postcopy > > When Postcopy migration is invoked via method (2) or (3) above, > the guest on the destination host seems to hang or get stuck sometimes. > > * During Postcopy migration, multiple threads are spawned on the destination > host to start the guest and setup devices. One such thread starts vhost > device via vhost_dev_start() function and another called fault_thread handles Hmm, I thought it was one of the vcpu threads that invoked vhost_dev_start(), rather than any migration thread? > page faults in user space using kernel's userfaultfd(2) system. > > When fault_thread exits upon completion of Postcopy migration, it sends a > 'postcopy_end' message to the vhost-user device. But sometimes 'postcopy_end' > message is sent while vhost device is being setup via vhost_dev_start(). > > Thread-1 Thread-2 > > vhost_dev_start postcopy_ram_incoming_cleanup > vhost_device_iotlb_miss postcopy_notify > vhost_backend_update_device_iotlb vhost_user_postcopy_notifier > vhost_user_send_device_iotlb_msg vhost_user_postcopy_end > process_message_reply process_message_reply > vhost_user_read vhost_user_read > vhost_user_read_header vhost_user_read_header > "Fail to update device iotlb" "Failed to receive reply to postcopy_end" > > This creates confusion when vhost device receives 'postcopy_end' message while > it is still trying to update IOTLB entries. > > This seems to leave the guest in a stranded/hung state because fault_thread > has exited saying Postcopy migration has ended, but vhost-device is probably > still expecting updates. QEMU logs following errors on the destination host > === > ... > qemu-kvm: vhost_user_read_header: 700871,700871: Failed to read msg header. Flags 0x0 instead of 0x5. > qemu-kvm: vhost_device_iotlb_miss: 700871,700871: Fail to update device iotlb > qemu-kvm: vhost_user_postcopy_end: 700871,700900: Failed to receive reply to postcopy_end > qemu-kvm: vhost_user_read_header: 700871,700871: Failed to read msg header. Flags 0x0 instead of 0x5. > qemu-kvm: vhost_device_iotlb_miss: 700871,700871: Fail to update device iotlb > qemu-kvm: vhost_user_read_header: 700871,700871: Failed to read msg header. Flags 0x8 instead of 0x5. > qemu-kvm: vhost_device_iotlb_miss: 700871,700871: Fail to update device iotlb > qemu-kvm: vhost_user_read_header: 700871,700871: Failed to read msg header. Flags 0x16 instead of 0x5. > qemu-kvm: vhost_device_iotlb_miss: 700871,700871: Fail to update device iotlb > qemu-kvm: vhost_user_read_header: 700871,700871: Failed to read msg header. Flags 0x0 instead of 0x5. > qemu-kvm: vhost_device_iotlb_miss: 700871,700871: Fail to update device iotlb > === > > * Couple of patches here help to fix/handle these errors. I remember after you added the rwlock, there's still a hang issue. Did you investigated that? Or do you mean this series will fix all the problems? Thanks, > > Thank you. > --- > Prasad Pandit (2): > vhost-user: add a write-read lock > vhost: fail device start if iotlb update fails > > hw/virtio/vhost-user.c | 423 +++++++++++++++++++-------------- > hw/virtio/vhost.c | 6 +- > include/hw/virtio/vhost-user.h | 3 + > 3 files changed, 259 insertions(+), 173 deletions(-) > > -- > 2.45.2 >
On Thu, 11 Jul 2024 at 21:08, Peter Xu <peterx@redhat.com> wrote: > Hmm, I thought it was one of the vcpu threads that invoked > vhost_dev_start(), rather than any migration thread? [QEMU=vhost-user-front-end] <===========> [QEMU=vhost-user-front-end] ^ | | | | | | V [external-process=vhost-user-back-end] [external-process=vhost-user-back-end] === vhost-user-protocol: -> https://www.qemu.org/docs/master/interop/vhost-user.html#vhost-user-proto * It is not clear which thread calls vhost_dev_start() routine, it could be a vCPU thread. Sending 'postcopy_end' message to the 'vhost-user-back-end', hints that the device was being migrated and migration finished before the device set-up was done. The protocol above says "...The nature of the channel is implementation-defined, but it must generally behave like a pipe: The writing end will write all the data it has into it, signalling the end of data by closing its end. The reading end must read all of this data (until encountering the end of file) and process it." * It does not mention sending the 'postcopy_end' message. But it talks about the front-end sending 'VHOST_USER_CHECK_DEVICE_STATE' to the back-end to check if the migration of the device state was successful or not. > I remember after you added the rwlock, there's still a hang issue. > Did you investigated that? Or do you mean this series will fix all the problems? * No, this series does not fix the guest hang issue. Root cause of that is still a mystery. If migration is ending abruptly before all of the guest state is migrated, the guest hang scenario seems possible. Adding vhost-user-rw-lock does not address the issue of end of migration. * From the protocol page above, it is not clear if the front-end should allow/have multiple threads talking to the same vhost-user device. Thank you. --- - Prasad
On Mon, Jul 15, 2024 at 03:44:06PM +0530, Prasad Pandit wrote: > > I remember after you added the rwlock, there's still a hang issue. > > Did you investigated that? Or do you mean this series will fix all the problems? > > * No, this series does not fix the guest hang issue. Root cause of > that is still a mystery. If migration is ending abruptly before all of > the guest state is migrated, the guest hang scenario seems possible. > Adding vhost-user-rw-lock does not address the issue of end of > migration. IMHO it's better we debug and fix all the issues before merging this one, otherwise we may overlook something. You could pass over the patch to whoever going to debug this, so it will be included in the whole set to be posted when the bug is completely fixed. > * From the protocol page above, it is not clear if the front-end > should allow/have multiple threads talking to the same vhost-user > device. The protocol should have no restriction on the thread model of a front-end. It only describes the wire protocol. IIUC the protocol was designed to be serialized by nature (where there's no request ID, so we can't match reply to any of the previous response), then the front-end can manage the threads well to serialize all the requests, like using this rwlock. Thanks,
Hello Peter, On Mon, 15 Jul 2024 at 19:10, Peter Xu <peterx@redhat.com> wrote: > IMHO it's better we debug and fix all the issues before merging this one, > otherwise we may overlook something. * Well we don't know where the issue is, not sure where the fix may go in, ex. if the issue turns out to be how virsh(1) invokes migrate-postcopy, fix may go in virsh(1). Patches in this series anyway don't help to fix the migration convergence issue, so they could be reviewed independently I guess. > You could pass over the patch to whoever going to debug this, so it will be included in the whole set to be > posted when the bug is completely fixed. * Yes, this patch series is linked there. > The protocol should have no restriction on the thread model of a front-end. > It only describes the wire protocol. > > IIUC the protocol was designed to be serialized by nature (where there's no > request ID, so we can't match reply to any of the previous response), then > the front-end can manage the threads well to serialize all the requests, > like using this rwlock. * I see, okay. The simple protocol definition seems to indicate that it is meant for one front-end/back-end pair. If we are dividing the front-end across multiple threads, maybe we need a document to describe those threads and how they work, at least for the QEMU (front-end) side. Because the back-end could be a non-QEMU process, we can not do much there. (just thinking) Thank you. --- - Prasad
On Tue, Jul 16, 2024 at 03:44:54PM +0530, Prasad Pandit wrote: > Hello Peter, > > On Mon, 15 Jul 2024 at 19:10, Peter Xu <peterx@redhat.com> wrote: > > IMHO it's better we debug and fix all the issues before merging this one, > > otherwise we may overlook something. > > * Well we don't know where the issue is, not sure where the fix may go > in, ex. if the issue turns out to be how virsh(1) invokes > migrate-postcopy, fix may go in virsh(1). Patches in this series > anyway don't help to fix the migration convergence issue, so they > could be reviewed independently I guess. I still think we should find a complete solution before merging anything, because I'm not 100% confident the issue to be further investigated is irrelevant to this patch. No strong opinions, I'll leave that to Michael to decide. > > > You could pass over the patch to whoever going to debug this, so it will be included in the whole set to be > > posted when the bug is completely fixed. > > * Yes, this patch series is linked there. > > > The protocol should have no restriction on the thread model of a front-end. > > It only describes the wire protocol. > > > > IIUC the protocol was designed to be serialized by nature (where there's no > > request ID, so we can't match reply to any of the previous response), then > > the front-end can manage the threads well to serialize all the requests, > > like using this rwlock. > > * I see, okay. The simple protocol definition seems to indicate that > it is meant for one front-end/back-end pair. If we are dividing the > front-end across multiple threads, maybe we need a document to > describe those threads and how they work, at least for the QEMU > (front-end) side. Because the back-end could be a non-QEMU process, we > can not do much there. (just thinking) IMHO that's not part of the protocol but impl details, so the current doc looks all fine to me. Thanks,
On Tue, Jul 16, 2024 at 06:02:50PM -0400, Peter Xu wrote: > On Tue, Jul 16, 2024 at 03:44:54PM +0530, Prasad Pandit wrote: > > Hello Peter, > > > > On Mon, 15 Jul 2024 at 19:10, Peter Xu <peterx@redhat.com> wrote: > > > IMHO it's better we debug and fix all the issues before merging this one, > > > otherwise we may overlook something. > > > > * Well we don't know where the issue is, not sure where the fix may go > > in, ex. if the issue turns out to be how virsh(1) invokes > > migrate-postcopy, fix may go in virsh(1). Patches in this series > > anyway don't help to fix the migration convergence issue, so they > > could be reviewed independently I guess. > > I still think we should find a complete solution before merging anything, > because I'm not 100% confident the issue to be further investigated is > irrelevant to this patch. > > No strong opinions, I'll leave that to Michael to decide. > > > > > > You could pass over the patch to whoever going to debug this, so it will be included in the whole set to be > > > posted when the bug is completely fixed. > > > > * Yes, this patch series is linked there. > > > > > The protocol should have no restriction on the thread model of a front-end. > > > It only describes the wire protocol. > > > > > > IIUC the protocol was designed to be serialized by nature (where there's no > > > request ID, so we can't match reply to any of the previous response), then > > > the front-end can manage the threads well to serialize all the requests, > > > like using this rwlock. > > > > * I see, okay. The simple protocol definition seems to indicate that > > it is meant for one front-end/back-end pair. If we are dividing the > > front-end across multiple threads, maybe we need a document to > > describe those threads and how they work, at least for the QEMU > > (front-end) side. Because the back-end could be a non-QEMU process, we > > can not do much there. (just thinking) > > IMHO that's not part of the protocol but impl details, so the current doc > looks all fine to me. > > Thanks, > > -- > Peter Xu I just want to understand how we managed to have two threads talking in parallel. BQL is normally enough, which path manages to invoke vhost-user with BQL not taken? Just check BQL taken on each vhost user invocation and you will figure it out.
Hi, Michael, On Wed, Jul 17, 2024 at 04:55:52AM -0400, Michael S. Tsirkin wrote: > I just want to understand how we managed to have two threads > talking in parallel. BQL is normally enough, which path > manages to invoke vhost-user with BQL not taken? > Just check BQL taken on each vhost user invocation and > you will figure it out. Prasad mentioned how the race happened in the cover letter: https://lore.kernel.org/r/20240711131424.181615-1-ppandit@redhat.com Thread-1 Thread-2 vhost_dev_start postcopy_ram_incoming_cleanup vhost_device_iotlb_miss postcopy_notify vhost_backend_update_device_iotlb vhost_user_postcopy_notifier vhost_user_send_device_iotlb_msg vhost_user_postcopy_end process_message_reply process_message_reply vhost_user_read vhost_user_read vhost_user_read_header vhost_user_read_header "Fail to update device iotlb" "Failed to receive reply to postcopy_end" The normal case should be that thread-2 is postcopy_ram_listen_thread(), and this happens when postcopy migration is close to the end. Thanks,
On Wed, Jul 17, 2024 at 09:33:01AM -0400, Peter Xu wrote: > Hi, Michael, > > On Wed, Jul 17, 2024 at 04:55:52AM -0400, Michael S. Tsirkin wrote: > > I just want to understand how we managed to have two threads > > talking in parallel. BQL is normally enough, which path > > manages to invoke vhost-user with BQL not taken? > > Just check BQL taken on each vhost user invocation and > > you will figure it out. > > Prasad mentioned how the race happened in the cover letter: > > https://lore.kernel.org/r/20240711131424.181615-1-ppandit@redhat.com > > Thread-1 Thread-2 > > vhost_dev_start postcopy_ram_incoming_cleanup > vhost_device_iotlb_miss postcopy_notify > vhost_backend_update_device_iotlb vhost_user_postcopy_notifier > vhost_user_send_device_iotlb_msg vhost_user_postcopy_end > process_message_reply process_message_reply > vhost_user_read vhost_user_read > vhost_user_read_header vhost_user_read_header > "Fail to update device iotlb" "Failed to receive reply to postcopy_end" > > The normal case should be that thread-2 is postcopy_ram_listen_thread(), > and this happens when postcopy migration is close to the end. > > Thanks, > > -- > Peter Xu OK, so postcopy_ram_ things run without the BQL?
On Wed, Jul 17, 2024 at 09:40:06AM -0400, Michael S. Tsirkin wrote: > On Wed, Jul 17, 2024 at 09:33:01AM -0400, Peter Xu wrote: > > Hi, Michael, > > > > On Wed, Jul 17, 2024 at 04:55:52AM -0400, Michael S. Tsirkin wrote: > > > I just want to understand how we managed to have two threads > > > talking in parallel. BQL is normally enough, which path > > > manages to invoke vhost-user with BQL not taken? > > > Just check BQL taken on each vhost user invocation and > > > you will figure it out. > > > > Prasad mentioned how the race happened in the cover letter: > > > > https://lore.kernel.org/r/20240711131424.181615-1-ppandit@redhat.com > > > > Thread-1 Thread-2 > > > > vhost_dev_start postcopy_ram_incoming_cleanup > > vhost_device_iotlb_miss postcopy_notify > > vhost_backend_update_device_iotlb vhost_user_postcopy_notifier > > vhost_user_send_device_iotlb_msg vhost_user_postcopy_end > > process_message_reply process_message_reply > > vhost_user_read vhost_user_read > > vhost_user_read_header vhost_user_read_header > > "Fail to update device iotlb" "Failed to receive reply to postcopy_end" > > > > The normal case should be that thread-2 is postcopy_ram_listen_thread(), > > and this happens when postcopy migration is close to the end. > > > > Thanks, > > > > -- > > Peter Xu > > > OK, so postcopy_ram_ things run without the BQL? There are a lot of postcopy_ram_* functions, I didn't check all of them but I think it's true in this case. Thanks.
On Wed, Jul 17, 2024 at 09:47:17AM -0400, Peter Xu wrote: > On Wed, Jul 17, 2024 at 09:40:06AM -0400, Michael S. Tsirkin wrote: > > On Wed, Jul 17, 2024 at 09:33:01AM -0400, Peter Xu wrote: > > > Hi, Michael, > > > > > > On Wed, Jul 17, 2024 at 04:55:52AM -0400, Michael S. Tsirkin wrote: > > > > I just want to understand how we managed to have two threads > > > > talking in parallel. BQL is normally enough, which path > > > > manages to invoke vhost-user with BQL not taken? > > > > Just check BQL taken on each vhost user invocation and > > > > you will figure it out. > > > > > > Prasad mentioned how the race happened in the cover letter: > > > > > > https://lore.kernel.org/r/20240711131424.181615-1-ppandit@redhat.com > > > > > > Thread-1 Thread-2 > > > > > > vhost_dev_start postcopy_ram_incoming_cleanup > > > vhost_device_iotlb_miss postcopy_notify > > > vhost_backend_update_device_iotlb vhost_user_postcopy_notifier > > > vhost_user_send_device_iotlb_msg vhost_user_postcopy_end > > > process_message_reply process_message_reply > > > vhost_user_read vhost_user_read > > > vhost_user_read_header vhost_user_read_header > > > "Fail to update device iotlb" "Failed to receive reply to postcopy_end" > > > > > > The normal case should be that thread-2 is postcopy_ram_listen_thread(), > > > and this happens when postcopy migration is close to the end. > > > > > > Thanks, > > > > > > -- > > > Peter Xu > > > > > > OK, so postcopy_ram_ things run without the BQL? > > There are a lot of postcopy_ram_* functions, I didn't check all of them but > I think it's true in this case. Thanks. > > -- > Peter Xu So pls document this in the commit log.
On Sun, 21 Jul 2024 at 01:11, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Wed, Jul 17, 2024 at 04:55:52AM -0400, Michael S. Tsirkin wrote: > > > > > I just want to understand how we managed to have two threads > > > > > talking in parallel. BQL is normally enough, which path > > > > > manages to invoke vhost-user with BQL not taken? > > > > > Just check BQL taken on each vhost user invocation and > > > > > you will figure it out. > > > > > > > OK, so postcopy_ram_ things run without the BQL? > > > > There are a lot of postcopy_ram_* functions, I didn't check all of them but > > I think it's true in this case. Thanks. > > > So pls document this in the commit log. * ie. IIUC, if we take BQL in postcop_ram_* functions, we don't need this 'vhost_user_request_reply_lock patch'? I'll check the postcopy_ram_* functions to see what's happening there. Thank you. --- - Prasad
On Tue, Jul 23, 2024 at 10:33:58AM +0530, Prasad Pandit wrote: > On Sun, 21 Jul 2024 at 01:11, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > On Wed, Jul 17, 2024 at 04:55:52AM -0400, Michael S. Tsirkin wrote: > > > > > > I just want to understand how we managed to have two threads > > > > > > talking in parallel. BQL is normally enough, which path > > > > > > manages to invoke vhost-user with BQL not taken? > > > > > > Just check BQL taken on each vhost user invocation and > > > > > > you will figure it out. > > > > > > > > > OK, so postcopy_ram_ things run without the BQL? > > > > > > There are a lot of postcopy_ram_* functions, I didn't check all of them but > > > I think it's true in this case. Thanks. > > > > > So pls document this in the commit log. > > * ie. IIUC, if we take BQL in postcop_ram_* functions, we don't need > this 'vhost_user_request_reply_lock patch'? I'll check the > postcopy_ram_* functions to see what's happening there. Go ahead, Prasad. But just to mention postcopy stuff doesn't take BQL may be because there's some good reason. So if you're not confident on some changes, you can share a patch before testing in an involved environment. Personally, I'd avoid using BQL as long as it can be replaced by a finer grained lock. Otherwise the lock semantics can be more unclear, and we'll never get rid of BQL. IOW, I'd prefer a smaller lock in general if possible to avoid BQL, but we can see how the patch look like when it's ready. Thanks,
Hi, On Tue, 23 Jul 2024 at 10:33, Prasad Pandit <ppandit@redhat.com> wrote: > On Sun, 21 Jul 2024 at 01:11, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > On Wed, Jul 17, 2024 at 04:55:52AM -0400, Michael S. Tsirkin wrote: > > > > > > I just want to understand how we managed to have two threads > > > > > > talking in parallel. BQL is normally enough, which path > > > > > > manages to invoke vhost-user with BQL not taken? > > > > > > Just check BQL taken on each vhost user invocation and > > > > > > you will figure it out. > > > > > > > > > OK, so postcopy_ram_ things run without the BQL? > > I'll check the postcopy_ram_* functions to see what's happening there. === 2024-07-23T17:11:25.934756Z qemu-kvm: vhost_user_postcopy_end: 161184:161213: BQL not taken 2024-07-23T17:11:25.934994Z qemu-kvm: vhost_user_postcopy_end: 161184:161213: BQL not taken 2024-07-23T17:11:25.935095Z qemu-kvm: vhost_user_postcopy_end: 161184:161213: BQL not taken === * postcopy_ram_listen_thread() does not take the BQL. Trying to see where to take it. Thank you. --- - Prasad
From: Prasad Pandit <pjp@fedoraproject.org> Hello, * virsh(1) offers multiple options to initiate Postcopy migration: 1) virsh migrate --postcopy --postcopy-after-precopy 2) virsh migrate --postcopy + virsh migrate-postcopy 3) virsh migrate --postcopy --timeout <N> --timeout-postcopy When Postcopy migration is invoked via method (2) or (3) above, the guest on the destination host seems to hang or get stuck sometimes. * During Postcopy migration, multiple threads are spawned on the destination host to start the guest and setup devices. One such thread starts vhost device via vhost_dev_start() function and another called fault_thread handles page faults in user space using kernel's userfaultfd(2) system. When fault_thread exits upon completion of Postcopy migration, it sends a 'postcopy_end' message to the vhost-user device. But sometimes 'postcopy_end' message is sent while vhost device is being setup via vhost_dev_start(). Thread-1 Thread-2 vhost_dev_start postcopy_ram_incoming_cleanup vhost_device_iotlb_miss postcopy_notify vhost_backend_update_device_iotlb vhost_user_postcopy_notifier vhost_user_send_device_iotlb_msg vhost_user_postcopy_end process_message_reply process_message_reply vhost_user_read vhost_user_read vhost_user_read_header vhost_user_read_header "Fail to update device iotlb" "Failed to receive reply to postcopy_end" This creates confusion when vhost device receives 'postcopy_end' message while it is still trying to update IOTLB entries. This seems to leave the guest in a stranded/hung state because fault_thread has exited saying Postcopy migration has ended, but vhost-device is probably still expecting updates. QEMU logs following errors on the destination host === ... qemu-kvm: vhost_user_read_header: 700871,700871: Failed to read msg header. Flags 0x0 instead of 0x5. qemu-kvm: vhost_device_iotlb_miss: 700871,700871: Fail to update device iotlb qemu-kvm: vhost_user_postcopy_end: 700871,700900: Failed to receive reply to postcopy_end qemu-kvm: vhost_user_read_header: 700871,700871: Failed to read msg header. Flags 0x0 instead of 0x5. qemu-kvm: vhost_device_iotlb_miss: 700871,700871: Fail to update device iotlb qemu-kvm: vhost_user_read_header: 700871,700871: Failed to read msg header. Flags 0x8 instead of 0x5. qemu-kvm: vhost_device_iotlb_miss: 700871,700871: Fail to update device iotlb qemu-kvm: vhost_user_read_header: 700871,700871: Failed to read msg header. Flags 0x16 instead of 0x5. qemu-kvm: vhost_device_iotlb_miss: 700871,700871: Fail to update device iotlb qemu-kvm: vhost_user_read_header: 700871,700871: Failed to read msg header. Flags 0x0 instead of 0x5. qemu-kvm: vhost_device_iotlb_miss: 700871,700871: Fail to update device iotlb === * Couple of patches here help to fix/handle these errors. Thank you. --- Prasad Pandit (2): vhost-user: add a write-read lock vhost: fail device start if iotlb update fails hw/virtio/vhost-user.c | 423 +++++++++++++++++++-------------- hw/virtio/vhost.c | 6 +- include/hw/virtio/vhost-user.h | 3 + 3 files changed, 259 insertions(+), 173 deletions(-) -- 2.45.2