On 7/15/21 8:48 PM, Dan Williams wrote:
> On Thu, Jul 15, 2021 at 5:52 AM Joao Martins <joao.m.martins@xxxxxxxxxx> wrote:
>> On 7/15/21 2:08 AM, Dan Williams wrote:
>>> On Wed, Jul 14, 2021 at 12:36 PM Joao Martins <joao.m.martins@xxxxxxxxxx> wrote:
>>>> + 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.
