| Message ID | 20230715031729.2420338-5-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few fixup and cleanup patches for memory-failure | expand |
On Sat, Jul 15, 2023 at 11:17:29AM +0800, Miaohe Lin wrote: > Memory failure is not interested in logically offlined page. Skip this > type of pages. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/memory-failure.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 42e63b0ab5f7..ed79b69837de 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -1559,7 +1559,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn, > * Here we are interested only in user-mapped pages, so skip any > * other types of pages. > */ > - if (PageReserved(p) || PageSlab(p) || PageTable(p)) > + if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p)) hwpoison_user_mappings() is called after some checks are done, so I'm not sure that it's the right place to check PageOffline(). We could check it before setting PageHWPoison() as we do at the beginning of memory_failure() around pfn_to_online_page(). Does it make sense? Thanks, Naoya Horiguchi > return true; > if (!(PageLRU(hpage) || PageHuge(p))) > return true; > @@ -2513,7 +2513,8 @@ int unpoison_memory(unsigned long pfn) > goto unlock_mutex; > } > > - if (folio_test_slab(folio) || PageTable(&folio->page) || folio_test_reserved(folio)) > + if (folio_test_slab(folio) || PageTable(&folio->page) || > + folio_test_reserved(folio) || PageOffline(&folio->page)) > goto unlock_mutex; > > /* > -- > 2.33.0 > > >
On 2023/7/20 9:09, Naoya Horiguchi wrote: > On Sat, Jul 15, 2023 at 11:17:29AM +0800, Miaohe Lin wrote: >> Memory failure is not interested in logically offlined page. Skip this >> type of pages. >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/memory-failure.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >> index 42e63b0ab5f7..ed79b69837de 100644 >> --- a/mm/memory-failure.c >> +++ b/mm/memory-failure.c >> @@ -1559,7 +1559,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn, >> * Here we are interested only in user-mapped pages, so skip any >> * other types of pages. >> */ >> - if (PageReserved(p) || PageSlab(p) || PageTable(p)) >> + if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p)) > > hwpoison_user_mappings() is called after some checks are done, so I'm not > sure that it's the right place to check PageOffline(). hwpoison_user_mappings() is called after the "if (!PageLRU(p) && !PageWriteback(p))" check in memory_failure(). So the page can't also be PageReserved(p) or PageSlab(p) or PageTable(p) here? I think the check here just wants to make things clear that only user-mapped pages are interested. Or am I miss something? Thanks Naoya.
On Thu, Jul 20, 2023 at 04:42:04PM +0800, Miaohe Lin wrote: > On 2023/7/20 9:09, Naoya Horiguchi wrote: > > On Sat, Jul 15, 2023 at 11:17:29AM +0800, Miaohe Lin wrote: > >> Memory failure is not interested in logically offlined page. Skip this > >> type of pages. > >> > >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > >> --- > >> mm/memory-failure.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c > >> index 42e63b0ab5f7..ed79b69837de 100644 > >> --- a/mm/memory-failure.c > >> +++ b/mm/memory-failure.c > >> @@ -1559,7 +1559,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn, > >> * Here we are interested only in user-mapped pages, so skip any > >> * other types of pages. > >> */ > >> - if (PageReserved(p) || PageSlab(p) || PageTable(p)) > >> + if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p)) > > > > hwpoison_user_mappings() is called after some checks are done, so I'm not > > sure that it's the right place to check PageOffline(). > > hwpoison_user_mappings() is called after the "if (!PageLRU(p) && !PageWriteback(p))" check in memory_failure(). > So the page can't also be PageReserved(p) or PageSlab(p) or PageTable(p) here? I think the check here just wants > to make things clear that only user-mapped pages are interested. Or am I miss something? No, you're right, So this "if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))" can be considered as checking potential deviation. OK, so the patch is fine. Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 42e63b0ab5f7..ed79b69837de 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -1559,7 +1559,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn, * Here we are interested only in user-mapped pages, so skip any * other types of pages. */ - if (PageReserved(p) || PageSlab(p) || PageTable(p)) + if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p)) return true; if (!(PageLRU(hpage) || PageHuge(p))) return true; @@ -2513,7 +2513,8 @@ int unpoison_memory(unsigned long pfn) goto unlock_mutex; } - if (folio_test_slab(folio) || PageTable(&folio->page) || folio_test_reserved(folio)) + if (folio_test_slab(folio) || PageTable(&folio->page) || + folio_test_reserved(folio) || PageOffline(&folio->page)) goto unlock_mutex; /*
Memory failure is not interested in logically offlined page. Skip this type of pages. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/memory-failure.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)