On Wed, Jun 10, 2026 at 03:24:30PM +0800, Miaohe Lin wrote: > On 2026/6/10 5:00, Michael S. Tsirkin wrote: > > On Tue, Jun 09, 2026 at 04:54:01PM -0400, Zi Yan wrote: > >> On 9 Jun 2026, at 16:34, Michael S. Tsirkin wrote: > >> > >>> On Tue, Jun 09, 2026 at 02:52:47PM -0400, Zi Yan wrote: > >>>> On 9 Jun 2026, at 14:39, Zi Yan wrote: > >>>> > >>>>> On 9 Jun 2026, at 14:38, David Hildenbrand (Arm) wrote: > >>>>> > >>>>>> On 6/9/26 20:10, Andrew Morton wrote: > >>>>>>> On Tue, 9 Jun 2026 06:12:49 -0400 "Michael S. Tsirkin" > >>>>>>> <[email protected]> wrote: > >>>>>>> > >>>>>>>> TestSetPageHWPoison() is called without zone->lock, so its atomic > >>>>>>>> update to page->flags can race with non-atomic flag operations > >>>>>>>> that run under zone->lock in the buddy allocator. > >>>>>>>> > >>>>>>>> In particular, __free_pages_prepare() does: > >>>>>>>> > >>>>>>>> page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP; > >>>>>>>> > >>>>>>>> This non-atomic read-modify-write, while correctly excluding > >>>>>>>> __PG_HWPOISON from the mask, can still lose a concurrent > >>>>>>>> TestSetPageHWPoison if the read happens before the poison bit > >>>>>>>> is set and the write happens after. Will only get worse if/when > >>>>>>>> we add more non-atomic flag operations. > >>>>>>>> > >>>>>>>> Fix by acquiring zone->lock around TestSetPageHWPoison and > >>>>>>>> around ClearPageHWPoison in the retry path. This > >>>>>>>> serializes with all buddy flag manipulation. The cost is > >>>>>>>> negligible: one lock/unlock in an extremely rare path > >>>>>>>> (hardware memory errors). > >>>>>>>> > >>>>>>>> Note: SetPageHWPoison and TestClearPageHWPoison calls elsewhere > >>>>>>>> in this file operate on pages already removed from the buddy > >>>>>>>> allocator or on non-buddy pages (DAX, hugetlb), so they do not > >>>>>>>> need zone->lock protection. > >>>>>>> > >>>>>>> Sashiko is saying this doesn't do anything "Because > >>>>>>> __free_pages_prepare() executes entirely locklessly". Did it goof? > >>>>>>> > >>>>>>> https://sashiko.dev/#/patchset/df06b66fe4ff8e925ee0714955abc2183a727b90.1780998980.git....@redhat.com > >>>>>> > >>>>>> Battle of the bots: it's right. > >>>>> > >>>>> Yep, __free_pages_prepare() changes the page flag without holding > >>>>> zone->lock. > >>>> > >>>> __free_pages_prepare() works on frozen pages and assumes no one else > >>>> touches the input page. To avoid this race, memory_failure() might > >>>> want to try_get_page() before TestClearPageHWPoison(), but I am not > >>>> sure if that works along with memory failure flow. > >>>> > >>>> Best Regards, > >>>> Yan, Zi > >>> > >>> > >>> > >>> Actually memory failure already plays with this down the road no? > >>> > >>> So maybe it's enough to just SetPageHWPoison afterwards again? > >>> > >>> > >>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c > >>> index ee42d4361309..4758fea94a96 100644 > >>> --- a/mm/memory-failure.c > >>> +++ b/mm/memory-failure.c > >>> @@ -2415,6 +2415,7 @@ int memory_failure(unsigned long pfn, int flags) > >>> if (!res) { > >>> if (is_free_buddy_page(p)) { > >>> if (take_page_off_buddy(p)) { > >>> + SetPageHWPoison(p); > >>> page_ref_inc(p); > >>> res = MF_RECOVERED; > >>> } else { > >>> > >>> > >>> and maybe in a bunch of other places in there? > >> > >> You mean for fear of losing HWPoison flag in the earlier > >> TestSetPageHWPoison(), > >> just set it again here? > > > > Yea. > > > >> Why not do it after get_hwpoison_page(), since that > >> is the expected page flag? > > > > It's still in the buddy at that point right? I'm worried buddy might > > poke at flags. > > Since __free_pages_prepare() executes entirely locklessly, the only way to > ensure > HWPoison flag won't be lost might be only set hwpoison flag iff we can make > sure > pages are not on the way to buddy... > > Thanks. > .
Right so here after take_page_off_buddy it's ok, for example. -- MST
