Re: [PATCH v3 04/14] mm/memremap: add ZONE_DEVICE support for compound pages

On 7/15/21 8:48 PM, Dan Williams wrote:

On Thu, Jul 15, 2021 at 5:52 AM Joao Martins [off-list ref] wrote:

quoted

On 7/15/21 2:08 AM, Dan Williams wrote:

quoted

On Wed, Jul 14, 2021 at 12:36 PM Joao Martins [off-list ref] wrote:

quoted

+                                       unsigned long zone_idx, int nid,
+                                       struct dev_pagemap *pgmap,
+                                       unsigned long nr_pages)
+{
+       unsigned int order_align = order_base_2(nr_pages);
+       unsigned long i;
+
+       __SetPageHead(page);
+
+       for (i = 1; i < nr_pages; i++) {

The switch of loop styles is jarring. I.e. the switch from
memmap_init_zone_device() that is using pfn, end_pfn, and a local
'struct page *' variable to this helper using pfn + i and a mix of
helpers (__init_zone_device_page,  prep_compound_tail) that have
different expectations of head page + tail_idx and current page.

I.e. this reads more obviously correct to me, but maybe I'm just in
the wrong headspace:

        for (pfn = head_pfn + 1; pfn < end_pfn; pfn++) {
                struct page *page = pfn_to_page(pfn);

                __init_zone_device_page(page, pfn, zone_idx, nid, pgmap);
                prep_compound_tail(head, pfn - head_pfn);

Personally -- and I am dubious given I have been staring at this code -- I find that what
I wrote a little better as it follows more what compound page initialization does. Like
it's easier for me to read that I am initializing a number of tail pages and a head page
(for a known geometry size).

Additionally, it's unnecessary (and a tiny ineficient?) to keep doing pfn_to_page(pfn)
provided ZONE_DEVICE requires SPARSEMEM_VMEMMAP and so your page pointers are all
contiguous and so for any given PFN we can avoid having deref vmemmap vaddrs back and
forth. Which is the second reason I pass a page, and iterate over its tails based on a
head page pointer. But I was at too minds when writing this, so if the there's no added
inefficiency I can rewrite like the above.

I mainly just don't want 2 different styles between
memmap_init_zone_device() and this helper. So if the argument is that
"it's inefficient to use pfn_to_page() here" then why does the caller
use pfn_to_page()? I won't argue too much for one way or the other,
I'm still biased towards my rewrite, but whatever you pick just make
the style consistent.

Meanwhile, turns out my concerns didn't materialize. I am not seeing a
visible difference compared to old numbers. So I switched to the style
you suggested above.