Re: [PATCH v4 4/4] selftests/hmm-tests: Add test for dirty bits

[John Hubbard <jhubbard@nvidia.com>]

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