linux-kernel - Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a0a3f023-956d-a558-c3ab-53ae8b709b68@suse.cz>
Date:   Fri, 17 Feb 2017 17:09:57 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Johannes Weiner <hannes@...xchg.org>
Cc:     linux-mm@...ck.org, Joonsoo Kim <iamjoonsoo.kim@....com>,
        David Rientjes <rientjes@...gle.com>,
        Mel Gorman <mgorman@...hsingularity.net>,
        linux-kernel@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH v2 04/10] mm, page_alloc: count movable pages when
 stealing from pageblock

On 02/14/2017 07:10 PM, Johannes Weiner wrote:
> 
> That makes sense to me. I have just one nit about the patch:
> 
>> @@ -1981,10 +1994,29 @@ static void steal_suitable_fallback(struct zone *zone, struct page *page,
>>  		return;
>>  	}
>>  
>> -	pages = move_freepages_block(zone, page, start_type);
>> +	free_pages = move_freepages_block(zone, page, start_type,
>> +						&good_pages);
>> +	/*
>> +	 * good_pages is now the number of movable pages, but if we
>> +	 * want UNMOVABLE or RECLAIMABLE allocation, it's more tricky
>> +	 */
>> +	if (start_type != MIGRATE_MOVABLE) {
>> +		/*
>> +		 * If we are falling back to MIGRATE_MOVABLE pageblock,
>> +		 * treat all non-movable pages as good. If it's UNMOVABLE
>> +		 * falling back to RECLAIMABLE or vice versa, be conservative
>> +		 * as we can't distinguish the exact migratetype.
>> +		 */
>> +		old_block_type = get_pageblock_migratetype(page);
>> +		if (old_block_type == MIGRATE_MOVABLE)
>> +			good_pages = pageblock_nr_pages
>> +						- free_pages - good_pages;
> 
> This line had me scratch my head for a while, and I think it's mostly
> because of the variable naming and the way the comments are phrased.
> 
> Could you use a variable called movable_pages to pass to and be filled
> in by move_freepages_block?
> 
> And instead of good_pages something like starttype_pages or
> alike_pages or st_pages or mt_pages or something, to indicate the
> number of pages that are comparable to the allocation's migratetype?
> 
>> -	/* Claim the whole block if over half of it is free */
>> -	if (pages >= (1 << (pageblock_order-1)) ||
>> +	/* Claim the whole block if over half of it is free or good type */
>> +	if (free_pages + good_pages >= (1 << (pageblock_order-1)) ||
>>  			page_group_by_mobility_disabled)
>>  		set_pageblock_migratetype(page, start_type);
> 
> This would then read
> 
> 	if (free_pages + alike_pages ...)
> 
> which I think would be more descriptive.
> 
> The comment leading the entire section following move_freepages_block
> could then say something like "If a sufficient number of pages in the
> block are either free or of comparable migratability as our
> allocation, claim the whole block." Followed by the caveats of how we
> determine this migratibility.
> 
> Or maybe even the function. The comment above the function seems out
> of date after this patch.

I'll incorporate this for the next posting, thanks for the feedback!