linux-kernel - Re: [PATCH v5 06/14] mm, compaction: reduce zone checking frequency in the migration scanner

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]

Date:	Tue, 29 Jul 2014 11:31:51 +0200
From:	Vlastimil Babka <vbabka@...e.cz>
To:	David Rientjes <rientjes@...gle.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-mm:;
Subject: Re: [PATCH v5 06/14] mm, compaction: reduce zone checking frequency
 in the migration scanner

On 07/29/2014 02:44 AM, David Rientjes wrote:
>> Signed-off-by: Vlastimil Babka <vbabka@...e.cz>
>> Cc: Minchan Kim <minchan@...nel.org>
>> Acked-by: Mel Gorman <mgorman@...e.de>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@....com>
>> Cc: Michal Nazarewicz <mina86@...a86.com>
>> Cc: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
>> Cc: Christoph Lameter <cl@...ux.com>
>> Cc: Rik van Riel <riel@...hat.com>
>> Cc: David Rientjes <rientjes@...gle.com>
>
> Acked-by: David Rientjes <rientjes@...gle.com>
>
> Minor comments below.

Thanks,

>> +/*
>> + * Check that the whole (or subset of) a pageblock given by the interval of
>> + * [start_pfn, end_pfn) is valid and within the same zone, before scanning it
>> + * with the migration of free compaction scanner. The scanners then need to
>> + * use only pfn_valid_within() check for arches that allow holes within
>> + * pageblocks.
>> + *
>> + * Return struct page pointer of start_pfn, or NULL if checks were not passed.
>> + *
>> + * It's possible on some configurations to have a setup like node0 node1 node0
>> + * i.e. it's possible that all pages within a zones range of pages do not
>> + * belong to a single zone. We assume that a border between node0 and node1
>> + * can occur within a single pageblock, but not a node0 node1 node0
>> + * interleaving within a single pageblock. It is therefore sufficient to check
>> + * the first and last page of a pageblock and avoid checking each individual
>> + * page in a pageblock.
>> + */
>> +static struct page *pageblock_within_zone(unsigned long start_pfn,
>> +				unsigned long end_pfn, struct zone *zone)
>
> The name of this function is quite strange, it's returning a pointer to
> the actual start page but the name implies it would be a boolean.

Yeah but I couldn't think of a better name that wouldn't be long and ugly :(

>> +{
>> +	struct page *start_page;
>> +	struct page *end_page;
>> +
>> +	/* end_pfn is one past the range we are checking */
>> +	end_pfn--;
>> +
>
> With the given implementation, yes, but I'm not sure if that should be
> assumed for any class of callers.  It seems better to call with
> end_pfn - 1.

Well, I think the rest of compaction functions assume one-past-end 
parameters so this would make it an exception. Better hide the exception 
in the implementation and not expose it to callers?

>> +	if (!pfn_valid(start_pfn) || !pfn_valid(end_pfn))
>> +		return NULL;
>> +
>
> Ok, so even with this check, we still need to check pfn_valid_within() for
> all pfns between start_pfn and end_pfn if there are memory holes.  I
> checked that both the migration and freeing scanners do that before
> reading your comment above the function, looks good.

Yeah, and thankfully pfn_valid_within() is a no-op on many archs :)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/