Re: [PATCH] mm/page_alloc: Fix race conditions on getting migratetype in buffered_rmqueue

[Hui Zhu]

On Sun, Jan 18, 2015 at 6:19 PM, Vlastimil Babka <vbabka@xxxxxxx> wrote:
> On 18.1.2015 10:17, Hui Zhu wrote:
>>
>> From: Hui Zhu <zhuhui@xxxxxxxxxx>
>>
>> To test the patch [1], I use KGTP and a script [2] to show
>> NR_FREE_CMA_PAGES
>> and gross of cma_nr_free.  The values are always not same.
>> I check the code of pages alloc and free and found that race conditions
>> on getting migratetype in buffered_rmqueue.
>
>
> Can you elaborate? What does this races with, are you dynamically changing
> the size of CMA area, or what? The migratetype here is based on which free
> list the page was found on. Was it misplaced then? Wasn't Joonsoo's recent
> series supposed to eliminate this?

My bad.
I thought move_freepages has race condition with this part.  But I
missed it will check PageBuddy before set_freepage_migratetype.
Sorry for that.

I will do more work around this one and [1].

Thanks for your review.

Best,
Hui

>
>> Then I add move the code of getting migratetype inside the zone->lock
>> protection part.
>
>
> Not just that, you are also reading migratetype from pageblock bitmap
> instead of the one embedded in the free page. Which is more expensive
> and we already do that more often than we would like to because of CMA.
> And it appears to be a wrong fix for a possible misplacement bug. If there's
> such misplacement, the wrong stats are not the only problem.
>
>>
>> Because this issue will affect system even if the Linux kernel does't
>> have [1].  So I post this patch separately.
>
>
> But we can't test that without [1], right? Maybe the issue is introduced by
> [1]?
>
>
>>
>> This patchset is based on fc7f0dd381720ea5ee5818645f7d0e9dece41cb0.
>>
>> [1] https://lkml.org/lkml/2015/1/18/28
>> [2] https://github.com/teawater/kgtp/blob/dev/add-ons/cma_free.py
>>
>> Signed-off-by: Hui Zhu <zhuhui@xxxxxxxxxx>
>> ---
>>   mm/page_alloc.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 7633c50..f3d6922 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -1694,11 +1694,12 @@ again:
>>                 }
>>                 spin_lock_irqsave(&zone->lock, flags);
>>                 page = __rmqueue(zone, order, migratetype);
>> +               if (page)
>> +                       migratetype = get_pageblock_migratetype(page);
>> +               else
>> +                       goto failed_unlock;
>>                 spin_unlock(&zone->lock);
>> -               if (!page)
>> -                       goto failed;
>> -               __mod_zone_freepage_state(zone, -(1 << order),
>> -                                         get_freepage_migratetype(page));
>> +               __mod_zone_freepage_state(zone, -(1 << order),
>> migratetype);
>>         }
>>         __mod_zone_page_state(zone, NR_ALLOC_BATCH, -(1 << order));
>> @@ -1715,6 +1716,8 @@ again:
>>                 goto again;
>>         return page;
>>   +failed_unlock:
>> +       spin_unlock(&zone->lock);
>>   failed:
>>         local_irq_restore(flags);
>>         return NULL;
>
>
--
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/