From: Alistair Popple <apopple@nvidia.com>
To: John Hubbard <jhubbard@nvidia.com>
Cc: linux-mm@kvack.org, akpm@linux-foundation.org,
Peter Xu <peterx@redhat.com>, Nadav Amit <nadav.amit@gmail.com>,
huang ying <huang.ying.caritas@gmail.com>,
LKML <linux-kernel@vger.kernel.org>,
"Sierra Guiza, Alejandro (Alex)" <alex.sierra@amd.com>,
Felix Kuehling <Felix.Kuehling@amd.com>,
Jason Gunthorpe <jgg@nvidia.com>,
David Hildenbrand <david@redhat.com>,
Ralph Campbell <rcampbell@nvidia.com>,
Matthew Wilcox <willy@infradead.org>,
Karol Herbst <kherbst@redhat.com>, Lyude Paul <lyude@redhat.com>,
Ben Skeggs <bskeggs@redhat.com>,
Logan Gunthorpe <logang@deltatee.com>,
paulus@ozlabs.org, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v4 4/4] selftests/hmm-tests: Add test for dirty bits
Date: Tue, 13 Sep 2022 12:33:40 +1000 [thread overview]
Message-ID: <87czc0m1wd.fsf@nvdebian.thelocal> (raw)
In-Reply-To: <90a0441e-47ad-007f-06c1-b30e5f7bb692@nvidia.com>
John Hubbard <jhubbard@nvidia.com> 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,
next prev parent reply other threads:[~2022-09-13 2:42 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-02 0:35 [PATCH v4 1/4] mm/migrate_device.c: Flush TLB while holding PTL Alistair Popple
2022-09-02 0:35 ` [PATCH v4 2/4] mm/migrate_device.c: Add missing flush_cache_page() Alistair Popple
2022-09-02 6:51 ` David Hildenbrand
2022-09-02 20:39 ` Peter Xu
2022-09-02 0:35 ` [PATCH v4 3/4] mm/migrate_device.c: Copy pte dirty bit to page Alistair Popple
2022-09-02 6:53 ` David Hildenbrand
2022-09-02 0:35 ` [PATCH v4 4/4] selftests/hmm-tests: Add test for dirty bits Alistair Popple
2022-09-05 0:41 ` John Hubbard
2022-09-07 11:13 ` Alistair Popple
2022-09-07 21:48 ` John Hubbard
2022-09-13 2:33 ` Alistair Popple [this message]
2022-09-02 6:51 ` [PATCH v4 1/4] mm/migrate_device.c: Flush TLB while holding PTL David Hildenbrand
2022-09-02 20:35 ` Peter Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87czc0m1wd.fsf@nvdebian.thelocal \
--to=apopple@nvidia.com \
--cc=Felix.Kuehling@amd.com \
--cc=akpm@linux-foundation.org \
--cc=alex.sierra@amd.com \
--cc=bskeggs@redhat.com \
--cc=david@redhat.com \
--cc=huang.ying.caritas@gmail.com \
--cc=jgg@nvidia.com \
--cc=jhubbard@nvidia.com \
--cc=kherbst@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=logang@deltatee.com \
--cc=lyude@redhat.com \
--cc=nadav.amit@gmail.com \
--cc=paulus@ozlabs.org \
--cc=peterx@redhat.com \
--cc=rcampbell@nvidia.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).