Re: [PATCH v10] mm: compaction: handle incorrect MIGRATE_UNMOVABLE type pageblocks

[Minchan Kim <minchan@kernel.org>]

On 06/11/2012 11:54 PM, Bartlomiej Zolnierkiewicz wrote:

> On Monday 11 June 2012 15:39:16 Minchan Kim wrote:
>> On Mon, Jun 11, 2012 at 12:43:14PM +0200, Bartlomiej Zolnierkiewicz wrote:
>>> On Monday 11 June 2012 03:26:49 Minchan Kim wrote:
>>>> Hi Bartlomiej,
>>>>
>>>> On 06/08/2012 05:46 PM, Bartlomiej Zolnierkiewicz wrote:
>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> This version is much simpler as it just uses __count_immobile_pages()
>>>>> instead of using its own open coded version and it integrates changes
>>>>
>>>>
>>>> That's a good idea. I don't have noticed that function is there.
>>>> When I look at the function, it has a problem, too.
>>>> Please, look at this.
>>>>
>>>> https://lkml.org/lkml/2012/6/10/180
>>>>
>>>> If reviewer is okay that patch, I would like to resend your patch based on that. 
>>>
>>> Ok, I would later merge all changes into v11 and rebase on top of your patch.
>>>
>>>>> from Minchan Kim (without page_count change as it doesn't seem correct
>>>>
>>>>
>>>> Why do you think so?
>>>> If it isn't correct, how can you prevent racing with THP page freeing?
>>>
>>> After seeing the explanation for the previous fix it is all clear now.
>>>
>>>>> and __count_immobile_pages() does the check in the standard way; if it
>>>>> still is a problem I think that removing 1st phase check altogether
>>>>> would be better instead of adding more locking complexity).
>>>>>
>>>>> The patch also adds compact_rescued_unmovable_blocks vmevent to vmstats
>>>>> to make it possible to easily check if the code is working in practice.
>>>>
>>>>
>>>> I think that part should be another patch.
>>>>
>>>> 1. Adding new vmstat would be arguable so it might interrupt this patch merging.
>>>
>>> Why would it be arguable?  It seems non-intrusive and obvious to me.
>>
>> Once you add new vmstat, someone can make another dependent code in userspace.
>> It means your new vmstat would become a new ABI so we should be careful.
> 
> I know about it but I doubt that it will be ever used by the user-space
> for other purpose than showing kernel statistics (even that is unlikely).
> 
>>>
>>>> 2. New vmstat adding is just for this patch is effective or not in real practice
>>>>    so if we prove it in future, let's revert the vmstat. Separating it would make it
>>>>    easily.
>>>
>>> I would like to add this vmstat permanently, not only for the testing period..
>>
>> "I would like to add this vmstat permanently" isn't logical at all.
>> You should mention why we need such vmstat and how administrator can parse it/
>> handle it if he needs.
> 
> I quickly went through vmstat history and I see rationales like this:
> "Optional patch, but useful for development and understanding the system."
> for adding new vmstat counters.  The new counter falls into this category.
> 


I can't agree your word "useful for development"
If we did it long time ago, I think it's wrong decision and never done again.

> compact_rescued_unmovable_blocks shows the number of MIGRATE_UNMOVABLE
> pageblocks converted back to MIGRATE_MOVABLE type by the memory compaction
> code.  Non-zero values indicate that large kernel-originated allocations
> of MIGRATE_UNMOVABLE type happen in the system and need special handling 
> from the memory compaction code.


So what can admin do it after he looked at?
Why should he know about "some unmovable block rescued"?

Thing admin want is allocation ratio or compaction ratio, NOT how many rescued.
If the patch is useful, THP alloc/compaction success ratio could be improved.
I think it's enough if you try to want to maintain it permanently.

> 
>> If we have Documentation/vmstat.txt, you should have written it down. Sigh.
> 
> But we don't have vmstat.txt even though we have a lot of vmstat counters. :(
> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung Poland R&D Center
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>