Re: [RFC PATCH 1/3] migration/multifd: Move channels_ready semaphore
On Tue, Oct 10, 2023 at 06:43:05PM -0300, Fabiano Rosas wrote: > Peter Xu writes: > > > On Fri, Sep 22, 2023 at 11:53:17AM -0300, Fabiano Rosas wrote: > >> Commit d2026ee117 ("multifd: Fix the number of channels ready") moved > >> the "post" of channels_ready to the start of the multifd_send_thread() > >> loop and added a missing "wait" at multifd_send_sync_main(). While it > >> does work, the placement of the wait goes against what the rest of the > >> code does. > >> > >> The sequence at multifd_send_thread() is: > >> > >> qemu_sem_post(_send_state->channels_ready); > >> qemu_sem_wait(>sem); > >> > >> if (flags & MULTIFD_FLAG_SYNC) { > >> qemu_sem_post(>sem_sync); > >> } > >> > >> Which means that the sending thread makes itself available > >> (channels_ready) and waits for more work (sem). So the sequence in the > >> migration thread should be to check if any channel is available > >> (channels_ready), give it some work and set it off (sem): > >> > >> qemu_sem_wait(_send_state->channels_ready); > > > > Here it means we have at least 1 free send thread, then... > > > >> > >> qemu_sem_post(>sem); > > > > ... here we enqueue some work to the current thread (pointed by "i"), no > > matter it's free or not, as "i" may not always point to the free thread. > > > > Yes. Which means channels_ready is currently useless. Since I posted > this I realized that and have been working on a series to remove it > completely. > > ... I'm not opposed to "fixing" whatever needs to be fixed here as well, but > I think removing it makes sense. I'll try to focus on that and post a v2 > here. Happy to read it. > > >> if (flags & MULTIFD_FLAG_SYNC) { > >> qemu_sem_wait(>sem_sync); > >> } > > > > So I must confess I never fully digest how these sem/mutex/.. worked in > > multifd, since the 1st day it's introduced.. so please take below comment > > with a grain of salt.. > > We definitely need to clarify some things in the multifd > design. Specially if we're going to use it as the main migration > infrastructure moving forward. Exactly. > > I think what we lack is a design direction. I'm not really interested in > how things work currently, but in how they *should* work based on the > design. Unfortunately we can't ignore how old code works; normally old code has its reason to work like that. So the best way to go is trying to figure out exactly how it works with the author, unless reaching the consensus it was a design mistake after that conversation. The luckiest thing here is we have the author around (Juan). Let's discuss thoroughly with him to make sure nothing is overlooked. > > I'm confused about: > > 1) why channels_ready exists? Were we trying to do some lockstep > movement of: populate MultiFDPages -> release the sender thread -> move > to next channel -> wait for it to become ready -> repeat. If so, that > semaphore should be per-channel I think. > > (my future proposal will be to remove the channels_ready semaphore) > > 2) why do we need sem_sync? The SYNC flag makes sense, but why the > source needs to sync with itself when syncing with dst? > > (my proposal in this series is to rename sem_sync to sem_done and use it > to track sending completion) > > 3) why do we need to take the params lock? Shouldn't the semaphores > already ensure that only one of the main thread and the sender thread > will touch the params? The comment in multifd_send_pages says that we > don't take locks for the pages structure, but that seems pointeless to > me since we still lock the params structure. Heh, so I'm not the only one who is confused with all these. :) You reminded me of the days when I was reviewing the initial versions of multifd, since when I failed to understand the code.. It's great to start discussing this again. I'd say go ahead and propose your patches; I'll read them. > > > It seems to me that the current design allows >1 pending_job for a thread. > > Here the current code didn't do "wait(channels_ready)" because it doesn't > > need to - it simply always queue an MULTIFD_FLAG_SYNC pending job over the > > thread, and wait for it to run. > > > > From that POV I think I can understand why "wait(channels_ready)" is not > > needed here. But then I'm confused because we don't have a real QUEUE to > > put those requests; we simply apply this: > > > > multifd_send_sync_main(): > > p->flags |= MULTIFD_FLAG_SYNC; > > > > Even if this send thread can be busy handling a batch of pages and > > accessing p->flags. I think it can actually race with the send thread > > reading the flag at the exact same time: > > > > multifd_send_thread(): > > multifd_send_fill_packet(p); > > flags = p->flags; <-- here > > It doesn't race in reading due to the p->mutex lock. But it looks like > it could miss a newly set flag when it unlocks to start sending > (qio_channel_write*). Right. See my follow up email, I
Re: [RFC PATCH 1/3] migration/multifd: Move channels_ready semaphore
Peter Xu writes: > On Fri, Sep 22, 2023 at 11:53:17AM -0300, Fabiano Rosas wrote: >> Commit d2026ee117 ("multifd: Fix the number of channels ready") moved >> the "post" of channels_ready to the start of the multifd_send_thread() >> loop and added a missing "wait" at multifd_send_sync_main(). While it >> does work, the placement of the wait goes against what the rest of the >> code does. >> >> The sequence at multifd_send_thread() is: >> >> qemu_sem_post(_send_state->channels_ready); >> qemu_sem_wait(>sem); >> >> if (flags & MULTIFD_FLAG_SYNC) { >> qemu_sem_post(>sem_sync); >> } >> >> Which means that the sending thread makes itself available >> (channels_ready) and waits for more work (sem). So the sequence in the >> migration thread should be to check if any channel is available >> (channels_ready), give it some work and set it off (sem): >> >> qemu_sem_wait(_send_state->channels_ready); > > Here it means we have at least 1 free send thread, then... > >> >> qemu_sem_post(>sem); > > ... here we enqueue some work to the current thread (pointed by "i"), no > matter it's free or not, as "i" may not always point to the free thread. > Yes. Which means channels_ready is currently useless. Since I posted this I realized that and have been working on a series to remove it completely. ... I'm not opposed to "fixing" whatever needs to be fixed here as well, but I think removing it makes sense. I'll try to focus on that and post a v2 here. >> if (flags & MULTIFD_FLAG_SYNC) { >> qemu_sem_wait(>sem_sync); >> } > > So I must confess I never fully digest how these sem/mutex/.. worked in > multifd, since the 1st day it's introduced.. so please take below comment > with a grain of salt.. We definitely need to clarify some things in the multifd design. Specially if we're going to use it as the main migration infrastructure moving forward. I think what we lack is a design direction. I'm not really interested in how things work currently, but in how they *should* work based on the design. I'm confused about: 1) why channels_ready exists? Were we trying to do some lockstep movement of: populate MultiFDPages -> release the sender thread -> move to next channel -> wait for it to become ready -> repeat. If so, that semaphore should be per-channel I think. (my future proposal will be to remove the channels_ready semaphore) 2) why do we need sem_sync? The SYNC flag makes sense, but why the source needs to sync with itself when syncing with dst? (my proposal in this series is to rename sem_sync to sem_done and use it to track sending completion) 3) why do we need to take the params lock? Shouldn't the semaphores already ensure that only one of the main thread and the sender thread will touch the params? The comment in multifd_send_pages says that we don't take locks for the pages structure, but that seems pointeless to me since we still lock the params structure. > It seems to me that the current design allows >1 pending_job for a thread. > Here the current code didn't do "wait(channels_ready)" because it doesn't > need to - it simply always queue an MULTIFD_FLAG_SYNC pending job over the > thread, and wait for it to run. > > From that POV I think I can understand why "wait(channels_ready)" is not > needed here. But then I'm confused because we don't have a real QUEUE to > put those requests; we simply apply this: > > multifd_send_sync_main(): > p->flags |= MULTIFD_FLAG_SYNC; > > Even if this send thread can be busy handling a batch of pages and > accessing p->flags. I think it can actually race with the send thread > reading the flag at the exact same time: > > multifd_send_thread(): > multifd_send_fill_packet(p); > flags = p->flags; <-- here It doesn't race in reading due to the p->mutex lock. But it looks like it could miss a newly set flag when it unlocks to start sending (qio_channel_write*). > And whether it sees MULTIFD_FLAG_SYNC is unpredictable. If it sees it, > it'll post(sem_sync) in this round. If it doesn't see it, it'll > post(sem_sync) in the next round. In whatever way, we'll generate an empty > multifd packet to the wire I think, even though I don't know whether that's > needed at all... > > I'm not sure whether we should fix it in a more complete form, by not > sending that empty multifd packet at all? Because that only contains the > header without any real page inside, IIUC, so it seems to be a waste of > resource. Here what we want is only to kick sem_sync? > >> >> The reason there's no deadlock today is that the migration thread >> enqueues the SYNC packet right before the wait on channels_ready and >> we end up taking advantage of the out-of-order post to sem: >> >> ... >> qemu_sem_post(>sem); >> } >> for (i = 0; i < migrate_multifd_channels(); i++) { >> MultiFDSendParams *p = _send_state->params[i]; >> >>
Re: [RFC PATCH 1/3] migration/multifd: Move channels_ready semaphore
On Tue, Oct 10, 2023 at 05:00:37PM -0400, Peter Xu wrote: > On Fri, Sep 22, 2023 at 11:53:17AM -0300, Fabiano Rosas wrote: > > Commit d2026ee117 ("multifd: Fix the number of channels ready") moved > > the "post" of channels_ready to the start of the multifd_send_thread() > > loop and added a missing "wait" at multifd_send_sync_main(). While it > > does work, the placement of the wait goes against what the rest of the > > code does. > > > > The sequence at multifd_send_thread() is: > > > > qemu_sem_post(_send_state->channels_ready); > > qemu_sem_wait(>sem); > > > > if (flags & MULTIFD_FLAG_SYNC) { > > qemu_sem_post(>sem_sync); > > } > > > > Which means that the sending thread makes itself available > > (channels_ready) and waits for more work (sem). So the sequence in the > > migration thread should be to check if any channel is available > > (channels_ready), give it some work and set it off (sem): > > > > qemu_sem_wait(_send_state->channels_ready); > > Here it means we have at least 1 free send thread, then... > > > > > qemu_sem_post(>sem); > > ... here we enqueue some work to the current thread (pointed by "i"), no > matter it's free or not, as "i" may not always point to the free thread. > > > if (flags & MULTIFD_FLAG_SYNC) { > > qemu_sem_wait(>sem_sync); > > } > > So I must confess I never fully digest how these sem/mutex/.. worked in > multifd, since the 1st day it's introduced.. so please take below comment > with a grain of salt.. > > It seems to me that the current design allows >1 pending_job for a thread. > Here the current code didn't do "wait(channels_ready)" because it doesn't > need to - it simply always queue an MULTIFD_FLAG_SYNC pending job over the > thread, and wait for it to run. > > From that POV I think I can understand why "wait(channels_ready)" is not > needed here. But then I'm confused because we don't have a real QUEUE to > put those requests; we simply apply this: > > multifd_send_sync_main(): > p->flags |= MULTIFD_FLAG_SYNC; > > Even if this send thread can be busy handling a batch of pages and > accessing p->flags. I think it can actually race with the send thread > reading the flag at the exact same time: > > multifd_send_thread(): > multifd_send_fill_packet(p); > flags = p->flags; <-- here > > And whether it sees MULTIFD_FLAG_SYNC is unpredictable. If it sees it, > it'll post(sem_sync) in this round. If it doesn't see it, it'll > post(sem_sync) in the next round. In whatever way, we'll generate an empty > multifd packet to the wire I think, even though I don't know whether that's > needed at all... A correction: Since it's protected by p->mutex, I think we will only get an empty multifd packet when we have pending_jobs==2.. because then we'll see pending_job==2 with p->flags==SYNC, we send pages along with flags=SYNC to dest, after that we kick sem_sync on src, then we found another pending_jobs==1 even if p->flags will be zero. The next multifd packet will be only containing header (flags=0) and with no pages. > > I'm not sure whether we should fix it in a more complete form, by not > sending that empty multifd packet at all? Because that only contains the > header without any real page inside, IIUC, so it seems to be a waste of > resource. Here what we want is only to kick sem_sync? When thinking more about it, now I'm unsure whether sync is really working as expected now in general.. IIUC SYNC message is used to flush all pages from source to destination. We need that because we want to order the different versions of guest pages, making sure the new version of a page always arrives later than its old version, hence after all pages migrated we'll be sure all guest pages on dest will be the latest. Let's define "version X for page Y" as PyVx. Version 1 of page 2 is P2V1. So if without SYNC, a race can happen like this: sender 1 sender 2 receiver 1receiver 2 ---- send P1V1 ...P1 changed content.. queued again in sender 2... send P1V2 ...If we got unlucky on receiving order of P1 versions... receive P1V2 receive P1V1 So if receiver 1 got P1V1 after receiver 2 got P1V2, we'll ultimately have P1V1 on dst, which is an old data, causing data corrupt after migration. Now we have the SYNC packet, but would it always work? I'll discuss with the latest RAM_SAVE_FLAG_MULTIFD_FLUSH sync message: src mainsender 1 sender 2dst main receiver 1 receiver 2 -- -- send P1V1 send MULTIFD_FLUSH ...P1 changed.. queued again in sender 2... send P1V2
Re: [RFC PATCH 1/3] migration/multifd: Move channels_ready semaphore
On Fri, Sep 22, 2023 at 11:53:17AM -0300, Fabiano Rosas wrote: > Commit d2026ee117 ("multifd: Fix the number of channels ready") moved > the "post" of channels_ready to the start of the multifd_send_thread() > loop and added a missing "wait" at multifd_send_sync_main(). While it > does work, the placement of the wait goes against what the rest of the > code does. > > The sequence at multifd_send_thread() is: > > qemu_sem_post(_send_state->channels_ready); > qemu_sem_wait(>sem); > > if (flags & MULTIFD_FLAG_SYNC) { > qemu_sem_post(>sem_sync); > } > > Which means that the sending thread makes itself available > (channels_ready) and waits for more work (sem). So the sequence in the > migration thread should be to check if any channel is available > (channels_ready), give it some work and set it off (sem): > > qemu_sem_wait(_send_state->channels_ready); Here it means we have at least 1 free send thread, then... > > qemu_sem_post(>sem); ... here we enqueue some work to the current thread (pointed by "i"), no matter it's free or not, as "i" may not always point to the free thread. > if (flags & MULTIFD_FLAG_SYNC) { > qemu_sem_wait(>sem_sync); > } So I must confess I never fully digest how these sem/mutex/.. worked in multifd, since the 1st day it's introduced.. so please take below comment with a grain of salt.. It seems to me that the current design allows >1 pending_job for a thread. Here the current code didn't do "wait(channels_ready)" because it doesn't need to - it simply always queue an MULTIFD_FLAG_SYNC pending job over the thread, and wait for it to run. >From that POV I think I can understand why "wait(channels_ready)" is not needed here. But then I'm confused because we don't have a real QUEUE to put those requests; we simply apply this: multifd_send_sync_main(): p->flags |= MULTIFD_FLAG_SYNC; Even if this send thread can be busy handling a batch of pages and accessing p->flags. I think it can actually race with the send thread reading the flag at the exact same time: multifd_send_thread(): multifd_send_fill_packet(p); flags = p->flags; <-- here And whether it sees MULTIFD_FLAG_SYNC is unpredictable. If it sees it, it'll post(sem_sync) in this round. If it doesn't see it, it'll post(sem_sync) in the next round. In whatever way, we'll generate an empty multifd packet to the wire I think, even though I don't know whether that's needed at all... I'm not sure whether we should fix it in a more complete form, by not sending that empty multifd packet at all? Because that only contains the header without any real page inside, IIUC, so it seems to be a waste of resource. Here what we want is only to kick sem_sync? > > The reason there's no deadlock today is that the migration thread > enqueues the SYNC packet right before the wait on channels_ready and > we end up taking advantage of the out-of-order post to sem: > > ... > qemu_sem_post(>sem); > } > for (i = 0; i < migrate_multifd_channels(); i++) { > MultiFDSendParams *p = _send_state->params[i]; > > qemu_sem_wait(_send_state->channels_ready); > trace_multifd_send_sync_main_wait(p->id); > qemu_sem_wait(>sem_sync); > ... > > Move the channels_ready wait before the sem post to keep the sequence > consistent. Also fix the error path to post to channels_ready and > sem_sync in the correct order. > > Signed-off-by: Fabiano Rosas > --- > migration/multifd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/migration/multifd.c b/migration/multifd.c > index a7c7a947e3..d626740f2f 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -618,6 +618,7 @@ int multifd_send_sync_main(QEMUFile *f) > > trace_multifd_send_sync_main_signal(p->id); > > +qemu_sem_wait(_send_state->channels_ready); > qemu_mutex_lock(>mutex); > > if (p->quit) { > @@ -635,7 +636,6 @@ int multifd_send_sync_main(QEMUFile *f) > for (i = 0; i < migrate_multifd_channels(); i++) { > MultiFDSendParams *p = _send_state->params[i]; > > -qemu_sem_wait(_send_state->channels_ready); > trace_multifd_send_sync_main_wait(p->id); > qemu_sem_wait(>sem_sync); > > @@ -763,8 +763,8 @@ out: > * who pay attention to me. > */ > if (ret != 0) { > -qemu_sem_post(>sem_sync); > qemu_sem_post(_send_state->channels_ready); > +qemu_sem_post(>sem_sync); I'm not sure why such movement will have a difference; afaiu on the semaphore semantics, post() to two sems don't matter on order? > } > > qemu_mutex_lock(>mutex); > -- > 2.35.3 > -- Peter Xu
Re: [RFC PATCH 1/3] migration/multifd: Move channels_ready semaphore
Elena Ufimtseva writes: > On Fri, Sep 22, 2023 at 11:53:17AM -0300, Fabiano Rosas wrote: >> Commit d2026ee117 ("multifd: Fix the number of channels ready") moved >> the "post" of channels_ready to the start of the multifd_send_thread() >> loop and added a missing "wait" at multifd_send_sync_main(). While it >> does work, the placement of the wait goes against what the rest of the >> code does. >> >> The sequence at multifd_send_thread() is: >> >> qemu_sem_post(_send_state->channels_ready); >> qemu_sem_wait(>sem); >> >> if (flags & MULTIFD_FLAG_SYNC) { >> qemu_sem_post(>sem_sync); >> } >> >> Which means that the sending thread makes itself available >> (channels_ready) and waits for more work (sem). So the sequence in the >> migration thread should be to check if any channel is available >> (channels_ready), give it some work and set it off (sem): >> >> qemu_sem_wait(_send_state->channels_ready); >> >> qemu_sem_post(>sem); >> if (flags & MULTIFD_FLAG_SYNC) { >> qemu_sem_wait(>sem_sync); >> } >> >> The reason there's no deadlock today is that the migration thread >> enqueues the SYNC packet right before the wait on channels_ready and >> we end up taking advantage of the out-of-order post to sem: >> >> ... >> qemu_sem_post(>sem); >> } >> for (i = 0; i < migrate_multifd_channels(); i++) { >> MultiFDSendParams *p = _send_state->params[i]; >> >> qemu_sem_wait(_send_state->channels_ready); >> trace_multifd_send_sync_main_wait(p->id); >> qemu_sem_wait(>sem_sync); >> ... >> >> Move the channels_ready wait before the sem post to keep the sequence >> consistent. Also fix the error path to post to channels_ready and >> sem_sync in the correct order. >> > > Thank you Fabiano, > > Your solution is more complete. I also had in mind getting rid of > sem_sync. > > With your second patch, this one could be merged with it? > >> Signed-off-by: Fabiano Rosas >> --- >> migration/multifd.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/migration/multifd.c b/migration/multifd.c >> index a7c7a947e3..d626740f2f 100644 >> --- a/migration/multifd.c >> +++ b/migration/multifd.c >> @@ -618,6 +618,7 @@ int multifd_send_sync_main(QEMUFile *f) >> >> trace_multifd_send_sync_main_signal(p->id); >> >> +qemu_sem_wait(_send_state->channels_ready); >> qemu_mutex_lock(>mutex); >> >> if (p->quit) { >> @@ -635,7 +636,6 @@ int multifd_send_sync_main(QEMUFile *f) >> for (i = 0; i < migrate_multifd_channels(); i++) { >> MultiFDSendParams *p = _send_state->params[i]; >> >> -qemu_sem_wait(_send_state->channels_ready); >> trace_multifd_send_sync_main_wait(p->id); >> qemu_sem_wait(>sem_sync); >> >> @@ -763,8 +763,8 @@ out: >> * who pay attention to me. >> */ >> if (ret != 0) { >> -qemu_sem_post(>sem_sync); >> qemu_sem_post(_send_state->channels_ready); >> +qemu_sem_post(>sem_sync); > > Can this thread in this error case be woken up again between > these two qemu_sem_posts? > I see in other places p->quit is set to true before it. > Or maybe it should one more patch to make these consistent > as well. That's a good point. There's clearly something going on here if we need a 'running', a 'quit' and a 'exiting' flag. The tls code uses quit as a signal in one direction while the regular multifd path uses it in another. I'll give it some more thought. Thanks
Re: [RFC PATCH 1/3] migration/multifd: Move channels_ready semaphore
On Fri, Sep 22, 2023 at 11:53:17AM -0300, Fabiano Rosas wrote: > Commit d2026ee117 ("multifd: Fix the number of channels ready") moved > the "post" of channels_ready to the start of the multifd_send_thread() > loop and added a missing "wait" at multifd_send_sync_main(). While it > does work, the placement of the wait goes against what the rest of the > code does. > > The sequence at multifd_send_thread() is: > > qemu_sem_post(_send_state->channels_ready); > qemu_sem_wait(>sem); > > if (flags & MULTIFD_FLAG_SYNC) { > qemu_sem_post(>sem_sync); > } > > Which means that the sending thread makes itself available > (channels_ready) and waits for more work (sem). So the sequence in the > migration thread should be to check if any channel is available > (channels_ready), give it some work and set it off (sem): > > qemu_sem_wait(_send_state->channels_ready); > > qemu_sem_post(>sem); > if (flags & MULTIFD_FLAG_SYNC) { > qemu_sem_wait(>sem_sync); > } > > The reason there's no deadlock today is that the migration thread > enqueues the SYNC packet right before the wait on channels_ready and > we end up taking advantage of the out-of-order post to sem: > > ... > qemu_sem_post(>sem); > } > for (i = 0; i < migrate_multifd_channels(); i++) { > MultiFDSendParams *p = _send_state->params[i]; > > qemu_sem_wait(_send_state->channels_ready); > trace_multifd_send_sync_main_wait(p->id); > qemu_sem_wait(>sem_sync); > ... > > Move the channels_ready wait before the sem post to keep the sequence > consistent. Also fix the error path to post to channels_ready and > sem_sync in the correct order. > Thank you Fabiano, Your solution is more complete. I also had in mind getting rid of sem_sync. With your second patch, this one could be merged with it? > Signed-off-by: Fabiano Rosas > --- > migration/multifd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/migration/multifd.c b/migration/multifd.c > index a7c7a947e3..d626740f2f 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -618,6 +618,7 @@ int multifd_send_sync_main(QEMUFile *f) > > trace_multifd_send_sync_main_signal(p->id); > > +qemu_sem_wait(_send_state->channels_ready); > qemu_mutex_lock(>mutex); > > if (p->quit) { > @@ -635,7 +636,6 @@ int multifd_send_sync_main(QEMUFile *f) > for (i = 0; i < migrate_multifd_channels(); i++) { > MultiFDSendParams *p = _send_state->params[i]; > > -qemu_sem_wait(_send_state->channels_ready); > trace_multifd_send_sync_main_wait(p->id); > qemu_sem_wait(>sem_sync); > > @@ -763,8 +763,8 @@ out: > * who pay attention to me. > */ > if (ret != 0) { > -qemu_sem_post(>sem_sync); > qemu_sem_post(_send_state->channels_ready); > +qemu_sem_post(>sem_sync); Can this thread in this error case be woken up again between these two qemu_sem_posts? I see in other places p->quit is set to true before it. Or maybe it should one more patch to make these consistent as well. Elena U. > } > > qemu_mutex_lock(>mutex); > -- > 2.35.3 >