On Wed, 21 May 2014 16:13:39 +0200 Vlastimil Babka <[email protected]> wrote:
> >> > >> ... > >> > >> @@ -718,9 +739,11 @@ static void isolate_freepages(struct zone *zone, > >> /* > >> * This can iterate a massively long zone without finding any > >> * suitable migration targets, so periodically check if we need > >> - * to schedule. > >> + * to schedule, or even abort async compaction. > >> */ > >> - cond_resched(); > >> + if (!(block_start_pfn % (SWAP_CLUSTER_MAX * pageblock_nr_pages)) > >> + && compact_should_abort(cc)) > > > > This seems rather gratuitously inefficient and isn't terribly clear. > > What's wrong with > > > > if ((++foo % SWAP_CLUSTER_MAX) == 0 && compact_should_abort(cc)) > > It's a new variable and it differs from how isolate_migratepages_range() does > this. > But yeah, I might change it later there as well. There it makes even more > sense. > E.g. when skipping whole pageblock there, pfn % SWAP_CLUSTER_MAX will be > always zero > so the periodicity varies. > > > ? > > > > (Assumes that SWAP_CLUSTER_MAX is power-of-2 and that the compiler will > > use &) > > I hoped that compiler would be smart enough about SWAP_CLUSTER_MAX * > pageblock_nr_pages > as well, as those are constants and also power-of-2. But I didn't check the > assembly. Always check the assembly! Just a quick `size mm/compaction.o' is enough tell if you're on the right track. > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -779,6 +779,7 @@ static void isolate_freepages(struct zone *zone, > unsigned long block_start_pfn; /* start of current pageblock */ > unsigned long block_end_pfn; /* end of current pageblock */ > unsigned long low_pfn; /* lowest pfn scanner is able to scan */ > + unsigned long nr_blocks_scanned = 0; /* for periodical abort checks */ > int nr_freepages = cc->nr_freepages; > struct list_head *freelist = &cc->freepages; > > @@ -813,7 +814,7 @@ static void isolate_freepages(struct zone *zone, > * suitable migration targets, so periodically check if we need > * to schedule, or even abort async compaction. > */ > - if (!(block_start_pfn % (SWAP_CLUSTER_MAX * pageblock_nr_pages)) > + if ((++nr_blocks_scanned % SWAP_CLUSTER_MAX) == 0 > && compact_should_abort(cc)) > break; This change actually makes the code worse, and the .o file gets larger. For some stupid reason we went and make pageblock_nr_pages all lower case but surprise surprise, it's actually a literal constant. So the compiler does the multiplication at compile time and converts the modulus operation into a bitwise AND. Duh. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
