Re: [BUGFIX][PATCH] fix wrong lru rotate back at lumpty reclaim

Minchan Kim
2009-06-09T11:31:46Z

2009/6/9 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>:
> Minchan Kim wrote:
>
>> I mean follow as
>>  908         /*
>>  909          * Attempt to take all pages in the order aligned region
>>  910          * surrounding the tag page.  Only take those pages of
>>  911          * the same active state as that tag page.  We may safely
>>  912          * round the target page pfn down to the requested order
>>  913          * as the mem_map is guarenteed valid out to MAX_ORDER,
>>  914          * where that page is in a different zone we will detect
>>  915          * it from its zone id and abort this block scan.
>>  916          */
>>  917         zone_id = page_zone_id(page);
>>
> But what this code really do is.
> ==
> 931                         /* Check that we have not crossed a zone
> boundary. */
>  932                         if (unlikely(page_zone_id(cursor_page) !=
> zone_id))
>  933                                 continue;
> ==
> continue. I think this should be "break"
> I wonder what "This block scan" means is "scanning this aligned block".

It is to find first page in same zone with target page when we have
crossed a zone.
so it shouldn't stop due to that.

I think 'abort' means stopping only the page.
If it is right, it would be better to change follow as.
"and continue scanning next page"

Let's Cced Andy Whitcroft.

> But I think the whoe code is not written as commented.
>
>>
>>>> If I understand it properly , don't we add goto phrase ?
>>>>
>>> No.
>>
>> If it is so, the break also is meaningless.
>>
> yes. I'll remove it. But need to add "exit from for loop" logic again.
>
> I'm sorry that the wrong logic of this loop was out of my sight.
> I'll review and rewrite this part all, tomorrow.

Yes. I will review tomorrow, too. :)

> Thanks,
> -Kame
>
>

--
Kinds regards,
Minchan Kim