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<---
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..
--
Peter Xu
