Re: [PATCH] mm: Make PageType more efficient

[Alexander Duyck <alexander.duyck@gmail.com>]

On Wed, Mar 11, 2020 at 6:13 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Mar 10, 2020 at 02:50:50PM -0700, Alexander Duyck wrote:
> > On Tue, Mar 10, 2020 at 1:37 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > -#define PageType(page, flag)                                           \
> > > > > -       ((page->page_type & (PAGE_TYPE_BASE | flag)) == PAGE_TYPE_BASE)
> > > > > -
> >
> > >From what I can tell this is the only consumer of PAGE_TYPE_BASE.
> > Since it is removed you can probably remove that definition as well.
>
> I _could_ ... I do want to indicate to people that they probably
> shouldn't use those bits in order to leave space for overflow and
> wraparound of _mapcount.

You already have some of this with the check against
PAGE_MAPCOUNT_RESERVE. That is guaranteeing the sign bit is set and
that at least one of the bits has been cleared as otherwise it would
be equal to 128. The bit you aren't enforcing though is the upper 4
bits. It might make sense to just replace the #define with a comment
further down that the upper 4 bit are reserved.

> > > > > +#define PageType(page, flag)                                           \
> > > > > +       (page_has_type(page) && (~page->page_type & flag))
> >
> > You can probably spare a cycle or two here by testing for
> > "!(page->page_type & flag)". That way you avoid the extra bit flipping
> > since the compiler can just handle the result of the AND op as it sees
> > fit.
>
> GCC already knows to do that optimisation; mm/page_alloc.o is identical
> (same md5sum) when changing from (~page->page_type & flag) to
> !(page->page_type & flag).  So it's just a question of which one is
> easier for humans to read and reason about.  Do you have an opinion
> which one you'd like to see?

So it looks like Andrew and Ira are kind of thinking the same way I am.

Also one other thing that occurred to me is that by breaking it up the
way you did I think it might open things up for possible races since
the flag and upper bits are being done as two separate checks. It
might make sense to just combine this all into one function and use
the READ_ONCE macro on the read of the page_type so that you can be
guaranteed that the reading of the variable is atomic.