Re: [PATCH 3/3] mm: adjust rss counters for migration entiries

[Hugh Dickins]

On Wed, 18 Jan 2012, Andrew Morton wrote:
> On Wed, 11 Jan 2012 17:41:26 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> > On Wed, 11 Jan 2012 12:23:11 +0400
> > Konstantin Khlebnikov <khlebnikov@xxxxxxxxxx> wrote:

I only just got around to looking at these, sorry.

> 
> Putting "fix" in the patch title text is a good way of handling this.
> 
> I renamed [3/3] to "mm: fix rss count leakage during migration" and
> shall queue it for 3.3.  If people think we should also backport it
> into -stable then please let me know.

I don't think it needs backporting to stable: unless I'm forgetting
something, the only thing that actually uses these rss counters is the
OOM killer, and I don't think that will be greatly affected by the bug.

> 
> I reordered the patches and worked the chagnelogs quite a bit.  I now
> have:
> 
> : From: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxx>
> : Subject: mm: fix rss count leakage during migration
> : 
> : Memory migration fills a pte with a migration entry and it doesn't update
> : the rss counters.  Then it replaces the migration entry with the new page
> : (or the old one if migration failed).  But between these two passes this
> : pte can be unmaped, or a task can fork a child and it will get a copy of
> : this migration entry.  Nobody accounts for this in the rss counters.
> : 
> : This patch properly adjust rss counters for migration entries in
> : zap_pte_range() and copy_one_pte().  Thus we avoid extra atomic operations
> : on the migration fast-path.
> : 
> : Signed-off-by: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxx>
> : Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> : Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>

That was a good find, Konstantin: thank you.

> 
> and
> 
> : From: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxx>
> : Subject: mm: add rss counters consistency check
> : 
> : Warn about non-zero rss counters at final mmdrop.
> : 
> : This check will prevent reoccurences of bugs such as that fixed in "mm:
> : fix rss count leakage during migration".
> : 
> : I didn't hide this check under CONFIG_VM_DEBUG because it rather small and
> : rss counters cover whole page-table management, so this is a good
> : invariant.
> : 
> : Signed-off-by: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxx>
> : Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> : Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>

I'd be happier with this one if you do hide the check under
CONFIG_VM_DEBUG - or even under CONFIG_DEBUG_VM if you want it to
be compiled in sometimes ;)  I suppose NR_MM_COUNTERS is only 3,
so it isn't a huge overhead; but I think you're overestimating the
importance of these counters, and it would look better under DEBUG_VM.

> 
> and
> 
> : From: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxx>
> : Subject: mm: postpone migrated page mapping reset
> : 
> : Postpone resetting page->mapping until the final remove_migration_ptes(). 
> : Otherwise the expression PageAnon(migration_entry_to_page(entry)) does not
> : work.
> : 
> : Signed-off-by: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxx>
> : Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> : Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>

Isn't this one actually an essential part of the fix?  It should have
been part of the same patch, but you split them apart, now Andrew has
reordered them and pushed one part to 3.3, but this needs to go in too?

Hugh
--
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/