Re: [mm PATCH v5 7/7] mm: Use common iterator for deferred_init_pages and deferred_free_pages

[Alexander Duyck]

On 11/9/2018 8:13 PM, Pavel Tatashin wrote:
> On 18-11-05 13:20:01, Alexander Duyck wrote:
> > +static unsigned long __next_pfn_valid_range(unsigned long *i,
> > +					    unsigned long end_pfn)
> >   {
> > -	if (!pfn_valid_within(pfn))
> > -		return false;
> > -	if (!(pfn & (pageblock_nr_pages - 1)) && !pfn_valid(pfn))
> > -		return false;
> > -	return true;
> > +	unsigned long pfn = *i;
> > +	unsigned long count;
> > +
> > +	while (pfn < end_pfn) {
> > +		unsigned long t = ALIGN(pfn + 1, pageblock_nr_pages);
> > +		unsigned long pageblock_pfn = min(t, end_pfn);
> > +
> > +#ifndef CONFIG_HOLES_IN_ZONE
> > +		count = pageblock_pfn - pfn;
> > +		pfn = pageblock_pfn;
> > +		if (!pfn_valid(pfn))
> > +			continue;
> > +#else
> > +		for (count = 0; pfn < pageblock_pfn; pfn++) {
> > +			if (pfn_valid_within(pfn)) {
> > +				count++;
> > +				continue;
> > +			}
> > +
> > +			if (count)
> > +				break;
> > +		}
> > +
> > +		if (!count)
> > +			continue;
> > +#endif
> > +		*i = pfn;
> > +		return count;
> > +	}
> > +
> > +	return 0;
> >   }
> >   
> > +#define for_each_deferred_pfn_valid_range(i, start_pfn, end_pfn, pfn, count) \
> > +	for (i = (start_pfn),						     \
> > +	     count = __next_pfn_valid_range(&i, (end_pfn));		     \
> > +	     count && ({ pfn = i - count; 1; });			     \
> > +	     count = __next_pfn_valid_range(&i, (end_pfn)))
>
> Can this be improved somehow? It took me a while to understand this
> piece of code. i is actually end of block, and not an index by PFN, ({pfn = i - count; 1;}) is
> simply hard to parse. Why can't we make __next_pfn_valid_range() to
> return both end and a start of a block?

One thing I could do is flip the direction and work from the end to the 
start. If I did that then 'i' and 'pfn' would be the same value and I 
wouldn't have to do the subtraction. If that works for you I could 
probably do that and it may actually be more efficient.

Otherwise I could probably pass pfn as a reference, and compute it in 
the case where count is non-zero.

> The rest is good:
>
> Reviewed-by: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
>
> Thank you,
> Pasha