On 11/04/2023 16:20, Aneesh Kumar K.V wrote:
> Joao Martins <joao.m.martins@xxxxxxxxxx> writes:
>> On 11/04/2023 15:18, Aneesh Kumar K.V wrote:
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index 3bb3484563ed..292411d8816f 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -6844,10 +6844,13 @@ static void __ref __init_zone_device_page(struct page *page, unsigned long pfn,
>>> * of an altmap. See vmemmap_populate_compound_pages().
>>> */
>>> static inline unsigned long compound_nr_pages(struct vmem_altmap *altmap,
>>> + struct dev_pagemap *pgmap,
>>> unsigned long nr_pages)
>>> {
>>> - return is_power_of_2(sizeof(struct page)) &&
>>> - !altmap ? 2 * (PAGE_SIZE / sizeof(struct page)) : nr_pages;
>>> + if (vmemmap_can_optimize(altmap, pgmap))
>>> + return 2 * (PAGE_SIZE / sizeof(struct page));
>>> + else
>>> + return nr_pages;
>>> }
>>>
>>
>> Keep the ternary operator as already is the case for compound_nr_pages to avoid
>> doing too much in one patch:
>>
>> return vmemmap_can_optimize(altmap, pgmap) ?
>> 2 * (PAGE_SIZE / sizeof(struct page)) : nr_pages;
>>
>> Or you really want to remove the ternary operator perhaps take the unnecessary
>> else and make the long line be less indented:
>>
>> if (!vmemmap_can_optimize(altmap, pgmap))
>> return nr_pages;
>>
>> return 2 * (PAGE_SIZE / sizeof(struct page));
>>
>> I don't think the latter is a significant improvement over the ternary one. But
>> I guess that's a matter of preferred style.
>
> How about
>
> static inline unsigned long compound_nr_pages(struct vmem_altmap *altmap,
> struct dev_pagemap *pgmap)
> {
>
> if (!vmemmap_can_optimize(altmap, pgmap))
> return pgmap_vmemmap_nr(pgmap);
>
> return 2 * (PAGE_SIZE / sizeof(struct page));
> }
>
I am a bit more fond of the ternary option. But this snip above is fine as well,
including with the s/nr_pages/pgmap_vmemmap_nr(pgmap)/ change you added.
