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: