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

["Paul E. McKenney" <paulmck@kernel.org>]

On Wed, May 11, 2022 at 04:13:10PM -0700, John Hubbard wrote:
> On 5/11/22 16:08, Minchan Kim wrote:
> > > OK, so the code checks the wrong item each time. But the code really
> > > only needs to know "is either _CMA or _ISOLATE set?". And so you
> > 
> > Yes.
> > 
> > > can just sidestep the entire question by writing it like this:
> > > 
> > > int mt = get_pageblock_migratetype(page);
> > > 
> > > if (mt & (MIGRATE_ISOLATE | MIGRATE_CMA))
> > > 	return false;
> > 
> > I am confused. Isn't it same question?
> > 
> >                                                      set_pageblock_migratetype(MIGRATE_ISOLATE)
> > if (get_pageblock_migrate(page) & MIGRATE_CMA)
> > 
> >                                                      set_pageblock_migratetype(MIGRATE_CMA)
> > 
> > if (get_pageblock_migrate(page) & MIGRATE_ISOLATE)
> 
> Well no, because the "&" operation is a single operation on the CPU, and
> isn't going to get split up like that.

Chiming in a bit late...

The usual way that this sort of thing causes trouble is if there is a
single store instruction that changes the value from MIGRATE_ISOLATE
to MIGRATE_CMA, and if the compiler decides to fetch twice, AND twice,
and then combine the results.  This could give a zero outcome where the
underlying variable never had the value zero.

Is this sort of thing low probability?

Definitely.

Isn't this sort of thing prohibited?

Definitely not.

So what you have will likely work for at least a while longer, but it
is not guaranteed and it forces you to think a lot harder about what
the current implementations of the compiler can and cannot do to you.

The following LWN article goes through some of the possible optimizations
(vandalisms?) in this area: https://lwn.net/Articles/793253/

In the end, it is your code, so you get to decide how much you would
like to keep track of what compilers get up to over time.  ;-)

							Thanx, Paul