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-spacefor 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_UNMOVABLEpageblocks converted back to MIGRATE_MOVABLE type by the memory compactioncode. Non-zero values indicate that large kernel-originated allocationsof 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. :(