Re: [PATCH v6 04/10] mm/memremap: add ZONE_DEVICE support for compound pages

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

On 11/25/21 06:11, Christoph Hellwig wrote:
> On Wed, Nov 24, 2021 at 07:09:59PM +0000, Joao Martins wrote:
>> Add a new @vmemmap_shift property for struct dev_pagemap which specifies that a
>> devmap is composed of a set of compound pages of order @vmemmap_shift, instead of
>> base pages. When a compound page devmap is requested, all but the first
>> page are initialised as tail pages instead of order-0 pages.
> 
> Please wrap commit log lines after 73 characters.
> 
Fixed.

>>  #define for_each_device_pfn(pfn, map, i) \
>> -	for (pfn = pfn_first(map, i); pfn < pfn_end(map, i); pfn = pfn_next(pfn))
>> +	for (pfn = pfn_first(map, i); pfn < pfn_end(map, i); pfn = pfn_next(map, pfn))
> 
> It would be nice to fix up this long line while you're at it.
> 
OK -- I am gonna assume that it's enough to move pfn = pfn_next(...)
clause into the next line.

>>  static void dev_pagemap_kill(struct dev_pagemap *pgmap)
>>  {
>> @@ -315,8 +315,8 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params *params,
>>  	memmap_init_zone_device(&NODE_DATA(nid)->node_zones[ZONE_DEVICE],
>>  				PHYS_PFN(range->start),
>>  				PHYS_PFN(range_len(range)), pgmap);
>> -	percpu_ref_get_many(pgmap->ref, pfn_end(pgmap, range_id)
>> -			- pfn_first(pgmap, range_id));
>> +	percpu_ref_get_many(pgmap->ref, (pfn_end(pgmap, range_id)
>> +			- pfn_first(pgmap, range_id)) >> pgmap->vmemmap_shift);
> 
> In the Linux coding style the - goes ointo the first line.
> 
> But it would be really nice to clean this up with a helper ala pfn_len
> anyway:
> 
> 	percpu_ref_get_many(pgmap->ref,
> 			    pfn_len(pgmap, range_id) >> pgmap->vmemmap_shift);
> 
OK, I moved the computation to an helper.

I've staged your comments (see below diff for this patch), plus wrapping the commit
message to 73 columns (I've also double-checked and this one seems to be the only one
making that mistake).

I'll wait a couple days to follow up v7 should you have further comments
in other patches.

diff --git a/mm/memremap.c b/mm/memremap.c
index 3afa246eb1ab..d591f3aa8884 100644
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -109,6 +109,12 @@ static unsigned long pfn_next(struct dev_pagemap *pgmap, unsigned
long pfn)
        return pfn + pgmap_vmemmap_nr(pgmap);
 }

+static unsigned long pfn_len(struct dev_pagemap *pgmap, unsigned long range_id)
+{
+       return (pfn_end(pgmap, range_id) -
+               pfn_first(pgmap, range_id)) >> pgmap->vmemmap_shift;
+}
+
 /*
  * This returns true if the page is reserved by ZONE_DEVICE driver.
  */
@@ -130,7 +136,8 @@ bool pfn_zone_device_reserved(unsigned long pfn)
 }

 #define for_each_device_pfn(pfn, map, i) \
-       for (pfn = pfn_first(map, i); pfn < pfn_end(map, i); pfn = pfn_next(map, pfn))
+       for (pfn = pfn_first(map, i); pfn < pfn_end(map, i); \
+            pfn = pfn_next(map, pfn))

 static void dev_pagemap_kill(struct dev_pagemap *pgmap)
 {
@@ -315,8 +322,7 @@ static int pagemap_range(struct dev_pagemap *pgmap, struct mhp_params
*params,
        memmap_init_zone_device(&NODE_DATA(nid)->node_zones[ZONE_DEVICE],
                                PHYS_PFN(range->start),
                                PHYS_PFN(range_len(range)), pgmap);
-       percpu_ref_get_many(pgmap->ref, (pfn_end(pgmap, range_id)
-                       - pfn_first(pgmap, range_id)) >> pgmap->vmemmap_shift);
+       percpu_ref_get_many(pgmap->ref, pfn_len(pgmap, range_id));
        return 0;

 err_add_memory: