Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 7 Jul 2020 13:04:06 -0700
From:   Andrew Morton <akpm@linux-foundation.org>
To:     Anshuman Khandual <anshuman.khandual@arm.com>
Cc:     linux-mm@kvack.org, Daniel Jordan <daniel.m.jordan@oracle.com>,
        Hugh Dickins <hughd@google.com>,
        Matthew Wilcox <willy@infradead.org>, Zi Yan <ziy@nvidia.com>,
        John Hubbard <jhubbard@nvidia.com>,
        Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH V3] mm/vmstat: Add events for THP migration without
 split
Message-Id: <20200707130406.21ce3d61a4cce25831abb2d1@linux-foundation.org>
In-Reply-To: <1594080415-27924-1-git-send-email-anshuman.khandual@arm.com>
References: <1594080415-27924-1-git-send-email-anshuman.khandual@arm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue,  7 Jul 2020 05:36:55 +0530 Anshuman Khandual <anshuman.khandual@arm.com> wrote:

> Add following new vmstat events which will help in validating THP migration
> without split. Statistics reported through these new VM events will help in
> performance debugging.
> 
> 1. THP_MIGRATION_SUCCESS
> 2. THP_MIGRATION_FAILURE
> 3. THP_MIGRATION_SPLIT
> 
> In addition, these new events also update normal page migration statistics
> appropriately via PGMIGRATE_SUCCESS and PGMIGRATE_FAILURE. While here, this
> updates current trace event 'mm_migrate_pages' to accommodate now available
> THP statistics.

Patch looks straightforward enough.  It would be nice to see some
confirmation from others that these metrics are a desirable thing to
export.

> ...
>
> -	trace_mm_migrate_pages(nr_succeeded, nr_failed, mode, reason);
> +	if (nr_thp_succeeded)
> +		count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
> +	if (nr_thp_failed)
> +		count_vm_events(THP_MIGRATION_FAILURE, nr_thp_failed);
> +	if (nr_thp_split)
> +		count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);

Are these "if"s worthwhile to have?

> +	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
> +			       nr_thp_failed, nr_thp_split, mode, reason);