| Message ID | 20230715031729.2420338-3-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:27AM +0800, Miaohe Lin wrote: > If unpoison_memory() fails to clear page hwpoisoned flag, return value > ret is expected to be -EBUSY. But when get_hwpoison_page() returns 1 > and fails to clear page hwpoisoned flag due to races, return value will > be unexpected 1 leading to users being confused. Thank you for fixing this. > > Fixes: bf181c582588 ("mm/hwpoison: fix unpoison_memory()") > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/memory-failure.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 9ab97016877e..ac074f82f5b3 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -2546,11 +2546,11 @@ int unpoison_memory(unsigned long pfn) > unpoison_pr_info("Unpoison: failed to grab page %#lx\n", > pfn, &unpoison_rs); > } else { > + ret = -EBUSY; It seems be a code smell to me that the variable "ret" is used not only to save the return value of unpoison_memory(), but also to save the return value from get_hwpoison_page(). So I think that it might be better to use another auto-variable solely to save the return value of get_hwpoison_page. Then ret has the initial value -EBUSY at this point and no need to reinitialize it. Thanks, Naoya Horiguchi > if (PageHuge(p)) { > huge = true; > count = folio_free_raw_hwp(folio, false); > if (count == 0) { > - ret = -EBUSY; > folio_put(folio); > goto unlock_mutex; > } > -- > 2.33.0 > > >
On 2023/7/20 7:48, Naoya Horiguchi wrote: > On Sat, Jul 15, 2023 at 11:17:27AM +0800, Miaohe Lin wrote: >> If unpoison_memory() fails to clear page hwpoisoned flag, return value >> ret is expected to be -EBUSY. But when get_hwpoison_page() returns 1 >> and fails to clear page hwpoisoned flag due to races, return value will >> be unexpected 1 leading to users being confused. > > Thank you for fixing this. > >> >> Fixes: bf181c582588 ("mm/hwpoison: fix unpoison_memory()") >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/memory-failure.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >> index 9ab97016877e..ac074f82f5b3 100644 >> --- a/mm/memory-failure.c >> +++ b/mm/memory-failure.c >> @@ -2546,11 +2546,11 @@ int unpoison_memory(unsigned long pfn) >> unpoison_pr_info("Unpoison: failed to grab page %#lx\n", >> pfn, &unpoison_rs); >> } else { >> + ret = -EBUSY; > > It seems be a code smell to me that the variable "ret" is used not only to > save the return value of unpoison_memory(), but also to save the return > value from get_hwpoison_page(). So I think that it might be better to use > another auto-variable solely to save the return value of get_hwpoison_page. > Then ret has the initial value -EBUSY at this point and no need to > reinitialize it. > This should be a nice improvement. Will do it in v2. Thanks Naoya.
diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 9ab97016877e..ac074f82f5b3 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -2546,11 +2546,11 @@ int unpoison_memory(unsigned long pfn) unpoison_pr_info("Unpoison: failed to grab page %#lx\n", pfn, &unpoison_rs); } else { + ret = -EBUSY; if (PageHuge(p)) { huge = true; count = folio_free_raw_hwp(folio, false); if (count == 0) { - ret = -EBUSY; folio_put(folio); goto unlock_mutex; }
If unpoison_memory() fails to clear page hwpoisoned flag, return value ret is expected to be -EBUSY. But when get_hwpoison_page() returns 1 and fails to clear page hwpoisoned flag due to races, return value will be unexpected 1 leading to users being confused. Fixes: bf181c582588 ("mm/hwpoison: fix unpoison_memory()") Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/memory-failure.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)