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    [from the cache]
©2003-2011 Jasper Spaans