linux-kernel - Re: [PATCH 2/2] mm/compaction: cleanup isolate

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:	Wed, 23 Apr 2014 16:31:14 +0200
From:	Vlastimil Babka <vbabka@...e.cz>
To:	Joonsoo Kim <js1304@...il.com>
CC:	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Minchan Kim <minchan@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mel Gorman <mgorman@...e.de>,
	Heesub Shin <heesub.shin@...sung.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux Memory Management List <linux-mm@...ck.org>,
	Dongjun Shin <d.j.shin@...sung.com>,
	Sunghwan Yun <sunghwan.yun@...sung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Michal Nazarewicz <mina86@...a86.com>,
	Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
	Christoph Lameter <cl@...ux.com>,
	Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

>>>
>>> Hello,
>>>
>>> How about doing more clean-up at this time?
>>>
>>> What I did is that taking end_pfn out of the loop and consider zone
>>> boundary once. After then, we just subtract pageblock_nr_pages on
>>> every iteration. With this change, we can remove local variable, z_end_pfn.
>>> Another things I did are removing max() operation and un-needed
>>> assignment to isolate variable.
>>>
>>> Thanks.
>>>
>>> --------->8------------
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index 1c992dc..95a506d 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -671,10 +671,10 @@ static void isolate_freepages(struct zone *zone,
>>>                               struct compact_control *cc)
>>>  {
>>>       struct page *page;
>>> -     unsigned long pfn;           /* scanning cursor */
>>> +     unsigned long pfn;           /* start of scanning window */
>>> +     unsigned long end_pfn;       /* end of scanning window */
>>>       unsigned long low_pfn;       /* lowest pfn scanner is able to scan */
>>>       unsigned long next_free_pfn; /* start pfn for scaning at next round */
>>> -     unsigned long z_end_pfn;     /* zone's end pfn */
>>>       int nr_freepages = cc->nr_freepages;
>>>       struct list_head *freelist = &cc->freepages;
>>>
>>> @@ -688,15 +688,16 @@ static void isolate_freepages(struct zone *zone,
>>>        * is using.
>>>        */
>>>       pfn = cc->free_pfn & ~(pageblock_nr_pages-1);
>>> -     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>
>>>       /*
>>> -      * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
>>> -      * isolated, the pfn < low_pfn check will kick in.
>>> +      * Take care when isolating in last pageblock of a zone which
>>> +      * ends in the middle of a pageblock.
>>>        */
>>> -     next_free_pfn = 0;
>>> +     end_pfn = min(pfn + pageblock_nr_pages, zone_end_pfn(zone));
>>> +     low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
>>>
>>> -     z_end_pfn = zone_end_pfn(zone);
>>> +     /* If no pages are isolated, the pfn < low_pfn check will kick in. */
>>> +     next_free_pfn = 0;
>>>
>>>       /*
>>>        * Isolate free pages until enough are available to migrate the
>>> @@ -704,9 +705,8 @@ static void isolate_freepages(struct zone *zone,
>>>        * and free page scanners meet or enough free pages are isolated.
>>>        */
>>>       for (; pfn >= low_pfn && cc->nr_migratepages > nr_freepages;
>>> -                                     pfn -= pageblock_nr_pages) {
>>> +             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
>>
>> If zone_end_pfn was in the middle of a pageblock, then your end_pfn will
>> always be in the middle of a pageblock and you will not scan half of all
>> pageblocks.
>>
> 
> Okay. I think a way to fix it.
> By assigning pfn(start of scanning window) to
> end_pfn(end of scanning window) for the next loop, we can solve the problem
> you mentioned. How about below?
> 
> -             pfn -= pageblock_nr_pages, end_pfn -= pageblock_nr_pages) {
> +            end_pfn = pfn, pfn -= pageblock_nr_pages) {

Hm that's perhaps a bit subtle but it would work.
Maybe better names for pfn and end_pfn would be block_start_pfn and
block_end_pfn. And in those comments, s/scanning window/current pageblock/.
And please don't move the low_pfn assignment like you did. The comment
above the original location explains it, the comment above the new
location doesn't. It's use in the loop is also related to 'pfn', not
'end_pfn'.

> Thanks.
> 

--
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/