Message-ID: <20200609222905.vgh3x7i2d5wzmtzh@ca-dmjordan1.us.oracle.com>
Date:   Tue, 9 Jun 2020 18:29:05 -0400
From:   Daniel Jordan <daniel.m.jordan@...cle.com>
To:     Anshuman Khandual <anshuman.khandual@....com>
Cc:     Zi Yan <ziy@...dia.com>, Matthew Wilcox <willy@...radead.org>,
        linux-mm@...ck.org, hughd@...gle.com, daniel.m.jordan@...cle.com,
        Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
        John Hubbard <jhubbard@...dia.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] mm/vmstat: Add events for THP migration without split

On Tue, Jun 09, 2020 at 05:05:45PM +0530, Anshuman Khandual wrote:
> On 06/05/2020 07:54 PM, Zi Yan wrote:
> > On 4 Jun 2020, at 23:35, Anshuman Khandual wrote:
> >> Thanks Zi, for such a detailed explanation. Ideally, we should separate THP
> >> migration from base page migration in terms of statistics. PGMIGRATE_SUCCESS
> >> and PGMIGRATE_FAIL should continue to track statistics when migration starts
> >> with base pages. But for THP, we should track the following events.
> > 
> > You mean PGMIGRATE_SUCCESS and PGMIGRATE_FAIL will not track the number of migrated subpages
> > from split THPs? Will it cause userspace issues since their semantics are changed?
> 
> Yeah, basic idea is to carve out all THP migration related statistics from
> the normal page migration. Not sure if that might cause any issue for user
> space as you have mentioned. Does /proc/vmstat indicate some sort of an ABI
> whose semantics can not really change ? Some more opinions here from others
> would be helpful.

vmstats have had their implementations changed for THP before, so there's at
least some precedent.

https://lore.kernel.org/linux-mm/1559025859-72759-2-git-send-email-yang.shi@linux.alibaba.com/

Others know better than I do.

> The current situation is definitely bit problematic where PGMIGRATE_SUCCESS
> increments (+1) both for normal and successful THP migration. Same situation
> for PGMIGRATE_FAILURE as well.

Yeah, that's suboptimal.  Maybe a separate patch to get thps accounted properly
in those stats?

> Hence, there are two clear choices available.
> 
> 1. THP and normal page migration stats are separate and mutually exclusive
> 
> OR
> 
> 2. THP migration has specific counters but normal page migration counters
>    still account for everything in THP migration in terms of normal pages

FWIW, 2 makes more sense to me because it suits the existing names (it's
PGMIGRATE_SUCCESS not PGMIGRATE_SMALL_PAGES_SUCCESS) and it's less surprising
than leaving thp out of them.

> But IIUC, either way the current PGMIGRATE_SUCCESS or PGMIGRATE_FAIL stats
> will change for the user as visible from /proc/vmstat.
> 
> > 
> >>
> >> 1. THP_MIGRATION_SUCCESS 	- THP migration is successful, without split
> >> 2. THP_MIGRATION_FAILURE 	- THP could neither be migrated, nor be split
> > 
> > They make sense to me.> 
> >> 3. THP_MIGRATION_SPLIT_SUCCESS  - THP got split and all sub pages migrated
> >> 4. THP_MIGRATION_SPLIT_FAILURE  - THP got split but all sub pages could not be migrated
> >>
> >> THP_MIGRATION_SPLIT_FAILURE could either increment once for a single THP or
> >> number of subpages that did not get migrated after split. As you mentioned,
> >> this will need some extra code in the core migration. Nonetheless, if these
> >> new events look good, will be happy to make required changes.
> > 
> > Maybe THP_MIGRATION_SPLIT would be simpler? My concern is that whether we need such
> 
> Also, it will not require a new migration queue tracking the split THP sub pages.
> 
> > detailed information or not. Maybe trace points would be good enough for 3 and 4.

I share the concern about whether that many new stats are necessary.  The
changelog says this is for performance debugging, so it seems THP success and
THP split would cover that.

The split stat becomes redundant if the other information is needed in the
future, but it can presumably be removed in that case.

Powered by blists - more mailing lists