Re: [PATCH v6 3/6] mm/cma: populate ZONE_CMA

[Joonsoo Kim]

On Tue, Oct 18, 2016 at 05:27:30PM +0900, Joonsoo Kim wrote:
> On Tue, Oct 18, 2016 at 09:42:57AM +0200, Vlastimil Babka wrote:
> > On 10/14/2016 05:03 AM, js1304@xxxxxxxxx wrote:
> > >@@ -145,6 +145,35 @@ static int __init cma_activate_area(struct cma *cma)
> > > static int __init cma_init_reserved_areas(void)
> > > {
> > > 	int i;
> > >+	struct zone *zone;
> > >+	pg_data_t *pgdat;
> > >+
> > >+	if (!cma_area_count)
> > >+		return 0;
> > >+
> > >+	for_each_online_pgdat(pgdat) {
> > >+		unsigned long start_pfn = UINT_MAX, end_pfn = 0;
> > >+
> > >+		for (i = 0; i < cma_area_count; i++) {
> > >+			if (pfn_to_nid(cma_areas[i].base_pfn) !=
> > >+				pgdat->node_id)
> > >+				continue;
> > >+
> > >+			start_pfn = min(start_pfn, cma_areas[i].base_pfn);
> > >+			end_pfn = max(end_pfn, cma_areas[i].base_pfn +
> > >+						cma_areas[i].count);
> > >+		}
> > >+
> > >+		if (!end_pfn)
> > >+			continue;
> > >+
> > >+		zone = &pgdat->node_zones[ZONE_CMA];
> > >+
> > >+		/* ZONE_CMA doesn't need to exceed CMA region */
> > >+		zone->zone_start_pfn = max(zone->zone_start_pfn, start_pfn);
> > >+		zone->spanned_pages = min(zone_end_pfn(zone), end_pfn) -
> > >+					zone->zone_start_pfn;
> > 
> > Hmm, do the max/min here work as intended? IIUC the initial
> 
> Yeap.
> 
> > zone_start_pfn is UINT_MAX and zone->spanned_pages is 1? So at least
> > the max/min should be swapped?
> 
> No. CMA zone's start/end pfn are updated as node's start/end pfn.
> 
> > Also the zone_end_pfn(zone) on the second line already sees the
> > changes to zone->zone_start_pfn in the first line, so it's kind of a
> > mess. You should probably cache zone_end_pfn() to a temporary
> > variable before changing zone_start_pfn.
> 
> You're right although it doesn't cause any problem. I look at the code
> again and find that max/min isn't needed. Calculated start/end pfn
> should be inbetween node's start/end pfn so max(zone->zone_start_pfn,
> start_pfn) will return start_pfn and messed up min(zone_end_pfn(zone),
> end_pfn) will return end_pfn in all the cases.
> 
> Anyway, I will fix it as following.
> 
> zone->zone_start_pfn = start_pfn
> zone->spanned_pages = end_pfn - start_pfn

Hello,

Here comes fixed one.

----------->8------------