On Tue, Dec 08, 2020 at 09:59:19PM -0800, John Hubbard wrote:
On 12/8/20 9:28 AM, Joao Martins wrote:
> Add a new flag for struct dev_pagemap which designates that a a pagemap
> is described as a set of compound pages or in other words, that how
> pages are grouped together in the page tables are reflected in how we
> describe struct pages. This means that rather than initializing
> individual struct pages, we also initialize these struct pages, as
Let's not say "rather than x, we also do y", because it's
I think you want to just leave out the "also", like this:
"This means that rather than initializing> individual struct pages, we
initialize these struct pages ..."
Is that right?
I'd phrase it as:
Add a new flag for struct dev_pagemap which specifies that a pagemap is
composed of a set of compound pages instead of individual pages. When
these pages are initialised, most are initialised as tail pages
instead of order-0 pages.
> For certain ZONE_DEVICE users, like device-dax, which have a
> size, this creates an opportunity to optimize GUP and GUP-fast walkers,
> thus playing the same tricks as hugetlb pages.
Rather than "playing the same tricks", how about "are treated the same
way as THP or hugetlb pages"?
> + if (pgmap->flags & PGMAP_COMPOUND)
> + percpu_ref_get_many(pgmap->ref, (pfn_end(pgmap, range_id)
> + - pfn_first(pgmap, range_id)) / PHYS_PFN(pgmap->align));
Is there some reason that we cannot use range_len(), instead of pfn_end() minus
pfn_first()? (Yes, this more about the pre-existing code than about your change.)
And if not, then why are the nearby range_len() uses OK? I realize that range_len()
is simpler and skips a case, but it's not clear that it's required here. But
new to this area so be warned. :)
Also, dividing by PHYS_PFN() feels quite misleading: that function does what you
happen to want, but is not named accordingly. Can you use or create something
more accurately named? Like "number of pages in this large page"?
We have compound_nr(), but that takes a struct page as an argument.
We also have HPAGE_NR_PAGES. I'm not quite clear what you want.