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

[Alistair Popple <apopple@nvidia.com>]

John Hubbard <jhubbard@nvidia.com> writes:

> On 9/1/22 17:35, Alistair Popple wrote:

[...]

>> +/*
>> + * Try and migrate a dirty page that has previously been swapped to disk. This
>> + * checks that we don't loose dirty bits.
>
> s/loose/lose/

Thanks.

>> + */
>> +TEST_F(hmm, migrate_dirty_page)
>> +{
>> +	struct hmm_buffer *buffer;
>> +	unsigned long npages;
>> +	unsigned long size;
>> +	unsigned long i;
>> +	int *ptr;
>> +	int tmp = 0;
>> +
>> +	npages = ALIGN(HMM_BUFFER_SIZE, self->page_size) >> self->page_shift;
>> +	ASSERT_NE(npages, 0);
>> +	size = npages << self->page_shift;
>> +
>> +	buffer = malloc(sizeof(*buffer));
>> +	ASSERT_NE(buffer, NULL);
>> +
>> +	buffer->fd = -1;
>> +	buffer->size = size;
>> +	buffer->mirror = malloc(size);
>> +	ASSERT_NE(buffer->mirror, NULL);
>> +
>> +	ASSERT_EQ(setup_cgroup(), 0);
>> +
>> +	buffer->ptr = mmap(NULL, size,
>> +			   PROT_READ | PROT_WRITE,
>> +			   MAP_PRIVATE | MAP_ANONYMOUS,
>> +			   buffer->fd, 0);
>> +	ASSERT_NE(buffer->ptr, MAP_FAILED);
>> +
>> +	/* Initialize buffer in system memory. */
>> +	for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
>> +		ptr[i] = 0;
>> +
>> +	ASSERT_FALSE(write_cgroup_param(cgroup, "memory.reclaim", 1UL<<30));
>> +
>> +	/* Fault pages back in from swap as clean pages */
>> +	for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
>> +		tmp += ptr[i];
>> +
>> +	/* Dirty the pte */
>> +	for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
>> +		ptr[i] = i;
>> +
>> +	/*
>> +	 * 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.

I'm not really sure what to do about it though. It's likely the fix for
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.

>> +
>> +	ASSERT_FALSE(write_cgroup_param(cgroup, "memory.reclaim", 1UL<<30));
>> +
>> +	/* Check we still see the updated data after restoring from swap. */
>> +	for (i = 0, ptr = buffer->ptr; i < size / sizeof(*ptr); ++i)
>> +		ASSERT_EQ(ptr[i], i);
>> +
>> +	hmm_buffer_free(buffer);
>> +	destroy_cgroup();
>> +}
>> +
>>  /*
>>   * Read anonymous memory multiple times.
>>   */
>
> thanks,