Messages in this thread Patch in this message | | | Date | Thu, 11 Jan 2024 09:51:47 -0800 | Subject | Re: [PATCH v4 4/4] selftests/mm: add tests for HWPOISON hugetlbfs read | From | Sidhartha Kumar <> |
| |
On 1/11/24 9:34 AM, Jiaqi Yan wrote: > On Thu, Jan 11, 2024 at 12:48 AM Muhammad Usama Anjum > <usama.anjum@collabora.com> wrote: >> >> On 1/11/24 7:32 AM, Sidhartha Kumar wrote: >>> On 1/10/24 2:15 AM, Muhammad Usama Anjum wrote: >>>> On 1/10/24 11:49 AM, Muhammad Usama Anjum wrote: >>>>> On 1/6/24 2:13 AM, Jiaqi Yan wrote: >>>>>> On Thu, Jan 4, 2024 at 10:27 PM Muhammad Usama Anjum >>>>>> <usama.anjum@collabora.com> wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> I'm trying to convert this test to TAP as I think the failures >>>>>>> sometimes go >>>>>>> unnoticed on CI systems if we only depend on the return value of the >>>>>>> application. I've enabled the following configurations which aren't >>>>>>> already >>>>>>> present in tools/testing/selftests/mm/config: >>>>>>> CONFIG_MEMORY_FAILURE=y >>>>>>> CONFIG_HWPOISON_INJECT=m >>>>>>> >>>>>>> I'll send a patch to add these configs later. Right now I'm trying to >>>>>>> investigate the failure when we are trying to inject the poison page by >>>>>>> madvise(MADV_HWPOISON). I'm getting device busy every single time. The >>>>>>> test >>>>>>> fails as it doesn't expect any business for the hugetlb memory. I'm not >>>>>>> sure if the poison handling code has issues or test isn't robust enough. >>>>>>> >>>>>>> ./hugetlb-read-hwpoison >>>>>>> Write/read chunk size=0x800 >>>>>>> ... HugeTLB read regression test... >>>>>>> ... ... expect to read 0x200000 bytes of data in total >>>>>>> ... ... actually read 0x200000 bytes of data in total >>>>>>> ... HugeTLB read regression test...TEST_PASSED >>>>>>> ... HugeTLB read HWPOISON test... >>>>>>> [ 9.280854] Injecting memory failure for pfn 0x102f01 at process >>>>>>> virtual >>>>>>> address 0x7f28ec101000 >>>>>>> [ 9.282029] Memory failure: 0x102f01: huge page still referenced by >>>>>>> 511 >>>>>>> users >>>>>>> [ 9.282987] Memory failure: 0x102f01: recovery action for huge >>>>>>> page: Failed >>>>>>> ... !!! MADV_HWPOISON failed: Device or resource busy >>>>>>> ... HugeTLB read HWPOISON test...TEST_FAILED >>>>>>> >>>>>>> I'm testing on v6.7-rc8. Not sure if this was working previously or not. >>>>>> >>>>>> Thanks for reporting this, Usama! >>>>>> >>>>>> I am also able to repro MADV_HWPOISON failure at "501a06fe8e4c >>>>>> (akpm/mm-stable, mm-stable) zswap: memcontrol: implement zswap >>>>>> writeback disabling." >>>>>> >>>>>> Then I checked out the earliest commit "ba91e7e5d15a (HEAD -> Base) >>>>>> selftests/mm: add tests for HWPOISON hugetlbfs read". The >>>>>> MADV_HWPOISON injection works and and the test passes: >>>>>> >>>>>> ... HugeTLB read HWPOISON test... >>>>>> ... ... expect to read 0x101000 bytes of data in total >>>>>> ... !!! read failed: Input/output error >>>>>> ... ... actually read 0x101000 bytes of data in total >>>>>> ... HugeTLB read HWPOISON test...TEST_PASSED >>>>>> ... HugeTLB seek then read HWPOISON test... >>>>>> ... ... init val=4 with offset=0x102000 >>>>>> ... ... expect to read 0xfe000 bytes of data in total >>>>>> ... ... actually read 0xfe000 bytes of data in total >>>>>> ... HugeTLB seek then read HWPOISON test...TEST_PASSED >>>>>> ... >>>>>> >>>>>> [ 2109.209225] Injecting memory failure for pfn 0x3190d01 at process >>>>>> virtual address 0x7f75e3101000 >>>>>> [ 2109.209438] Memory failure: 0x3190d01: recovery action for huge >>>>>> page: Recovered >>>>>> ... >>>>>> >>>>>> I think something in between broken MADV_HWPOISON on hugetlbfs, and we >>>>>> should be able to figure it out via bisection (and of course by >>>>>> reading delta commits between them, probably related to page >>>>>> refcount). >>>>> Thank you for this information. >>>>> >>>>>> >>>>>> That being said, I will be on vacation from tomorrow until the end of >>>>>> next week. So I will get back to this after next weekend. Meanwhile if >>>>>> you want to go ahead and bisect the problematic commit, that will be >>>>>> very much appreciated. >>>>> I'll try to bisect and post here if I find something. >>>> Found the culprit commit by bisection: >>>> >>>> a08c7193e4f18dc8508f2d07d0de2c5b94cb39a3 >>>> mm/filemap: remove hugetlb special casing in filemap.c > > Thanks Usama! > >>>> >>>> hugetlb-read-hwpoison started failing from this patch. I've added the >>>> author of this patch to this bug report. >>>> >>> Hi Usama, >>> >>> Thanks for pointing this out. After debugging, the below diff seems to fix >>> the issue and allows the tests to pass again. Could you test it on your >>> configuration as well just to confirm. >>> >>> Thanks, >>> Sidhartha >>> >>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c >>> index 36132c9125f9..3a248e4f7e93 100644 >>> --- a/fs/hugetlbfs/inode.c >>> +++ b/fs/hugetlbfs/inode.c >>> @@ -340,7 +340,7 @@ static ssize_t hugetlbfs_read_iter(struct kiocb *iocb, >>> struct iov_iter *to) >>> } else { >>> folio_unlock(folio); >>> >>> - if (!folio_test_has_hwpoisoned(folio)) >>> + if (!folio_test_hwpoison(folio)) > > Sidhartha, just curious why this change is needed? Does > PageHasHWPoisoned change after commit > "a08c7193e4f18dc8508f2d07d0de2c5b94cb39a3"? >
No its not an issue PageHasHWPoisoned(), the original code is testing for the wrong flag and I realized that has_hwpoison and hwpoison are two different flags. The memory-failure code calls folio_test_set_hwpoison() to set the hwpoison flag and does not set the has_hwpoison flag. When debugging, I realized this if statement was never true despite the code hitting folio_test_set_hwpoison(). Now we are testing the correct flag.
From page-flags.h
#ifdef CONFIG_MEMORY_FAILURE PG_hwpoison, /* hardware poisoned page. Don't touch */ #endif
folio_test_hwpoison() checks this flag ^^^
/* At least one page in this folio has the hwpoison flag set */ PG_has_hwpoisoned = PG_error,
while folio_test_has_hwpoisoned() checks this flag ^^^
Thanks, Sidhartha
>>> want = nr; >>> else { >>> /* >>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >>> index d8c853b35dbb..87f6bf7d8bc1 100644 >>> --- a/mm/memory-failure.c >>> +++ b/mm/memory-failure.c >>> @@ -973,7 +973,7 @@ struct page_state { >>> static bool has_extra_refcount(struct page_state *ps, struct page *p, >>> bool extra_pins) >>> { >>> - int count = page_count(p) - 1; >>> + int count = page_count(p) - folio_nr_pages(page_folio(p)); >>> >>> if (extra_pins) >>> count -= 1; >>> >> Tested the patch, it fixes the test. Please send this patch. >> >> Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com> >> >> -- >> BR, >> Muhammad Usama Anjum
| |