Re: [PATCH v4] mm: fix is_pinnable_page against on cma page

[John Hubbard]

On 5/23/22 09:33, Minchan Kim wrote:
...
> > So then:
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 0e42038382c1..b404f87e2682 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -482,7 +482,12 @@ unsigned long __get_pfnblock_flags_mask(const struct page *page,
> >          word_bitidx = bitidx / BITS_PER_LONG;
> >          bitidx &= (BITS_PER_LONG-1);
> >
> > -       word = bitmap[word_bitidx];
> > +       /*
> > +        * This races, without locks, with set_pageblock_migratetype(). Ensure
>                                                   
>                                               set_pfnblock_flags_mask would be better?
>                            
> > +        * a consistent (non-tearing) read of the memory array, so that results,
>
> Thanks for proceeding and suggestion, John.
>
> IIUC, the load tearing wouldn't be an issue since [1] fixed the issue.

Did it? [1] fixed something, but I'm not sure we can claim that that
code is now safe against tearing in all possible cases, especially given
the recent discussion here. Specifically, having this code do a read,
then follow that up with calculations, seems correct. Anything else is
sketchy...

> The concern in our dicussion was aggressive compiler(e.g., LTO) or code refactoring
> to make the code inline in *future* could potentially cause forcing refetching(i.e.,
> re-read) tie bitmap[word_bitidx].
>
> If so, shouldn't the comment be the one you helped before?

Well, maybe updated to something like this?

/*
 * This races, without locks, with set_pageblock_migratetype(). Ensure
 * a consistent (non-tearing) read of the memory array, so that results,
 * even though racy, are not corrupted--even if this function is
 * refactored and/or inlined.
 */



thanks,
--
John Hubbard
NVIDIA