[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 20/20] tests: Add postcopy preempt test
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PATCH 20/20] tests: Add postcopy preempt test |
Date: |
Tue, 1 Mar 2022 17:00:15 +0000 |
User-agent: |
Mutt/2.1.5 (2021-12-30) |
* Peter Xu (peterx@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 (peterx@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 / dgilbert@redhat.com / Manchester, UK