John Hubbard <jhubbard@xxxxxxxxxx> writes: > 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". True. Unfortunately our test framework needs some love as I don't think it's possible to return a result of "invalid" or "waived". We can skip a test though, so that might be the best option here. >> 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. Fair enough, I think what you're asking for is a higher level test that doesn't rely on implementation side-effects. I wrote this test mostly to discover if we could hit problems with the current implementation hence why it's a bit messy. But I think I can fix this up without relying on implementation side-effects - really I just want to confirm that at least some pages got swapped to disk which I can do via looking at /proc/self/pagemap. - Alistair > thanks,
