Re: [PATCH 1/2] mm: Restructure free-page stealing code and fix abug

[Srivatsa S. Bhat]

On 07/25/2013 08:40 AM, Wanpeng Li wrote:
> On Tue, Jul 23, 2013 at 12:18:06AM +0530, Srivatsa S. Bhat wrote:
>> The free-page stealing code in __rmqueue_fallback() is somewhat hard to
>> follow, and has an incredible amount of subtlety hidden inside!
>>
>> First off, there is a minor bug in the reporting of change-of-ownership of
>> pageblocks. Under some conditions, we try to move upto 'pageblock_nr_pages'
>> no. of pages to the preferred allocation list. But we change the ownership
>> of that pageblock to the preferred type only if we manage to successfully
>> move atleast half of that pageblock (or if page_group_by_mobility_disabled
>> is set).
>>
>> However, the current code ignores the latter part and sets the 'migratetype'
>> variable to the preferred type, irrespective of whether we actually changed
>> the pageblock migratetype of that block or not. So, the page_alloc_extfrag
>> tracepoint can end up printing incorrect info (i.e., 'change_ownership'
>> might be shown as 1 when it must have been 0).
>>
>> So fixing this involves moving the update of the 'migratetype' variable to
>> the right place. But looking closer, we observe that the 'migratetype' variable
>> is used subsequently for checks such as "is_migrate_cma()". Obviously the
>> intent there is to check if the *fallback* type is MIGRATE_CMA, but since we
>> already set the 'migratetype' variable to start_migratetype, we end up checking
>> if the *preferred* type is MIGRATE_CMA!!
>>
>> To make things more interesting, this actually doesn't cause a bug in practice,
>> because we never change *anything* if the fallback type is CMA.
>>
>> So, restructure the code in such a way that it is trivial to understand what
>> is going on, and also fix the above mentioned bug. And while at it, also add a
>> comment explaining the subtlety behind the migratetype used in the call to
>> expand().
>>
> 
> Greate catch!
> 

Thank you :-)

Regards,
Srivatsa S. Bhat

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