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;
Subject: Re: [RFC] mm/vmstat: Add events for THP migration without split
To:     Zi Yan <ziy@nvidia.com>
Cc:     linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        linux-kernel@vger.kernel.org
References: <1589257372-29576-1-git-send-email-anshuman.khandual@arm.com>
 <67C72AD3-120C-4825-B67B-AECD4245EC4E@nvidia.com>
From:   Anshuman Khandual <anshuman.khandual@arm.com>
Message-ID: <7b4504c8-8583-d426-fef1-4c14986798c3@arm.com>
Date:   Fri, 15 May 2020 09:20:43 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <67C72AD3-120C-4825-B67B-AECD4245EC4E@nvidia.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 05/14/2020 07:58 PM, Zi Yan wrote:
> On 12 May 2020, at 0:22, Anshuman Khandual wrote:
> 
>> Add the following new trace events which will help in validating migration
>> events involving PMD based THP pages.
>>
>> 1. THP_PMD_MIGRATION_ENTRY_SET
>> 2. THP_PMD_MIGRATION_ENTRY_REMOVE
>>
>> There are no clear method to confirm whether a THP migration happened with
>> out involving it's split. These trace events along with PGMIGRATE_SUCCESS
>> and PGMIGRATE_FAILURE will provide additional insights. After this change,
>>
>> A single 2M THP (2K base page) when migrated
>>
>> 1. Without split
>>
>> ................
>> pgmigrate_success 1
>> pgmigrate_fail 0
>> ................
>> thp_pmd_migration_entry_set 1
>> thp_pmd_migration_entry_remove 1
>> ................
>>
>> 2. With split
>>
>> ................
>> pgmigrate_success 512
>> pgmigrate_fail 0
>> ................
>> thp_pmd_migration_entry_set 0
>> thp_pmd_migration_entry_remove 0
>> ................
>>
>> pgmigrate_success as 1 instead of 512, provides a hint for possible THP
>> migration event. But then it gets mixed with normal page migrations over
>> time. These additional trace events provide required co-relation.
> 
> To track successful THP migrations, the code below should work, right?
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index b1092876e537..d394f5331288 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1220,6 +1220,8 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
>          * we want to retry.
>          */
>         if (rc == MIGRATEPAGE_SUCCESS) {
> +               if (PageTransHuge(newpage))
> +                       count_vm_event(THP_PMD_MIGRATION_SUCCESS);

Thats right.

>                 put_page(page);
>                 if (reason == MR_MEMORY_FAILURE) {
>                         /*

Another THP_PMD_MIGRATION_FAILURE event in migrate_pages() when the THP gets
split as a huge page could not be allocated. Both THP_PMD_MIGRATION_SUCCESS
and THP_PMD_MIGRATION_FAILURE will provide a better understanding regarding
THP migration events on the system.

> 
> Maybe you could give more details on why you want to add the THP migration event and
> how you are going to use the event in your use case. That would be very helpful to this
> code review. Are you going to do anything if you see THP migration failures?

Not at the moment. Like other VM events these new ones will provide required
statistics (and better understanding) on THP migration which can be used to
improve it further. Follows the same good old principle, if we cannot measure
we cannot improve.