[v2,26/39] tests/qtest: migration-test: Make sure QEMU process "to" exited after migration is canceled

Commit Message

Bin Meng Sept. 20, 2022, 10:31 a.m. UTC

From: Xuzhou Cheng <[e-mail]>

Make sure QEMU process "to" exited before launching another target
for migration in the test_multifd_tcp_cancel case.

Signed-off-by: Xuzhou Cheng <[e-mail]>
Signed-off-by: Bin Meng <[e-mail]>
Reviewed-by: Marc-André Lureau <[e-mail]>
---

Changes in v2:
- Change to a busy wait after migration is canceled

 tests/qtest/migration-test.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Dr. David Alan Gilbert Sept. 21, 2022, 4:29 p.m. UTC | #1

* Bin Meng ([e-mail]) wrote:
> From: Xuzhou Cheng <[e-mail]>
> 
> Make sure QEMU process "to" exited before launching another target
> for migration in the test_multifd_tcp_cancel case.
> 
> Signed-off-by: Xuzhou Cheng <[e-mail]>
> Signed-off-by: Bin Meng <[e-mail]>
> Reviewed-by: Marc-André Lureau <[e-mail]>

Hmm you might want to put a small usleep in that loop; otherwise
it'll burn CPU.

There is a slim risk with this that another, entirely unrelated, process 
will start up with the same PID between the end of migrate_cancel
and then you'll be spinning on it rather than the 'to' qemu.

I wonder if there's a better way to check for it dieing; e.g. an error
on it's qmp interface or something?

Dave

> ---
> 
> Changes in v2:
> - Change to a busy wait after migration is canceled
> 
>  tests/qtest/migration-test.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index c87afad9e8..aedd9ddb72 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -2133,6 +2133,10 @@ static void test_multifd_tcp_cancel(void)
>      wait_for_migration_pass(from);
>  
>      migrate_cancel(from);
> +    /* Make sure QEMU process "to" exited */
> +    while (qtest_probe_child(to)) {
> +        ;
> +    }
>  
>      args = (MigrateStart){
>          .only_target = true,
> -- 
> 2.34.1
>

Daniel P. Berrangé Sept. 21, 2022, 4:50 p.m. UTC | #2

On Wed, Sep 21, 2022 at 05:29:55PM +0100, Dr. David Alan Gilbert wrote:
> * Bin Meng ([e-mail]) wrote:
> > From: Xuzhou Cheng <[e-mail]>
> > 
> > Make sure QEMU process "to" exited before launching another target
> > for migration in the test_multifd_tcp_cancel case.
> > 
> > Signed-off-by: Xuzhou Cheng <[e-mail]>
> > Signed-off-by: Bin Meng <[e-mail]>
> > Reviewed-by: Marc-André Lureau <[e-mail]>
> 
> Hmm you might want to put a small usleep in that loop; otherwise
> it'll burn CPU.
> 
> There is a slim risk with this that another, entirely unrelated, process 
> will start up with the same PID between the end of migrate_cancel
> and then you'll be spinning on it rather than the 'to' qemu.
> 
> I wonder if there's a better way to check for it dieing; e.g. an error
> on it's qmp interface or something?

Both the qtest and qmp sockets should give EOF. So if there's an API
that can call g_poll() on the FD with POLL_HUP event, it would be the
reliable way to detect it, without busy-looping.

With regards,
Daniel

Marc-André Lureau Sept. 21, 2022, 9:54 p.m. UTC | #3

Hi

On Tue, Sep 20, 2022 at 3:18 PM Bin Meng <[e-mail]> wrote:

> From: Xuzhou Cheng <[e-mail]>
>
> Make sure QEMU process "to" exited before launching another target
> for migration in the test_multifd_tcp_cancel case.
>
> Signed-off-by: Xuzhou Cheng <[e-mail]>
> Signed-off-by: Bin Meng <[e-mail]>
> Reviewed-by: Marc-André Lureau <[e-mail]>
>

fwiw, I didn't r-b the version with a busy wait
(
https://patchew.org/QEMU/[e-mail]/[e-mail]/
)

---
>
> Changes in v2:
> - Change to a busy wait after migration is canceled
>
>  tests/qtest/migration-test.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index c87afad9e8..aedd9ddb72 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -2133,6 +2133,10 @@ static void test_multifd_tcp_cancel(void)
>      wait_for_migration_pass(from);
>
>      migrate_cancel(from);
> +    /* Make sure QEMU process "to" exited */
> +    while (qtest_probe_child(to)) {
> +        ;
> +    }
>
>      args = (MigrateStart){
>          .only_target = true,
> --
> 2.34.1
>
>
>

Bin Meng Sept. 22, 2022, 3:29 a.m. UTC | #4

On Thu, Sep 22, 2022 at 5:54 AM Marc-André Lureau
<[e-mail]> wrote:
>
> Hi
>
> On Tue, Sep 20, 2022 at 3:18 PM Bin Meng <[e-mail]> wrote:
>>
>> From: Xuzhou Cheng <[e-mail]>
>>
>> Make sure QEMU process "to" exited before launching another target
>> for migration in the test_multifd_tcp_cancel case.
>>
>> Signed-off-by: Xuzhou Cheng <[e-mail]>
>> Signed-off-by: Bin Meng <[e-mail]>
>> Reviewed-by: Marc-André Lureau <[e-mail]>
>
>
> fwiw, I didn't r-b the version with a busy wait
> (https://patchew.org/QEMU/[e-mail]/[e-mail]/)
>

My mistake. The R-B tag was added before I changed the implementation
and I forgot to remove the tag.

Regards,
Bin

Patch

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index c87afad9e8..aedd9ddb72 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2133,6 +2133,10 @@  static void test_multifd_tcp_cancel(void)
     wait_for_migration_pass(from);
 
     migrate_cancel(from);
+    /* Make sure QEMU process "to" exited */
+    while (qtest_probe_child(to)) {
+        ;
+    }
 
     args = (MigrateStart){
         .only_target = true,