* Peter Xu (pet...@redhat.com) wrote: > On Wed, Feb 23, 2022 at 03:50:24PM +0800, Peter Xu wrote: > > On Tue, Feb 22, 2022 at 12:51:59PM +0000, Dr. David Alan Gilbert wrote: > > > * Peter Xu (pet...@redhat.com) wrote: > > > > Two tests are added: a normal postcopy preempt test, and a recovery > > > > test. > > > > > > Yes, this is difficult; without hugepages the tests are limited; did you > > > see if this test actually causes pages to go down the fast path? > > > > I didn't observe the test case explicitly, but I did observe in my own test > > that I ran that it goes with the fast path, or I can't get a huge speed up. > > > > Meanwhile my own test is only using 2M huge pages, and I can observe > > interruptions of huge page sendings frequently. > > > > But yeah let me try to capture something in this test too, at least to make > > sure the standalone socket is being used. Covering of huge pages might be > > doable but obviously requires host privileges, so I'll leave that for later. > > When I tried to observe the test case today, I found that the preempt new > tests are all running with the new channels, however funnily I found the > original vanilla test is using it too! > > Looked into it, that's because the MigrateStart* pointer is freed in > test_migrate_start() but the test referenced it after that... so it's a > use-after-free bug in the test code. I need to squash this: > > ---8<--- > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > index 5053b40589..09a9ce4401 100644 > --- a/tests/qtest/migration-test.c > +++ b/tests/qtest/migration-test.c > @@ -664,6 +664,8 @@ static int migrate_postcopy_prepare(QTestState **from_ptr, > MigrateStart *args) > { > g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); > + /* NOTE: args will be freed in test_migrate_start(), cache it */ > + bool postcopy_preempt = args->postcopy_preempt; > QTestState *from, *to; > > if (test_migrate_start(&from, &to, uri, args)) { > @@ -674,7 +676,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr, > migrate_set_capability(to, "postcopy-ram", true); > migrate_set_capability(to, "postcopy-blocktime", true); > > - if (args->postcopy_preempt) { > + if (postcopy_preempt) { > migrate_set_capability(from, "postcopy-preempt", true); > migrate_set_capability(to, "postcopy-preempt", true); > } > ---8<---
Ah OK, yes I guess that's needed. > That's tricky, and we could have done something better.. E.g., we could > pass in the MigrateStart** into test_migrate_start() so it can clear it > when free, that's not silent use-after-free but crashing, which is better > in this case. > > I feel lucky I tried.. It could at least do with a comment on test_migrate_start? Dave > > -- > Peter Xu > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK