Re: [PATCH v2 1/2] mm/vmemmap/devdax: Fix kernel crash when probing devdax devices

[Joao Martins <joao.m.martins@oracle.com>]

On 11/04/2023 16:20, Aneesh Kumar K.V wrote:
> Joao Martins <joao.m.martins@oracle.com> 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.