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

[Minchan Kim]

On Mon, May 23, 2022 at 07:55:25PM -0700, John Hubbard wrote:
> 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

The load tearing you are trying to explain in the comment would be
solved by [1] since the bits will always align on a word and accessing
word size based on word aligned address is always atomic so there is
no load tearing problem IIUC.

Instead of the tearing problem, what we are trying to solve with
READ_ONCE is to prevent refetching when the function would be
inlined in the future.

> 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

set_pageblock_migratetype is more upper level function so it would
be better fit to say set_pfnblock_flags_mask.
                                     
>  * a consistent (non-tearing) read of the memory array, so that results,

So tearing problem should't already happen by [1] so I am trying to
explain refetching(or re-read) problem in the comment.

>  * even though racy, are not corrupted--even if this function is

The value is already atomic so I don't think it could be corrupted
even though it would be inlined in the future.

Please correct me if I miss something.

>  * refactored and/or inlined.
>  */