On Tue, 2012-10-30 at 09:14 -0600, Dave Hansen wrote: > On 10/20/2012 01:29 AM, Mel Gorman wrote: > > I'm travelling at the moment so apologies that I have not followed up on > > this. My problem is still the same with the patch - it changes more > > headers than is necessary and it is sparsemem specific. At minimum, try > > the suggestion of > > > > if (!early_pfn_valid(pfn)) { > > pfn = ALIGN(pfn + MAX_ORDER_NR_PAGES, MAX_ORDER_NR_PAGES) - 1; > > continue; > > } > > Sorry I didn't catch this until v2... > > Is that ALIGN() correct? If pfn=3, then it would expand to: > > (3+MAX_ORDER_NR_PAGES+MAX_ORDER_NR_PAGES-1) & ~(MAX_ORDER_NR_PAGES-1) > > You would end up skipping the current MAX_ORDER_NR_PAGES area, and then > one _extra_ because ALIGN() aligns up, and you're adding > MAX_ORDER_NR_PAGES too. It doesn't matter unless you run in to a > !early_valid_pfn() in the middle of a MAX_ORDER area, I guess. > > I think this would work, plus be a bit smaller: > > pfn = ALIGN(pfn + 1, MAX_ORDER_NR_PAGES) - 1; > Dave, I see your point about "rounding-up". But, I favor the way Mel suggested it. It more clearly shows the intent, which is to move up by MAX_ORDER_NR_PAGES. The "pfn+1" may suggest that there is some significance to the next pfn, but there is not. I find Mel's way easier to understand. Mike Y -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html