lkml.org 
[lkml]   [2012]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v10] mm: compaction: handle incorrect MIGRATE_UNMOVABLE type pageblocks
    Date
    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.

    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.

    > 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


    \
     
     \ /
      Last update: 2012-06-11 17:42    [W:0.027 / U:1.424 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site