On 9/7/22 04:13, Alistair Popple wrote:
+ /*
+ * Attempt to migrate memory to device, which should fail because
+ * hopefully some pages are backed by swap storage.
+ */
+ ASSERT_TRUE(hmm_migrate_sys_to_dev(self->fd, buffer, npages));
Are you really sure that you want to assert on that? Because doing so
guarantees a test failure if and when we every upgrade the kernel to
be able to migrate swap-backed pages. And I seem to recall that this
current inability to migrate swap-backed pages is considered a flaw
to be fixed, right?
Right, that's a good point. I was using failure (ASSERT_TRUE) here as a
way of detecting that at least some pages are swap-backed, because if no
pages end up being swap-backed the test is invalid.
Yes. But "invalid" or "waived" is a much different test result than
"failed".
I'm not really sure what to do about it though. It's likely the fix for
Remove the assert. If the test framework allows and you prefer, you
can print a warning.
swap-backed migration may make this bug impossible to hit anyway,
because the obvious fix is to just drop the pages from the swapcache
during migration which would force writeback during subsequent reclaim.
So I'm inclined to leave this here even if it only serves to remind us
about it when we do fix migration of swap-backed pages, because we will
of course run hmm-tests before submitting that fix :-) We can then
either fix the test or drop it if we think it's no longer possible to
hit.
Oh no no no, please. This is not how to do tests. If you want a TODO
list somewhere, there are other ways. But tests that require maintenance
when you change something are an anti-pattern.
thanks,
--
John Hubbard
NVIDIA
