[V4] mm/vmstat: Add events for THP migration without split

Commit Message

Anshuman Khandual July 9, 2020, 9:39 a.m. UTC

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_FAIL
3. THP_MIGRATION_SPLIT

In addition, these new events also update normal page migration statistics
appropriately via PGMIGRATE_SUCCESS and PGMIGRATE_FAIL. While here, this
updates current trace event 'mm_migrate_pages' to accommodate now available
THP statistics.

Cc: Jonathan Corbet <[e-mail]>
Cc: Steven Rostedt <[e-mail]>
Cc: Ingo Molnar <[e-mail]>
Cc: Daniel Jordan <[e-mail]>
Cc: Hugh Dickins <[e-mail]>
Cc: Matthew Wilcox <[e-mail]>
Cc: Zi Yan <[e-mail]>
Cc: John Hubbard <[e-mail]>
Cc: Naoya Horiguchi <[e-mail]>
Cc: Andrew Morton <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Zi Yan <[e-mail]>
Signed-off-by: Anshuman Khandual <[e-mail]>
---
Applies on 5.8-rc4.

Changes in V4:

- Changed THP_MIGRATION_FAILURE as THP_MIGRATION_FAIL per John
- Dropped all conditional 'if' blocks in migrate_pages() per Andrew and John
- Updated migration events documentation per John
- Updated thp_nr_pages variable as nr_subpages for an expected merge conflict
- Moved all new THP vmstat events into CONFIG_MIGRATION
- Updated Cc list with Documentation/ and tracing related addresses

Changes in V3: (https://patchwork.kernel.org/patch/11647237/)

- Formatted new events documentation with 'fmt' tool per Matthew
- Made events universally available i.e dropped ARCH_ENABLE_THP_MIGRATION
- Added THP_MIGRATION_SPLIT
- Updated trace_mm_migrate_pages() with THP events
- Made THP events update normal page migration events as well

Changes in V2: (https://patchwork.kernel.org/patch/11586893/)

- Dropped PMD reference both from code and commit message per Matthew
- Added documentation and updated the commit message per Daniel

Changes in V1: (https://patchwork.kernel.org/patch/11564497/)

- Changed function name as thp_pmd_migration_success() per John
- Folded in a fix (https://patchwork.kernel.org/patch/11563009/) from Hugh

Changes in RFC V2: (https://patchwork.kernel.org/patch/11554861/)

- Decopupled and renamed VM events from their implementation per Zi and John
- Added THP_PMD_MIGRATION_FAILURE VM event upon allocation failure and split

Changes in RFC V1: (https://patchwork.kernel.org/patch/11542055/)

 Documentation/vm/page_migration.rst | 27 +++++++++++++++
 include/linux/vm_event_item.h       |  3 ++
 include/trace/events/migrate.h      | 17 ++++++++--
 mm/migrate.c                        | 52 ++++++++++++++++++++++++-----
 mm/vmstat.c                         |  3 ++
 5 files changed, 91 insertions(+), 11 deletions(-)

Comments

Randy Dunlap July 9, 2020, 3:34 p.m. UTC | #1

Hi,

I have a few comments on this.

a. I reported it very early and should have been Cc-ed.

b. A patch that applies to mmotm or linux-next would have been better
than a full replacement patch.

c. I tried replacing what I believe is the correct/same patch file in mmotm
and still have build errors.

(more below)

On 7/9/20 2:39 AM, Anshuman Khandual wrote:

> ---
> Applies on 5.8-rc4.
> 
> Changes in V4:
> 
> - Changed THP_MIGRATION_FAILURE as THP_MIGRATION_FAIL per John
> - Dropped all conditional 'if' blocks in migrate_pages() per Andrew and John
> - Updated migration events documentation per John
> - Updated thp_nr_pages variable as nr_subpages for an expected merge conflict
> - Moved all new THP vmstat events into CONFIG_MIGRATION
> - Updated Cc list with Documentation/ and tracing related addresses
> 
> Changes in V3: (https://patchwork.kernel.org/patch/11647237/)
> 
> - Formatted new events documentation with 'fmt' tool per Matthew
> - Made events universally available i.e dropped ARCH_ENABLE_THP_MIGRATION
> - Added THP_MIGRATION_SPLIT
> - Updated trace_mm_migrate_pages() with THP events
> - Made THP events update normal page migration events as well
> 
> Changes in V2: (https://patchwork.kernel.org/patch/11586893/)
> 
> - Dropped PMD reference both from code and commit message per Matthew
> - Added documentation and updated the commit message per Daniel
> 
> Changes in V1: (https://patchwork.kernel.org/patch/11564497/)
> 
> - Changed function name as thp_pmd_migration_success() per John
> - Folded in a fix (https://patchwork.kernel.org/patch/11563009/) from Hugh
> 
> Changes in RFC V2: (https://patchwork.kernel.org/patch/11554861/)
> 
> - Decopupled and renamed VM events from their implementation per Zi and John
> - Added THP_PMD_MIGRATION_FAILURE VM event upon allocation failure and split
> 
> Changes in RFC V1: (https://patchwork.kernel.org/patch/11542055/)
> 
>  Documentation/vm/page_migration.rst | 27 +++++++++++++++
>  include/linux/vm_event_item.h       |  3 ++
>  include/trace/events/migrate.h      | 17 ++++++++--
>  mm/migrate.c                        | 52 ++++++++++++++++++++++++-----
>  mm/vmstat.c                         |  3 ++
>  5 files changed, 91 insertions(+), 11 deletions(-)
> 

> diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
> index 24fc7c3ae7d6..2e6ca53b9bbd 100644
> --- a/include/linux/vm_event_item.h
> +++ b/include/linux/vm_event_item.h
> @@ -56,6 +56,9 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>  #endif
>  #ifdef CONFIG_MIGRATION
>  		PGMIGRATE_SUCCESS, PGMIGRATE_FAIL,
> +		THP_MIGRATION_SUCCESS,
> +		THP_MIGRATION_FAIL,
> +		THP_MIGRATION_SPLIT,

These 3 new symbols are still only present if CONFIG_MIGRATION=y, but the build errors
are using these symbols even when CONFIG_MIGRATION is not set.

>  #endif
>  #ifdef CONFIG_COMPACTION
>  		COMPACTMIGRATE_SCANNED, COMPACTFREE_SCANNED,

> diff --git a/mm/migrate.c b/mm/migrate.c
> index f37729673558..c706e3576cfc 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1429,22 +1429,35 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  		enum migrate_mode mode, int reason)
>  {
>  	int retry = 1;
> +	int thp_retry = 1;
>  	int nr_failed = 0;
>  	int nr_succeeded = 0;
> +	int nr_thp_succeeded = 0;
> +	int nr_thp_failed = 0;
> +	int nr_thp_split = 0;
>  	int pass = 0;
> +	bool is_thp = false;
>  	struct page *page;
>  	struct page *page2;
>  	int swapwrite = current->flags & PF_SWAPWRITE;
> -	int rc;
> +	int rc, nr_subpages;
>  
>  	if (!swapwrite)
>  		current->flags |= PF_SWAPWRITE;
>  
> -	for(pass = 0; pass < 10 && retry; pass++) {
> +	for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
>  		retry = 0;
> +		thp_retry = 0;
>  
>  		list_for_each_entry_safe(page, page2, from, lru) {
>  retry:
> +			/*
> +			 * THP statistics is based on the source huge page.
> +			 * Capture required information that might get lost
> +			 * during migration.
> +			 */
> +			is_thp = PageTransHuge(page);
> +			nr_subpages = hpage_nr_pages(page);
>  			cond_resched();
>  
>  			if (PageHuge(page))
> @@ -1475,15 +1488,30 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  					unlock_page(page);
>  					if (!rc) {
>  						list_safe_reset_next(page, page2, lru);
> +						nr_thp_split++;
>  						goto retry;
>  					}
>  				}
> +				if (is_thp) {
> +					nr_thp_failed++;
> +					nr_failed += nr_subpages;
> +					goto out;
> +				}
>  				nr_failed++;
>  				goto out;
>  			case -EAGAIN:
> +				if (is_thp) {
> +					thp_retry++;
> +					break;
> +				}
>  				retry++;
>  				break;
>  			case MIGRATEPAGE_SUCCESS:
> +				if (is_thp) {
> +					nr_thp_succeeded++;
> +					nr_succeeded += nr_subpages;
> +					break;
> +				}
>  				nr_succeeded++;
>  				break;
>  			default:
> @@ -1493,19 +1521,27 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  				 * removed from migration page list and not
>  				 * retried in the next outer loop.
>  				 */
> +				if (is_thp) {
> +					nr_thp_failed++;
> +					nr_failed += nr_subpages;
> +					break;
> +				}
>  				nr_failed++;
>  				break;
>  			}
>  		}
>  	}
> -	nr_failed += retry;
> +	nr_failed += retry + thp_retry;
> +	nr_thp_failed += thp_retry;
>  	rc = nr_failed;
>  out:
> -	if (nr_succeeded)
> -		count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
> -	if (nr_failed)
> -		count_vm_events(PGMIGRATE_FAIL, nr_failed);
> -	trace_mm_migrate_pages(nr_succeeded, nr_failed, mode, reason);
> +	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
> +	count_vm_events(PGMIGRATE_FAIL, nr_failed);
> +	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
> +	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
> +	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);

These references still cause build errors.

> +	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
> +			       nr_thp_failed, nr_thp_split, mode, reason);
>  
>  	if (!swapwrite)
>  		current->flags &= ~PF_SWAPWRITE;
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 3fb23a21f6dd..09914a4bfee4 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1234,6 +1234,9 @@ const char * const vmstat_text[] = {
>  #ifdef CONFIG_MIGRATION
>  	"pgmigrate_success",
>  	"pgmigrate_fail",
> +	"thp_migration_success",
> +	"thp_migration_fail",
> +	"thp_migration_split",
>  #endif
>  #ifdef CONFIG_COMPACTION
>  	"compact_migrate_scanned",
>

Zi Yan July 9, 2020, 4:34 p.m. UTC | #2

On 9 Jul 2020, at 11:34, Randy Dunlap wrote:

> Hi,
>
> I have a few comments on this.
>
> a. I reported it very early and should have been Cc-ed.
>
> b. A patch that applies to mmotm or linux-next would have been better
> than a full replacement patch.
>
> c. I tried replacing what I believe is the correct/same patch file in mmotm
> and still have build errors.
>
> (more below)
>
> On 7/9/20 2:39 AM, Anshuman Khandual wrote:
>
>> ---
>> Applies on 5.8-rc4.
>>
>> Changes in V4:
>>
>> - Changed THP_MIGRATION_FAILURE as THP_MIGRATION_FAIL per John
>> - Dropped all conditional 'if' blocks in migrate_pages() per Andrew and John
>> - Updated migration events documentation per John
>> - Updated thp_nr_pages variable as nr_subpages for an expected merge conflict
>> - Moved all new THP vmstat events into CONFIG_MIGRATION
>> - Updated Cc list with Documentation/ and tracing related addresses
>>
>> Changes in V3: (https://patchwork.kernel.org/patch/11647237/)
>>
>> - Formatted new events documentation with 'fmt' tool per Matthew
>> - Made events universally available i.e dropped ARCH_ENABLE_THP_MIGRATION
>> - Added THP_MIGRATION_SPLIT
>> - Updated trace_mm_migrate_pages() with THP events
>> - Made THP events update normal page migration events as well
>>
>> Changes in V2: (https://patchwork.kernel.org/patch/11586893/)
>>
>> - Dropped PMD reference both from code and commit message per Matthew
>> - Added documentation and updated the commit message per Daniel
>>
>> Changes in V1: (https://patchwork.kernel.org/patch/11564497/)
>>
>> - Changed function name as thp_pmd_migration_success() per John
>> - Folded in a fix (https://patchwork.kernel.org/patch/11563009/) from Hugh
>>
>> Changes in RFC V2: (https://patchwork.kernel.org/patch/11554861/)
>>
>> - Decopupled and renamed VM events from their implementation per Zi and John
>> - Added THP_PMD_MIGRATION_FAILURE VM event upon allocation failure and split
>>
>> Changes in RFC V1: (https://patchwork.kernel.org/patch/11542055/)
>>
>>  Documentation/vm/page_migration.rst | 27 +++++++++++++++
>>  include/linux/vm_event_item.h       |  3 ++
>>  include/trace/events/migrate.h      | 17 ++++++++--
>>  mm/migrate.c                        | 52 ++++++++++++++++++++++++-----
>>  mm/vmstat.c                         |  3 ++
>>  5 files changed, 91 insertions(+), 11 deletions(-)
>>
>
>> diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
>> index 24fc7c3ae7d6..2e6ca53b9bbd 100644
>> --- a/include/linux/vm_event_item.h
>> +++ b/include/linux/vm_event_item.h
>> @@ -56,6 +56,9 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>>  #endif
>>  #ifdef CONFIG_MIGRATION
>>  		PGMIGRATE_SUCCESS, PGMIGRATE_FAIL,
>> +		THP_MIGRATION_SUCCESS,
>> +		THP_MIGRATION_FAIL,
>> +		THP_MIGRATION_SPLIT,
>
> These 3 new symbols are still only present if CONFIG_MIGRATION=y, but the build errors
> are using these symbols even when CONFIG_MIGRATION is not set.
>
>>  #endif
>>  #ifdef CONFIG_COMPACTION
>>  		COMPACTMIGRATE_SCANNED, COMPACTFREE_SCANNED,
>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index f37729673558..c706e3576cfc 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1429,22 +1429,35 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>  		enum migrate_mode mode, int reason)
>>  {
>>  	int retry = 1;
>> +	int thp_retry = 1;
>>  	int nr_failed = 0;
>>  	int nr_succeeded = 0;
>> +	int nr_thp_succeeded = 0;
>> +	int nr_thp_failed = 0;
>> +	int nr_thp_split = 0;
>>  	int pass = 0;
>> +	bool is_thp = false;
>>  	struct page *page;
>>  	struct page *page2;
>>  	int swapwrite = current->flags & PF_SWAPWRITE;
>> -	int rc;
>> +	int rc, nr_subpages;
>>
>>  	if (!swapwrite)
>>  		current->flags |= PF_SWAPWRITE;
>>
>> -	for(pass = 0; pass < 10 && retry; pass++) {
>> +	for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
>>  		retry = 0;
>> +		thp_retry = 0;
>>
>>  		list_for_each_entry_safe(page, page2, from, lru) {
>>  retry:
>> +			/*
>> +			 * THP statistics is based on the source huge page.
>> +			 * Capture required information that might get lost
>> +			 * during migration.
>> +			 */
>> +			is_thp = PageTransHuge(page);
>> +			nr_subpages = hpage_nr_pages(page);
>>  			cond_resched();
>>
>>  			if (PageHuge(page))
>> @@ -1475,15 +1488,30 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>  					unlock_page(page);
>>  					if (!rc) {
>>  						list_safe_reset_next(page, page2, lru);
>> +						nr_thp_split++;
>>  						goto retry;
>>  					}
>>  				}
>> +				if (is_thp) {
>> +					nr_thp_failed++;
>> +					nr_failed += nr_subpages;
>> +					goto out;
>> +				}
>>  				nr_failed++;
>>  				goto out;
>>  			case -EAGAIN:
>> +				if (is_thp) {
>> +					thp_retry++;
>> +					break;
>> +				}
>>  				retry++;
>>  				break;
>>  			case MIGRATEPAGE_SUCCESS:
>> +				if (is_thp) {
>> +					nr_thp_succeeded++;
>> +					nr_succeeded += nr_subpages;
>> +					break;
>> +				}
>>  				nr_succeeded++;
>>  				break;
>>  			default:
>> @@ -1493,19 +1521,27 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>  				 * removed from migration page list and not
>>  				 * retried in the next outer loop.
>>  				 */
>> +				if (is_thp) {
>> +					nr_thp_failed++;
>> +					nr_failed += nr_subpages;
>> +					break;
>> +				}
>>  				nr_failed++;
>>  				break;
>>  			}
>>  		}
>>  	}
>> -	nr_failed += retry;
>> +	nr_failed += retry + thp_retry;
>> +	nr_thp_failed += thp_retry;
>>  	rc = nr_failed;
>>  out:
>> -	if (nr_succeeded)
>> -		count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>> -	if (nr_failed)
>> -		count_vm_events(PGMIGRATE_FAIL, nr_failed);
>> -	trace_mm_migrate_pages(nr_succeeded, nr_failed, mode, reason);
>> +	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>> +	count_vm_events(PGMIGRATE_FAIL, nr_failed);
>> +	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
>> +	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
>> +	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
>
> These references still cause build errors.
>
>> +	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
>> +			       nr_thp_failed, nr_thp_split, mode, reason);
>>
>>  	if (!swapwrite)
>>  		current->flags &= ~PF_SWAPWRITE;
>> diff --git a/mm/vmstat.c b/mm/vmstat.c
>> index 3fb23a21f6dd..09914a4bfee4 100644
>> --- a/mm/vmstat.c
>> +++ b/mm/vmstat.c
>> @@ -1234,6 +1234,9 @@ const char * const vmstat_text[] = {
>>  #ifdef CONFIG_MIGRATION
>>  	"pgmigrate_success",
>>  	"pgmigrate_fail",
>> +	"thp_migration_success",
>> +	"thp_migration_fail",
>> +	"thp_migration_split",
>>  #endif
>>  #ifdef CONFIG_COMPACTION
>>  	"compact_migrate_scanned",
>>
>

Which arch are you building? I did not see any error
after applying this patch on mmotm (reverting the existing ones)
and compiling them on x86_64. I used make x86_64_defconfig and
unselected COMPACTION and MIGRATION.

mm/migrate.c and added vm events will not be used
if CONFIG_MIGRATION is unchecked. Why would they cause compilation errors?


—
Best Regards,
Yan Zi

Randy Dunlap July 9, 2020, 4:39 p.m. UTC | #3

On 7/9/20 9:34 AM, Zi Yan wrote:
> On 9 Jul 2020, at 11:34, Randy Dunlap wrote:
> 
>> Hi,
>>
>> I have a few comments on this.
>>
>> a. I reported it very early and should have been Cc-ed.
>>
>> b. A patch that applies to mmotm or linux-next would have been better
>> than a full replacement patch.
>>
>> c. I tried replacing what I believe is the correct/same patch file in mmotm
>> and still have build errors.
>>
>> (more below)
>>
>> On 7/9/20 2:39 AM, Anshuman Khandual wrote:
>>
>>> ---
>>> Applies on 5.8-rc4.
>>>
>>> Changes in V4:
>>>
>>> - Changed THP_MIGRATION_FAILURE as THP_MIGRATION_FAIL per John
>>> - Dropped all conditional 'if' blocks in migrate_pages() per Andrew and John
>>> - Updated migration events documentation per John
>>> - Updated thp_nr_pages variable as nr_subpages for an expected merge conflict
>>> - Moved all new THP vmstat events into CONFIG_MIGRATION
>>> - Updated Cc list with Documentation/ and tracing related addresses
>>>
>>> Changes in V3: (https://patchwork.kernel.org/patch/11647237/)
>>>
>>> - Formatted new events documentation with 'fmt' tool per Matthew
>>> - Made events universally available i.e dropped ARCH_ENABLE_THP_MIGRATION
>>> - Added THP_MIGRATION_SPLIT
>>> - Updated trace_mm_migrate_pages() with THP events
>>> - Made THP events update normal page migration events as well
>>>
>>> Changes in V2: (https://patchwork.kernel.org/patch/11586893/)
>>>
>>> - Dropped PMD reference both from code and commit message per Matthew
>>> - Added documentation and updated the commit message per Daniel
>>>
>>> Changes in V1: (https://patchwork.kernel.org/patch/11564497/)
>>>
>>> - Changed function name as thp_pmd_migration_success() per John
>>> - Folded in a fix (https://patchwork.kernel.org/patch/11563009/) from Hugh
>>>
>>> Changes in RFC V2: (https://patchwork.kernel.org/patch/11554861/)
>>>
>>> - Decopupled and renamed VM events from their implementation per Zi and John
>>> - Added THP_PMD_MIGRATION_FAILURE VM event upon allocation failure and split
>>>
>>> Changes in RFC V1: (https://patchwork.kernel.org/patch/11542055/)
>>>
>>>  Documentation/vm/page_migration.rst | 27 +++++++++++++++
>>>  include/linux/vm_event_item.h       |  3 ++
>>>  include/trace/events/migrate.h      | 17 ++++++++--
>>>  mm/migrate.c                        | 52 ++++++++++++++++++++++++-----
>>>  mm/vmstat.c                         |  3 ++
>>>  5 files changed, 91 insertions(+), 11 deletions(-)
>>>
>>
>>> diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
>>> index 24fc7c3ae7d6..2e6ca53b9bbd 100644
>>> --- a/include/linux/vm_event_item.h
>>> +++ b/include/linux/vm_event_item.h
>>> @@ -56,6 +56,9 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>>>  #endif
>>>  #ifdef CONFIG_MIGRATION
>>>  		PGMIGRATE_SUCCESS, PGMIGRATE_FAIL,
>>> +		THP_MIGRATION_SUCCESS,
>>> +		THP_MIGRATION_FAIL,
>>> +		THP_MIGRATION_SPLIT,
>>
>> These 3 new symbols are still only present if CONFIG_MIGRATION=y, but the build errors
>> are using these symbols even when CONFIG_MIGRATION is not set.
>>
>>>  #endif
>>>  #ifdef CONFIG_COMPACTION
>>>  		COMPACTMIGRATE_SCANNED, COMPACTFREE_SCANNED,
>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index f37729673558..c706e3576cfc 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1429,22 +1429,35 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>  		enum migrate_mode mode, int reason)
>>>  {
>>>  	int retry = 1;
>>> +	int thp_retry = 1;
>>>  	int nr_failed = 0;
>>>  	int nr_succeeded = 0;
>>> +	int nr_thp_succeeded = 0;
>>> +	int nr_thp_failed = 0;
>>> +	int nr_thp_split = 0;
>>>  	int pass = 0;
>>> +	bool is_thp = false;
>>>  	struct page *page;
>>>  	struct page *page2;
>>>  	int swapwrite = current->flags & PF_SWAPWRITE;
>>> -	int rc;
>>> +	int rc, nr_subpages;
>>>
>>>  	if (!swapwrite)
>>>  		current->flags |= PF_SWAPWRITE;
>>>
>>> -	for(pass = 0; pass < 10 && retry; pass++) {
>>> +	for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
>>>  		retry = 0;
>>> +		thp_retry = 0;
>>>
>>>  		list_for_each_entry_safe(page, page2, from, lru) {
>>>  retry:
>>> +			/*
>>> +			 * THP statistics is based on the source huge page.
>>> +			 * Capture required information that might get lost
>>> +			 * during migration.
>>> +			 */
>>> +			is_thp = PageTransHuge(page);
>>> +			nr_subpages = hpage_nr_pages(page);
>>>  			cond_resched();
>>>
>>>  			if (PageHuge(page))
>>> @@ -1475,15 +1488,30 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>  					unlock_page(page);
>>>  					if (!rc) {
>>>  						list_safe_reset_next(page, page2, lru);
>>> +						nr_thp_split++;
>>>  						goto retry;
>>>  					}
>>>  				}
>>> +				if (is_thp) {
>>> +					nr_thp_failed++;
>>> +					nr_failed += nr_subpages;
>>> +					goto out;
>>> +				}
>>>  				nr_failed++;
>>>  				goto out;
>>>  			case -EAGAIN:
>>> +				if (is_thp) {
>>> +					thp_retry++;
>>> +					break;
>>> +				}
>>>  				retry++;
>>>  				break;
>>>  			case MIGRATEPAGE_SUCCESS:
>>> +				if (is_thp) {
>>> +					nr_thp_succeeded++;
>>> +					nr_succeeded += nr_subpages;
>>> +					break;
>>> +				}
>>>  				nr_succeeded++;
>>>  				break;
>>>  			default:
>>> @@ -1493,19 +1521,27 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>  				 * removed from migration page list and not
>>>  				 * retried in the next outer loop.
>>>  				 */
>>> +				if (is_thp) {
>>> +					nr_thp_failed++;
>>> +					nr_failed += nr_subpages;
>>> +					break;
>>> +				}
>>>  				nr_failed++;
>>>  				break;
>>>  			}
>>>  		}
>>>  	}
>>> -	nr_failed += retry;
>>> +	nr_failed += retry + thp_retry;
>>> +	nr_thp_failed += thp_retry;
>>>  	rc = nr_failed;
>>>  out:
>>> -	if (nr_succeeded)
>>> -		count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>>> -	if (nr_failed)
>>> -		count_vm_events(PGMIGRATE_FAIL, nr_failed);
>>> -	trace_mm_migrate_pages(nr_succeeded, nr_failed, mode, reason);
>>> +	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>>> +	count_vm_events(PGMIGRATE_FAIL, nr_failed);
>>> +	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
>>> +	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
>>> +	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
>>
>> These references still cause build errors.
>>
>>> +	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
>>> +			       nr_thp_failed, nr_thp_split, mode, reason);
>>>
>>>  	if (!swapwrite)
>>>  		current->flags &= ~PF_SWAPWRITE;
>>> diff --git a/mm/vmstat.c b/mm/vmstat.c
>>> index 3fb23a21f6dd..09914a4bfee4 100644
>>> --- a/mm/vmstat.c
>>> +++ b/mm/vmstat.c
>>> @@ -1234,6 +1234,9 @@ const char * const vmstat_text[] = {
>>>  #ifdef CONFIG_MIGRATION
>>>  	"pgmigrate_success",
>>>  	"pgmigrate_fail",
>>> +	"thp_migration_success",
>>> +	"thp_migration_fail",
>>> +	"thp_migration_split",
>>>  #endif
>>>  #ifdef CONFIG_COMPACTION
>>>  	"compact_migrate_scanned",
>>>
>>
> 
> Which arch are you building? I did not see any error
> after applying this patch on mmotm (reverting the existing ones)
> and compiling them on x86_64. I used make x86_64_defconfig and
> unselected COMPACTION and MIGRATION.

Hi,

I am trying to build x86_64.
Maybe I am just having trouble replacing the patch file.
Like I tried to say, I would prefer to see an incremental patch
to fix mmotm or linux-next.


> mm/migrate.c and added vm events will not be used
> if CONFIG_MIGRATION is unchecked. Why would they cause compilation errors?

AFAICT, the patch adds these calls:
+	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
+	count_vm_events(PGMIGRATE_FAIL, nr_failed);
+	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
+	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
+	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
+	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
+			       nr_thp_failed, nr_thp_split, mode, reason);

even when CONFIG_MIGRATION is not set, but then the THP_MIGRATIION_SUCCESS/FAIL/SPLIT
symbols are not defined.

Zi Yan July 9, 2020, 5:42 p.m. UTC | #4

On 9 Jul 2020, at 12:39, Randy Dunlap wrote:

> On 7/9/20 9:34 AM, Zi Yan wrote:
>> On 9 Jul 2020, at 11:34, Randy Dunlap wrote:
>>
>>> Hi,
>>>
>>> I have a few comments on this.
>>>
>>> a. I reported it very early and should have been Cc-ed.
>>>
>>> b. A patch that applies to mmotm or linux-next would have been better
>>> than a full replacement patch.
>>>
>>> c. I tried replacing what I believe is the correct/same patch file in mmotm
>>> and still have build errors.
>>>
>>> (more below)
>>>
>>> On 7/9/20 2:39 AM, Anshuman Khandual wrote:
>>>
>>>> ---
>>>> Applies on 5.8-rc4.
>>>>
>>>> Changes in V4:
>>>>
>>>> - Changed THP_MIGRATION_FAILURE as THP_MIGRATION_FAIL per John
>>>> - Dropped all conditional 'if' blocks in migrate_pages() per Andrew and John
>>>> - Updated migration events documentation per John
>>>> - Updated thp_nr_pages variable as nr_subpages for an expected merge conflict
>>>> - Moved all new THP vmstat events into CONFIG_MIGRATION
>>>> - Updated Cc list with Documentation/ and tracing related addresses
>>>>
>>>> Changes in V3: (https://patchwork.kernel.org/patch/11647237/)
>>>>
>>>> - Formatted new events documentation with 'fmt' tool per Matthew
>>>> - Made events universally available i.e dropped ARCH_ENABLE_THP_MIGRATION
>>>> - Added THP_MIGRATION_SPLIT
>>>> - Updated trace_mm_migrate_pages() with THP events
>>>> - Made THP events update normal page migration events as well
>>>>
>>>> Changes in V2: (https://patchwork.kernel.org/patch/11586893/)
>>>>
>>>> - Dropped PMD reference both from code and commit message per Matthew
>>>> - Added documentation and updated the commit message per Daniel
>>>>
>>>> Changes in V1: (https://patchwork.kernel.org/patch/11564497/)
>>>>
>>>> - Changed function name as thp_pmd_migration_success() per John
>>>> - Folded in a fix (https://patchwork.kernel.org/patch/11563009/) from Hugh
>>>>
>>>> Changes in RFC V2: (https://patchwork.kernel.org/patch/11554861/)
>>>>
>>>> - Decopupled and renamed VM events from their implementation per Zi and John
>>>> - Added THP_PMD_MIGRATION_FAILURE VM event upon allocation failure and split
>>>>
>>>> Changes in RFC V1: (https://patchwork.kernel.org/patch/11542055/)
>>>>
>>>>  Documentation/vm/page_migration.rst | 27 +++++++++++++++
>>>>  include/linux/vm_event_item.h       |  3 ++
>>>>  include/trace/events/migrate.h      | 17 ++++++++--
>>>>  mm/migrate.c                        | 52 ++++++++++++++++++++++++-----
>>>>  mm/vmstat.c                         |  3 ++
>>>>  5 files changed, 91 insertions(+), 11 deletions(-)
>>>>
>>>
>>>> diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
>>>> index 24fc7c3ae7d6..2e6ca53b9bbd 100644
>>>> --- a/include/linux/vm_event_item.h
>>>> +++ b/include/linux/vm_event_item.h
>>>> @@ -56,6 +56,9 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>>>>  #endif
>>>>  #ifdef CONFIG_MIGRATION
>>>>  		PGMIGRATE_SUCCESS, PGMIGRATE_FAIL,
>>>> +		THP_MIGRATION_SUCCESS,
>>>> +		THP_MIGRATION_FAIL,
>>>> +		THP_MIGRATION_SPLIT,
>>>
>>> These 3 new symbols are still only present if CONFIG_MIGRATION=y, but the build errors
>>> are using these symbols even when CONFIG_MIGRATION is not set.
>>>
>>>>  #endif
>>>>  #ifdef CONFIG_COMPACTION
>>>>  		COMPACTMIGRATE_SCANNED, COMPACTFREE_SCANNED,
>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index f37729673558..c706e3576cfc 100644
>>>> --- a/mm/migrate.c
>>>> +++ b/mm/migrate.c
>>>> @@ -1429,22 +1429,35 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>  		enum migrate_mode mode, int reason)
>>>>  {
>>>>  	int retry = 1;
>>>> +	int thp_retry = 1;
>>>>  	int nr_failed = 0;
>>>>  	int nr_succeeded = 0;
>>>> +	int nr_thp_succeeded = 0;
>>>> +	int nr_thp_failed = 0;
>>>> +	int nr_thp_split = 0;
>>>>  	int pass = 0;
>>>> +	bool is_thp = false;
>>>>  	struct page *page;
>>>>  	struct page *page2;
>>>>  	int swapwrite = current->flags & PF_SWAPWRITE;
>>>> -	int rc;
>>>> +	int rc, nr_subpages;
>>>>
>>>>  	if (!swapwrite)
>>>>  		current->flags |= PF_SWAPWRITE;
>>>>
>>>> -	for(pass = 0; pass < 10 && retry; pass++) {
>>>> +	for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
>>>>  		retry = 0;
>>>> +		thp_retry = 0;
>>>>
>>>>  		list_for_each_entry_safe(page, page2, from, lru) {
>>>>  retry:
>>>> +			/*
>>>> +			 * THP statistics is based on the source huge page.
>>>> +			 * Capture required information that might get lost
>>>> +			 * during migration.
>>>> +			 */
>>>> +			is_thp = PageTransHuge(page);
>>>> +			nr_subpages = hpage_nr_pages(page);
>>>>  			cond_resched();
>>>>
>>>>  			if (PageHuge(page))
>>>> @@ -1475,15 +1488,30 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>  					unlock_page(page);
>>>>  					if (!rc) {
>>>>  						list_safe_reset_next(page, page2, lru);
>>>> +						nr_thp_split++;
>>>>  						goto retry;
>>>>  					}
>>>>  				}
>>>> +				if (is_thp) {
>>>> +					nr_thp_failed++;
>>>> +					nr_failed += nr_subpages;
>>>> +					goto out;
>>>> +				}
>>>>  				nr_failed++;
>>>>  				goto out;
>>>>  			case -EAGAIN:
>>>> +				if (is_thp) {
>>>> +					thp_retry++;
>>>> +					break;
>>>> +				}
>>>>  				retry++;
>>>>  				break;
>>>>  			case MIGRATEPAGE_SUCCESS:
>>>> +				if (is_thp) {
>>>> +					nr_thp_succeeded++;
>>>> +					nr_succeeded += nr_subpages;
>>>> +					break;
>>>> +				}
>>>>  				nr_succeeded++;
>>>>  				break;
>>>>  			default:
>>>> @@ -1493,19 +1521,27 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>  				 * removed from migration page list and not
>>>>  				 * retried in the next outer loop.
>>>>  				 */
>>>> +				if (is_thp) {
>>>> +					nr_thp_failed++;
>>>> +					nr_failed += nr_subpages;
>>>> +					break;
>>>> +				}
>>>>  				nr_failed++;
>>>>  				break;
>>>>  			}
>>>>  		}
>>>>  	}
>>>> -	nr_failed += retry;
>>>> +	nr_failed += retry + thp_retry;
>>>> +	nr_thp_failed += thp_retry;
>>>>  	rc = nr_failed;
>>>>  out:
>>>> -	if (nr_succeeded)
>>>> -		count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>>>> -	if (nr_failed)
>>>> -		count_vm_events(PGMIGRATE_FAIL, nr_failed);
>>>> -	trace_mm_migrate_pages(nr_succeeded, nr_failed, mode, reason);
>>>> +	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>>>> +	count_vm_events(PGMIGRATE_FAIL, nr_failed);
>>>> +	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
>>>> +	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
>>>> +	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
>>>
>>> These references still cause build errors.
>>>
>>>> +	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
>>>> +			       nr_thp_failed, nr_thp_split, mode, reason);
>>>>
>>>>  	if (!swapwrite)
>>>>  		current->flags &= ~PF_SWAPWRITE;
>>>> diff --git a/mm/vmstat.c b/mm/vmstat.c
>>>> index 3fb23a21f6dd..09914a4bfee4 100644
>>>> --- a/mm/vmstat.c
>>>> +++ b/mm/vmstat.c
>>>> @@ -1234,6 +1234,9 @@ const char * const vmstat_text[] = {
>>>>  #ifdef CONFIG_MIGRATION
>>>>  	"pgmigrate_success",
>>>>  	"pgmigrate_fail",
>>>> +	"thp_migration_success",
>>>> +	"thp_migration_fail",
>>>> +	"thp_migration_split",
>>>>  #endif
>>>>  #ifdef CONFIG_COMPACTION
>>>>  	"compact_migrate_scanned",
>>>>
>>>
>>
>> Which arch are you building? I did not see any error
>> after applying this patch on mmotm (reverting the existing ones)
>> and compiling them on x86_64. I used make x86_64_defconfig and
>> unselected COMPACTION and MIGRATION.
>
> Hi,
>
> I am trying to build x86_64.
> Maybe I am just having trouble replacing the patch file.
> Like I tried to say, I would prefer to see an incremental patch
> to fix mmotm or linux-next.

I agree. The patch does not apply to mmotm. Can you try the incremental
patch below? It should apply to mmotm.


Hi Andrew and Anshuman,

Should Anshuman resend the incremental patch or Andrew can fold it along
with the two patches in mmotm into one?


diff --git a/Documentation/vm/page_migration.rst b/Documentation/vm/page_migration.rst
index e65d49f3cf86..68883ac485fa 100644
--- a/Documentation/vm/page_migration.rst
+++ b/Documentation/vm/page_migration.rst
@@ -253,24 +253,32 @@ which are function pointers of struct address_space_operations.
      PG_isolated is alias with PG_reclaim flag so driver shouldn't use the flag
      for own purpose.

-Quantifying Migration
+Monitoring Migration
 =====================
-Following events can be used to quantify page migration.
-
-1. PGMIGRATE_SUCCESS       /* Normal page migration success */
-2. PGMIGRATE_FAIL          /* Normal page migration failure */
-3. THP_MIGRATION_SUCCESS   /* Transparent huge page migration success */
-4. THP_MIGRATION_FAILURE   /* Transparent huge page migration failure */
-5. THP_MIGRATION_SPLIT     /* Transparent huge page got split, retried */
-
-THP_MIGRATION_SUCCESS is when THP is migrated successfully without getting
-split into it's subpages. THP_MIGRATION_FAILURE is when THP could neither
-be migrated nor be split. THP_MIGRATION_SPLIT is when THP could not
-just be migrated as is but instead get split into it's subpages and later
-retried as normal pages. THP events would also update normal page migration
-statistics PGMIGRATE_SUCCESS and PGMIGRATE_FAILURE. These events will help
-in quantifying and analyzing various THP migration events including both
-success and failure cases.
+
+The following events (counters) can be used to monitor page migration.
+
+1. PGMIGRATE_SUCCESS: Normal page migration success. Each count means that a
+   page was migrated. If the page was a non-THP page, then this counter is
+   increased by one. If the page was a THP, then this counter is increased by
+   the number of THP subpages. For example, migration of a single 2MB THP that
+   has 4KB-size base pages (subpages) will cause this counter to increase by
+   512.
+
+2. PGMIGRATE_FAIL: Normal page migration failure. Same counting rules as for
+   _SUCCESS, above: this will be increased by the number of subpages, if it was
+   a THP.
+
+3. THP_MIGRATION_SUCCESS: A THP was migrated without being split.
+
+4. THP_MIGRATION_FAIL: A THP could not be migrated nor it could be split.
+
+5. THP_MIGRATION_SPLIT: A THP was migrated, but not as such: first, the THP had
+   to be split. After splitting, a migration retry was used for it's sub-pages.
+
+THP_MIGRATION_* events also update the appropriate PGMIGRATE_SUCCESS or
+PGMIGRATE_FAIL events. For example, a THP migration failure will cause both
+THP_MIGRATION_FAIL and PGMIGRATE_FAIL to increase.

 Christoph Lameter, May 8, 2006.
 Minchan Kim, Mar 28, 2016.
diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
index 5e7ffa025589..2e6ca53b9bbd 100644
--- a/include/linux/vm_event_item.h
+++ b/include/linux/vm_event_item.h
@@ -56,6 +56,9 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
 #endif
 #ifdef CONFIG_MIGRATION
 		PGMIGRATE_SUCCESS, PGMIGRATE_FAIL,
+		THP_MIGRATION_SUCCESS,
+		THP_MIGRATION_FAIL,
+		THP_MIGRATION_SPLIT,
 #endif
 #ifdef CONFIG_COMPACTION
 		COMPACTMIGRATE_SCANNED, COMPACTFREE_SCANNED,
@@ -95,9 +98,6 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
 		THP_ZERO_PAGE_ALLOC_FAILED,
 		THP_SWPOUT,
 		THP_SWPOUT_FALLBACK,
-		THP_MIGRATION_SUCCESS,
-		THP_MIGRATION_FAILURE,
-		THP_MIGRATION_SPLIT,
 #endif
 #ifdef CONFIG_MEMORY_BALLOON
 		BALLOON_INFLATE,
diff --git a/mm/migrate.c b/mm/migrate.c
index b0125c082549..c6cb8e676f9d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1425,7 +1425,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 	struct page *page;
 	struct page *page2;
 	int swapwrite = current->flags & PF_SWAPWRITE;
-	int rc, thp_n_pages;
+	int rc, nr_subpages;

 	if (!swapwrite)
 		current->flags |= PF_SWAPWRITE;
@@ -1442,7 +1442,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 			 * during migration.
 			 */
 			is_thp = PageTransHuge(page);
-			thp_n_pages = thp_nr_pages(page);
+			nr_subpages = thp_nr_pages(page);
 			cond_resched();

 			if (PageHuge(page))
@@ -1479,7 +1479,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				}
 				if (is_thp) {
 					nr_thp_failed++;
-					nr_failed += thp_n_pages;
+					nr_failed += nr_subpages;
 					goto out;
 				}
 				nr_failed++;
@@ -1494,7 +1494,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 			case MIGRATEPAGE_SUCCESS:
 				if (is_thp) {
 					nr_thp_succeeded++;
-					nr_succeeded += thp_n_pages;
+					nr_succeeded += nr_subpages;
 					break;
 				}
 				nr_succeeded++;
@@ -1508,7 +1508,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				 */
 				if (is_thp) {
 					nr_thp_failed++;
-					nr_failed += thp_n_pages;
+					nr_failed += nr_subpages;
 					break;
 				}
 				nr_failed++;
@@ -1520,16 +1520,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 	nr_thp_failed += thp_retry;
 	rc = nr_failed;
 out:
-	if (nr_succeeded)
-		count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
-	if (nr_failed)
-		count_vm_events(PGMIGRATE_FAIL, nr_failed);
-	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);
+	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
+	count_vm_events(PGMIGRATE_FAIL, nr_failed);
+	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
+	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
+	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
 	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
 			       nr_thp_failed, nr_thp_split, mode, reason);

diff --git a/mm/vmstat.c b/mm/vmstat.c
index 9892090df6a2..a21140373edb 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1274,6 +1274,9 @@ const char * const vmstat_text[] = {
 #ifdef CONFIG_MIGRATION
 	"pgmigrate_success",
 	"pgmigrate_fail",
+	"thp_migration_success",
+	"thp_migration_fail",
+	"thp_migration_split",
 #endif
 #ifdef CONFIG_COMPACTION
 	"compact_migrate_scanned",
@@ -1320,9 +1323,6 @@ const char * const vmstat_text[] = {
 	"thp_zero_page_alloc_failed",
 	"thp_swpout",
 	"thp_swpout_fallback",
-	"thp_migration_success",
-	"thp_migration_failure",
-	"thp_migration_split",
 #endif
 #ifdef CONFIG_MEMORY_BALLOON
 	"balloon_inflate",




—
Best Regards,
Yan Zi

John Hubbard July 9, 2020, 8:15 p.m. UTC | #5

On 2020-07-09 10:42, Zi Yan wrote:
...
> diff --git a/Documentation/vm/page_migration.rst b/Documentation/vm/page_migration.rst
> index e65d49f3cf86..68883ac485fa 100644
> --- a/Documentation/vm/page_migration.rst
> +++ b/Documentation/vm/page_migration.rst
> @@ -253,24 +253,32 @@ which are function pointers of struct address_space_operations.
>        PG_isolated is alias with PG_reclaim flag so driver shouldn't use the flag
>        for own purpose.
> 
> -Quantifying Migration
> +Monitoring Migration
>   =====================
> -Following events can be used to quantify page migration.
> -
> -1. PGMIGRATE_SUCCESS       /* Normal page migration success */
> -2. PGMIGRATE_FAIL          /* Normal page migration failure */
> -3. THP_MIGRATION_SUCCESS   /* Transparent huge page migration success */
> -4. THP_MIGRATION_FAILURE   /* Transparent huge page migration failure */
> -5. THP_MIGRATION_SPLIT     /* Transparent huge page got split, retried */
> -
> -THP_MIGRATION_SUCCESS is when THP is migrated successfully without getting
> -split into it's subpages. THP_MIGRATION_FAILURE is when THP could neither
> -be migrated nor be split. THP_MIGRATION_SPLIT is when THP could not
> -just be migrated as is but instead get split into it's subpages and later
> -retried as normal pages. THP events would also update normal page migration
> -statistics PGMIGRATE_SUCCESS and PGMIGRATE_FAILURE. These events will help
> -in quantifying and analyzing various THP migration events including both
> -success and failure cases.
> +
> +The following events (counters) can be used to monitor page migration.
> +
> +1. PGMIGRATE_SUCCESS: Normal page migration success. Each count means that a
> +   page was migrated. If the page was a non-THP page, then this counter is
> +   increased by one. If the page was a THP, then this counter is increased by
> +   the number of THP subpages. For example, migration of a single 2MB THP that
> +   has 4KB-size base pages (subpages) will cause this counter to increase by
> +   512.
> +
> +2. PGMIGRATE_FAIL: Normal page migration failure. Same counting rules as for
> +   _SUCCESS, above: this will be increased by the number of subpages, if it was
> +   a THP.
> +
> +3. THP_MIGRATION_SUCCESS: A THP was migrated without being split.
> +
> +4. THP_MIGRATION_FAIL: A THP could not be migrated nor it could be split.
> +
> +5. THP_MIGRATION_SPLIT: A THP was migrated, but not as such: first, the THP had
> +   to be split. After splitting, a migration retry was used for it's sub-pages.


Just a documentation nit: "its sub-pages", not "it's sub-pages".


thanks,

Randy Dunlap July 9, 2020, 9:24 p.m. UTC | #6

On 7/9/20 10:42 AM, Zi Yan wrote:

> 
> I agree. The patch does not apply to mmotm. Can you try the incremental
> patch below? It should apply to mmotm.
> 
> 
> Hi Andrew and Anshuman,
> 
> Should Anshuman resend the incremental patch or Andrew can fold it along
> with the two patches in mmotm into one?
> 
> 
> diff --git a/Documentation/vm/page_migration.rst b/Documentation/vm/page_migration.rst
> index e65d49f3cf86..68883ac485fa 100644
> --- a/Documentation/vm/page_migration.rst
> +++ b/Documentation/vm/page_migration.rst
> @@ -253,24 +253,32 @@ which are function pointers of struct address_space_operations.
>       PG_isolated is alias with PG_reclaim flag so driver shouldn't use the flag
>       for own purpose.
> 
> -Quantifying Migration
> +Monitoring Migration
>  =====================
> -Following events can be used to quantify page migration.
> -
> -1. PGMIGRATE_SUCCESS       /* Normal page migration success */
> -2. PGMIGRATE_FAIL          /* Normal page migration failure */
> -3. THP_MIGRATION_SUCCESS   /* Transparent huge page migration success */
> -4. THP_MIGRATION_FAILURE   /* Transparent huge page migration failure */
> -5. THP_MIGRATION_SPLIT     /* Transparent huge page got split, retried */
> -
> -THP_MIGRATION_SUCCESS is when THP is migrated successfully without getting
> -split into it's subpages. THP_MIGRATION_FAILURE is when THP could neither
> -be migrated nor be split. THP_MIGRATION_SPLIT is when THP could not
> -just be migrated as is but instead get split into it's subpages and later
> -retried as normal pages. THP events would also update normal page migration
> -statistics PGMIGRATE_SUCCESS and PGMIGRATE_FAILURE. These events will help
> -in quantifying and analyzing various THP migration events including both
> -success and failure cases.
> +
> +The following events (counters) can be used to monitor page migration.
> +
> +1. PGMIGRATE_SUCCESS: Normal page migration success. Each count means that a
> +   page was migrated. If the page was a non-THP page, then this counter is
> +   increased by one. If the page was a THP, then this counter is increased by
> +   the number of THP subpages. For example, migration of a single 2MB THP that
> +   has 4KB-size base pages (subpages) will cause this counter to increase by
> +   512.
> +
> +2. PGMIGRATE_FAIL: Normal page migration failure. Same counting rules as for
> +   _SUCCESS, above: this will be increased by the number of subpages, if it was
> +   a THP.
> +
> +3. THP_MIGRATION_SUCCESS: A THP was migrated without being split.
> +
> +4. THP_MIGRATION_FAIL: A THP could not be migrated nor it could be split.
> +
> +5. THP_MIGRATION_SPLIT: A THP was migrated, but not as such: first, the THP had
> +   to be split. After splitting, a migration retry was used for it's sub-pages.
> +
> +THP_MIGRATION_* events also update the appropriate PGMIGRATE_SUCCESS or
> +PGMIGRATE_FAIL events. For example, a THP migration failure will cause both
> +THP_MIGRATION_FAIL and PGMIGRATE_FAIL to increase.
> 
>  Christoph Lameter, May 8, 2006.
>  Minchan Kim, Mar 28, 2016.
> diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
> index 5e7ffa025589..2e6ca53b9bbd 100644
> --- a/include/linux/vm_event_item.h
> +++ b/include/linux/vm_event_item.h
> @@ -56,6 +56,9 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>  #endif
>  #ifdef CONFIG_MIGRATION
>  		PGMIGRATE_SUCCESS, PGMIGRATE_FAIL,
> +		THP_MIGRATION_SUCCESS,
> +		THP_MIGRATION_FAIL,
> +		THP_MIGRATION_SPLIT,
>  #endif
>  #ifdef CONFIG_COMPACTION
>  		COMPACTMIGRATE_SCANNED, COMPACTFREE_SCANNED,
> @@ -95,9 +98,6 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>  		THP_ZERO_PAGE_ALLOC_FAILED,
>  		THP_SWPOUT,
>  		THP_SWPOUT_FALLBACK,
> -		THP_MIGRATION_SUCCESS,
> -		THP_MIGRATION_FAILURE,
> -		THP_MIGRATION_SPLIT,
>  #endif
>  #ifdef CONFIG_MEMORY_BALLOON
>  		BALLOON_INFLATE,
> diff --git a/mm/migrate.c b/mm/migrate.c
> index b0125c082549..c6cb8e676f9d 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1425,7 +1425,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  	struct page *page;
>  	struct page *page2;
>  	int swapwrite = current->flags & PF_SWAPWRITE;
> -	int rc, thp_n_pages;
> +	int rc, nr_subpages;
> 
>  	if (!swapwrite)
>  		current->flags |= PF_SWAPWRITE;
> @@ -1442,7 +1442,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  			 * during migration.
>  			 */
>  			is_thp = PageTransHuge(page);
> -			thp_n_pages = thp_nr_pages(page);
> +			nr_subpages = thp_nr_pages(page);
>  			cond_resched();
> 
>  			if (PageHuge(page))
> @@ -1479,7 +1479,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  				}
>  				if (is_thp) {
>  					nr_thp_failed++;
> -					nr_failed += thp_n_pages;
> +					nr_failed += nr_subpages;
>  					goto out;
>  				}
>  				nr_failed++;
> @@ -1494,7 +1494,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  			case MIGRATEPAGE_SUCCESS:
>  				if (is_thp) {
>  					nr_thp_succeeded++;
> -					nr_succeeded += thp_n_pages;
> +					nr_succeeded += nr_subpages;
>  					break;
>  				}
>  				nr_succeeded++;
> @@ -1508,7 +1508,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  				 */
>  				if (is_thp) {
>  					nr_thp_failed++;
> -					nr_failed += thp_n_pages;
> +					nr_failed += nr_subpages;
>  					break;
>  				}
>  				nr_failed++;
> @@ -1520,16 +1520,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  	nr_thp_failed += thp_retry;
>  	rc = nr_failed;
>  out:
> -	if (nr_succeeded)
> -		count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
> -	if (nr_failed)
> -		count_vm_events(PGMIGRATE_FAIL, nr_failed);
> -	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);
> +	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
> +	count_vm_events(PGMIGRATE_FAIL, nr_failed);
> +	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
> +	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
> +	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
>  	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
>  			       nr_thp_failed, nr_thp_split, mode, reason);
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 9892090df6a2..a21140373edb 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1274,6 +1274,9 @@ const char * const vmstat_text[] = {
>  #ifdef CONFIG_MIGRATION
>  	"pgmigrate_success",
>  	"pgmigrate_fail",
> +	"thp_migration_success",
> +	"thp_migration_fail",
> +	"thp_migration_split",
>  #endif
>  #ifdef CONFIG_COMPACTION
>  	"compact_migrate_scanned",
> @@ -1320,9 +1323,6 @@ const char * const vmstat_text[] = {
>  	"thp_zero_page_alloc_failed",
>  	"thp_swpout",
>  	"thp_swpout_fallback",
> -	"thp_migration_success",
> -	"thp_migration_failure",
> -	"thp_migration_split",
>  #endif
>  #ifdef CONFIG_MEMORY_BALLOON
>  	"balloon_inflate",

Hi,

Thanks for the patch.
Works for me.

Anshuman Khandual July 10, 2020, 3:30 a.m. UTC | #7

On 07/09/2020 09:04 PM, Randy Dunlap wrote:
> Hi,
> 
> I have a few comments on this.
> 
> a. I reported it very early and should have been Cc-ed.

I should have Cc-ed you on this V4 patch, sorry about that.

> 
> b. A patch that applies to mmotm or linux-next would have been better
> than a full replacement patch.
I have followed that (i.e patch on mmotm/next as fix) only when the
required change is smaller as compared to the series on mmotm/next.
But for others a new patch should be better which can be replaced
on mmotm and next. At least that is my understanding and would like
to be corrected otherwise.

> 
> c. I tried replacing what I believe is the correct/same patch file in mmotm
> and still have build errors.

That should not have happened, all new THP migration events are with
CONFIG_MIGRATION rather than CONFIG_TRANSPARENT_HUGEPAGE previously.

Randy Dunlap July 10, 2020, 3:33 a.m. UTC | #8

On 7/9/20 8:30 PM, Anshuman Khandual wrote:
> 
> On 07/09/2020 09:04 PM, Randy Dunlap wrote:
>> Hi,
>>
>> I have a few comments on this.
>>
>> a. I reported it very early and should have been Cc-ed.
> 
> I should have Cc-ed you on this V4 patch, sorry about that.
> 
>>
>> b. A patch that applies to mmotm or linux-next would have been better
>> than a full replacement patch.
> I have followed that (i.e patch on mmotm/next as fix) only when the
> required change is smaller as compared to the series on mmotm/next.
> But for others a new patch should be better which can be replaced
> on mmotm and next. At least that is my understanding and would like
> to be corrected otherwise.
> 
>>
>> c. I tried replacing what I believe is the correct/same patch file in mmotm
>> and still have build errors.
> 
> That should not have happened, all new THP migration events are with
> CONFIG_MIGRATION rather than CONFIG_TRANSPARENT_HUGEPAGE previously.

Yes, I could have been mistaken about that last part.  Sorry about that.

Anshuman Khandual July 10, 2020, 3:41 a.m. UTC | #9

On 07/09/2020 11:12 PM, Zi Yan wrote:
> On 9 Jul 2020, at 12:39, Randy Dunlap wrote:
> 
>> On 7/9/20 9:34 AM, Zi Yan wrote:
>>> On 9 Jul 2020, at 11:34, Randy Dunlap wrote:
>>>
>>>> Hi,
>>>>
>>>> I have a few comments on this.
>>>>
>>>> a. I reported it very early and should have been Cc-ed.
>>>>
>>>> b. A patch that applies to mmotm or linux-next would have been better
>>>> than a full replacement patch.
>>>>
>>>> c. I tried replacing what I believe is the correct/same patch file in mmotm
>>>> and still have build errors.
>>>>
>>>> (more below)
>>>>
>>>> On 7/9/20 2:39 AM, Anshuman Khandual wrote:
>>>>
>>>>> ---
>>>>> Applies on 5.8-rc4.
>>>>>
>>>>> Changes in V4:
>>>>>
>>>>> - Changed THP_MIGRATION_FAILURE as THP_MIGRATION_FAIL per John
>>>>> - Dropped all conditional 'if' blocks in migrate_pages() per Andrew and John
>>>>> - Updated migration events documentation per John
>>>>> - Updated thp_nr_pages variable as nr_subpages for an expected merge conflict
>>>>> - Moved all new THP vmstat events into CONFIG_MIGRATION
>>>>> - Updated Cc list with Documentation/ and tracing related addresses
>>>>>
>>>>> Changes in V3: (https://patchwork.kernel.org/patch/11647237/)
>>>>>
>>>>> - Formatted new events documentation with 'fmt' tool per Matthew
>>>>> - Made events universally available i.e dropped ARCH_ENABLE_THP_MIGRATION
>>>>> - Added THP_MIGRATION_SPLIT
>>>>> - Updated trace_mm_migrate_pages() with THP events
>>>>> - Made THP events update normal page migration events as well
>>>>>
>>>>> Changes in V2: (https://patchwork.kernel.org/patch/11586893/)
>>>>>
>>>>> - Dropped PMD reference both from code and commit message per Matthew
>>>>> - Added documentation and updated the commit message per Daniel
>>>>>
>>>>> Changes in V1: (https://patchwork.kernel.org/patch/11564497/)
>>>>>
>>>>> - Changed function name as thp_pmd_migration_success() per John
>>>>> - Folded in a fix (https://patchwork.kernel.org/patch/11563009/) from Hugh
>>>>>
>>>>> Changes in RFC V2: (https://patchwork.kernel.org/patch/11554861/)
>>>>>
>>>>> - Decopupled and renamed VM events from their implementation per Zi and John
>>>>> - Added THP_PMD_MIGRATION_FAILURE VM event upon allocation failure and split
>>>>>
>>>>> Changes in RFC V1: (https://patchwork.kernel.org/patch/11542055/)
>>>>>
>>>>>  Documentation/vm/page_migration.rst | 27 +++++++++++++++
>>>>>  include/linux/vm_event_item.h       |  3 ++
>>>>>  include/trace/events/migrate.h      | 17 ++++++++--
>>>>>  mm/migrate.c                        | 52 ++++++++++++++++++++++++-----
>>>>>  mm/vmstat.c                         |  3 ++
>>>>>  5 files changed, 91 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
>>>>> index 24fc7c3ae7d6..2e6ca53b9bbd 100644
>>>>> --- a/include/linux/vm_event_item.h
>>>>> +++ b/include/linux/vm_event_item.h
>>>>> @@ -56,6 +56,9 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
>>>>>  #endif
>>>>>  #ifdef CONFIG_MIGRATION
>>>>>  		PGMIGRATE_SUCCESS, PGMIGRATE_FAIL,
>>>>> +		THP_MIGRATION_SUCCESS,
>>>>> +		THP_MIGRATION_FAIL,
>>>>> +		THP_MIGRATION_SPLIT,
>>>> These 3 new symbols are still only present if CONFIG_MIGRATION=y, but the build errors
>>>> are using these symbols even when CONFIG_MIGRATION is not set.
>>>>
>>>>>  #endif
>>>>>  #ifdef CONFIG_COMPACTION
>>>>>  		COMPACTMIGRATE_SCANNED, COMPACTFREE_SCANNED,
>>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>>> index f37729673558..c706e3576cfc 100644
>>>>> --- a/mm/migrate.c
>>>>> +++ b/mm/migrate.c
>>>>> @@ -1429,22 +1429,35 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>>  		enum migrate_mode mode, int reason)
>>>>>  {
>>>>>  	int retry = 1;
>>>>> +	int thp_retry = 1;
>>>>>  	int nr_failed = 0;
>>>>>  	int nr_succeeded = 0;
>>>>> +	int nr_thp_succeeded = 0;
>>>>> +	int nr_thp_failed = 0;
>>>>> +	int nr_thp_split = 0;
>>>>>  	int pass = 0;
>>>>> +	bool is_thp = false;
>>>>>  	struct page *page;
>>>>>  	struct page *page2;
>>>>>  	int swapwrite = current->flags & PF_SWAPWRITE;
>>>>> -	int rc;
>>>>> +	int rc, nr_subpages;
>>>>>
>>>>>  	if (!swapwrite)
>>>>>  		current->flags |= PF_SWAPWRITE;
>>>>>
>>>>> -	for(pass = 0; pass < 10 && retry; pass++) {
>>>>> +	for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
>>>>>  		retry = 0;
>>>>> +		thp_retry = 0;
>>>>>
>>>>>  		list_for_each_entry_safe(page, page2, from, lru) {
>>>>>  retry:
>>>>> +			/*
>>>>> +			 * THP statistics is based on the source huge page.
>>>>> +			 * Capture required information that might get lost
>>>>> +			 * during migration.
>>>>> +			 */
>>>>> +			is_thp = PageTransHuge(page);
>>>>> +			nr_subpages = hpage_nr_pages(page);
>>>>>  			cond_resched();
>>>>>
>>>>>  			if (PageHuge(page))
>>>>> @@ -1475,15 +1488,30 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>>  					unlock_page(page);
>>>>>  					if (!rc) {
>>>>>  						list_safe_reset_next(page, page2, lru);
>>>>> +						nr_thp_split++;
>>>>>  						goto retry;
>>>>>  					}
>>>>>  				}
>>>>> +				if (is_thp) {
>>>>> +					nr_thp_failed++;
>>>>> +					nr_failed += nr_subpages;
>>>>> +					goto out;
>>>>> +				}
>>>>>  				nr_failed++;
>>>>>  				goto out;
>>>>>  			case -EAGAIN:
>>>>> +				if (is_thp) {
>>>>> +					thp_retry++;
>>>>> +					break;
>>>>> +				}
>>>>>  				retry++;
>>>>>  				break;
>>>>>  			case MIGRATEPAGE_SUCCESS:
>>>>> +				if (is_thp) {
>>>>> +					nr_thp_succeeded++;
>>>>> +					nr_succeeded += nr_subpages;
>>>>> +					break;
>>>>> +				}
>>>>>  				nr_succeeded++;
>>>>>  				break;
>>>>>  			default:
>>>>> @@ -1493,19 +1521,27 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>>  				 * removed from migration page list and not
>>>>>  				 * retried in the next outer loop.
>>>>>  				 */
>>>>> +				if (is_thp) {
>>>>> +					nr_thp_failed++;
>>>>> +					nr_failed += nr_subpages;
>>>>> +					break;
>>>>> +				}
>>>>>  				nr_failed++;
>>>>>  				break;
>>>>>  			}
>>>>>  		}
>>>>>  	}
>>>>> -	nr_failed += retry;
>>>>> +	nr_failed += retry + thp_retry;
>>>>> +	nr_thp_failed += thp_retry;
>>>>>  	rc = nr_failed;
>>>>>  out:
>>>>> -	if (nr_succeeded)
>>>>> -		count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>>>>> -	if (nr_failed)
>>>>> -		count_vm_events(PGMIGRATE_FAIL, nr_failed);
>>>>> -	trace_mm_migrate_pages(nr_succeeded, nr_failed, mode, reason);
>>>>> +	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>>>>> +	count_vm_events(PGMIGRATE_FAIL, nr_failed);
>>>>> +	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
>>>>> +	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
>>>>> +	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
>>>> These references still cause build errors.
>>>>
>>>>> +	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
>>>>> +			       nr_thp_failed, nr_thp_split, mode, reason);
>>>>>
>>>>>  	if (!swapwrite)
>>>>>  		current->flags &= ~PF_SWAPWRITE;
>>>>> diff --git a/mm/vmstat.c b/mm/vmstat.c
>>>>> index 3fb23a21f6dd..09914a4bfee4 100644
>>>>> --- a/mm/vmstat.c
>>>>> +++ b/mm/vmstat.c
>>>>> @@ -1234,6 +1234,9 @@ const char * const vmstat_text[] = {
>>>>>  #ifdef CONFIG_MIGRATION
>>>>>  	"pgmigrate_success",
>>>>>  	"pgmigrate_fail",
>>>>> +	"thp_migration_success",
>>>>> +	"thp_migration_fail",
>>>>> +	"thp_migration_split",
>>>>>  #endif
>>>>>  #ifdef CONFIG_COMPACTION
>>>>>  	"compact_migrate_scanned",
>>>>>
>>> Which arch are you building? I did not see any error
>>> after applying this patch on mmotm (reverting the existing ones)
>>> and compiling them on x86_64. I used make x86_64_defconfig and
>>> unselected COMPACTION and MIGRATION.
>> Hi,
>>
>> I am trying to build x86_64.
>> Maybe I am just having trouble replacing the patch file.
>> Like I tried to say, I would prefer to see an incremental patch
>> to fix mmotm or linux-next.
> I agree. The patch does not apply to mmotm. Can you try the incremental
> patch below? It should apply to mmotm.
> 
> 
> Hi Andrew and Anshuman,
> 
> Should Anshuman resend the incremental patch or Andrew can fold it along
> with the two patches in mmotm into one?

V3 patch and fixes on it were reverted from linux-next yesterday. The change
for V4 patch was not small, hence just went with a fresh one instead. Please
do let me know if this is still not settled and needs further action.

https://lore.kernel.org/linux-next/[e-mail]/T/#u

Daniel Jordan July 24, 2020, 3:04 p.m. UTC | #10

I'm assuming the newly-enlarged positive error return of migrate_pages(2) won't
have adverse effects in userspace.  Didn't see issues with any user in debian
codesearch, and can't imagine how it could be relied on.

This look ok.  Just some nits, take them or leave them as you prefer.

Reviewed-by: Daniel Jordan <[e-mail]>

> diff --git a/include/trace/events/migrate.h b/include/trace/events/migrate.h
> index 705b33d1e395..4d434398d64d 100644
> --- a/include/trace/events/migrate.h
> +++ b/include/trace/events/migrate.h
> @@ -46,13 +46,18 @@ MIGRATE_REASON
>  TRACE_EVENT(mm_migrate_pages,
>  
>  	TP_PROTO(unsigned long succeeded, unsigned long failed,
> -		 enum migrate_mode mode, int reason),
> +		 unsigned long thp_succeeded, unsigned long thp_failed,
> +		 unsigned long thp_split, enum migrate_mode mode, int reason),
>  
> -	TP_ARGS(succeeded, failed, mode, reason),
> +	TP_ARGS(succeeded, failed, thp_succeeded, thp_failed,
> +		thp_split, mode, reason),
>  
>  	TP_STRUCT__entry(
>  		__field(	unsigned long,		succeeded)
>  		__field(	unsigned long,		failed)
> +		__field(	unsigned long,		thp_succeeded)
> +		__field(	unsigned long,		thp_failed)
> +		__field(	unsigned long,		thp_split)

These three are ints in the code, not unsigned long.  It can save space in the
trace event struct, 8 bytes on my machine.

> >>>> diff --git a/mm/migrate.c b/mm/migrate.c
> >>>> @@ -1429,22 +1429,35 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
                ...
> >>>> +	bool is_thp = false;

Don't need to initialize, could declare with rc/nr_subpages.

> >>>> +				if (is_thp) {
> >>>> +					nr_thp_failed++;
> >>>> +					nr_failed += nr_subpages;
> >>>> +					goto out;
> >>>> +				}
> >>>>  				nr_failed++;
> >>>>  				goto out;

This instead, in each of the three places with this pattern?:

                                        if (is_thp)
                                        	nr_thp_failed++;
                                        nr_failed += nr_subpages;
                                        goto out;

> diff --git a/Documentation/vm/page_migration.rst b/Documentation/vm/page_migration.rst
...
> +5. THP_MIGRATION_SPLIT: A THP was migrated, but not as such: first, the THP had
> +   to be split. After splitting, a migration retry was used for it's sub-pages.

The first part of this might be misinterpreted for the same reason Anshuman
changed this earlier (migration didn't necessarily happen).  We could just
delete "A THP was migrated, but not as such: first, "

Patch

diff --git a/Documentation/vm/page_migration.rst b/Documentation/vm/page_migration.rst
index 1d6cd7db4e43..68883ac485fa 100644
--- a/Documentation/vm/page_migration.rst
+++ b/Documentation/vm/page_migration.rst
@@ -253,5 +253,32 @@  which are function pointers of struct address_space_operations.
      PG_isolated is alias with PG_reclaim flag so driver shouldn't use the flag
      for own purpose.
 
+Monitoring Migration
+=====================
+
+The following events (counters) can be used to monitor page migration.
+
+1. PGMIGRATE_SUCCESS: Normal page migration success. Each count means that a
+   page was migrated. If the page was a non-THP page, then this counter is
+   increased by one. If the page was a THP, then this counter is increased by
+   the number of THP subpages. For example, migration of a single 2MB THP that
+   has 4KB-size base pages (subpages) will cause this counter to increase by
+   512.
+
+2. PGMIGRATE_FAIL: Normal page migration failure. Same counting rules as for
+   _SUCCESS, above: this will be increased by the number of subpages, if it was
+   a THP.
+
+3. THP_MIGRATION_SUCCESS: A THP was migrated without being split.
+
+4. THP_MIGRATION_FAIL: A THP could not be migrated nor it could be split.
+
+5. THP_MIGRATION_SPLIT: A THP was migrated, but not as such: first, the THP had
+   to be split. After splitting, a migration retry was used for it's sub-pages.
+
+THP_MIGRATION_* events also update the appropriate PGMIGRATE_SUCCESS or
+PGMIGRATE_FAIL events. For example, a THP migration failure will cause both
+THP_MIGRATION_FAIL and PGMIGRATE_FAIL to increase.
+
 Christoph Lameter, May 8, 2006.
 Minchan Kim, Mar 28, 2016.
diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
index 24fc7c3ae7d6..2e6ca53b9bbd 100644
--- a/include/linux/vm_event_item.h
+++ b/include/linux/vm_event_item.h
@@ -56,6 +56,9 @@  enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
 #endif
 #ifdef CONFIG_MIGRATION
 		PGMIGRATE_SUCCESS, PGMIGRATE_FAIL,
+		THP_MIGRATION_SUCCESS,
+		THP_MIGRATION_FAIL,
+		THP_MIGRATION_SPLIT,
 #endif
 #ifdef CONFIG_COMPACTION
 		COMPACTMIGRATE_SCANNED, COMPACTFREE_SCANNED,
diff --git a/include/trace/events/migrate.h b/include/trace/events/migrate.h
index 705b33d1e395..4d434398d64d 100644
--- a/include/trace/events/migrate.h
+++ b/include/trace/events/migrate.h
@@ -46,13 +46,18 @@  MIGRATE_REASON
 TRACE_EVENT(mm_migrate_pages,
 
 	TP_PROTO(unsigned long succeeded, unsigned long failed,
-		 enum migrate_mode mode, int reason),
+		 unsigned long thp_succeeded, unsigned long thp_failed,
+		 unsigned long thp_split, enum migrate_mode mode, int reason),
 
-	TP_ARGS(succeeded, failed, mode, reason),
+	TP_ARGS(succeeded, failed, thp_succeeded, thp_failed,
+		thp_split, mode, reason),
 
 	TP_STRUCT__entry(
 		__field(	unsigned long,		succeeded)
 		__field(	unsigned long,		failed)
+		__field(	unsigned long,		thp_succeeded)
+		__field(	unsigned long,		thp_failed)
+		__field(	unsigned long,		thp_split)
 		__field(	enum migrate_mode,	mode)
 		__field(	int,			reason)
 	),
@@ -60,13 +65,19 @@  TRACE_EVENT(mm_migrate_pages,
 	TP_fast_assign(
 		__entry->succeeded	= succeeded;
 		__entry->failed		= failed;
+		__entry->thp_succeeded	= thp_succeeded;
+		__entry->thp_failed	= thp_failed;
+		__entry->thp_split	= thp_split;
 		__entry->mode		= mode;
 		__entry->reason		= reason;
 	),
 
-	TP_printk("nr_succeeded=%lu nr_failed=%lu mode=%s reason=%s",
+	TP_printk("nr_succeeded=%lu nr_failed=%lu nr_thp_succeeded=%lu nr_thp_failed=%lu nr_thp_split=%lu mode=%s reason=%s",
 		__entry->succeeded,
 		__entry->failed,
+		__entry->thp_succeeded,
+		__entry->thp_failed,
+		__entry->thp_split,
 		__print_symbolic(__entry->mode, MIGRATE_MODE),
 		__print_symbolic(__entry->reason, MIGRATE_REASON))
 );
diff --git a/mm/migrate.c b/mm/migrate.c
index f37729673558..c706e3576cfc 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1429,22 +1429,35 @@  int migrate_pages(struct list_head *from, new_page_t get_new_page,
 		enum migrate_mode mode, int reason)
 {
 	int retry = 1;
+	int thp_retry = 1;
 	int nr_failed = 0;
 	int nr_succeeded = 0;
+	int nr_thp_succeeded = 0;
+	int nr_thp_failed = 0;
+	int nr_thp_split = 0;
 	int pass = 0;
+	bool is_thp = false;
 	struct page *page;
 	struct page *page2;
 	int swapwrite = current->flags & PF_SWAPWRITE;
-	int rc;
+	int rc, nr_subpages;
 
 	if (!swapwrite)
 		current->flags |= PF_SWAPWRITE;
 
-	for(pass = 0; pass < 10 && retry; pass++) {
+	for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
 		retry = 0;
+		thp_retry = 0;
 
 		list_for_each_entry_safe(page, page2, from, lru) {
 retry:
+			/*
+			 * THP statistics is based on the source huge page.
+			 * Capture required information that might get lost
+			 * during migration.
+			 */
+			is_thp = PageTransHuge(page);
+			nr_subpages = hpage_nr_pages(page);
 			cond_resched();
 
 			if (PageHuge(page))
@@ -1475,15 +1488,30 @@  int migrate_pages(struct list_head *from, new_page_t get_new_page,
 					unlock_page(page);
 					if (!rc) {
 						list_safe_reset_next(page, page2, lru);
+						nr_thp_split++;
 						goto retry;
 					}
 				}
+				if (is_thp) {
+					nr_thp_failed++;
+					nr_failed += nr_subpages;
+					goto out;
+				}
 				nr_failed++;
 				goto out;
 			case -EAGAIN:
+				if (is_thp) {
+					thp_retry++;
+					break;
+				}
 				retry++;
 				break;
 			case MIGRATEPAGE_SUCCESS:
+				if (is_thp) {
+					nr_thp_succeeded++;
+					nr_succeeded += nr_subpages;
+					break;
+				}
 				nr_succeeded++;
 				break;
 			default:
@@ -1493,19 +1521,27 @@  int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				 * removed from migration page list and not
 				 * retried in the next outer loop.
 				 */
+				if (is_thp) {
+					nr_thp_failed++;
+					nr_failed += nr_subpages;
+					break;
+				}
 				nr_failed++;
 				break;
 			}
 		}
 	}
-	nr_failed += retry;
+	nr_failed += retry + thp_retry;
+	nr_thp_failed += thp_retry;
 	rc = nr_failed;
 out:
-	if (nr_succeeded)
-		count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
-	if (nr_failed)
-		count_vm_events(PGMIGRATE_FAIL, nr_failed);
-	trace_mm_migrate_pages(nr_succeeded, nr_failed, mode, reason);
+	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
+	count_vm_events(PGMIGRATE_FAIL, nr_failed);
+	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
+	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
+	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
+	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
+			       nr_thp_failed, nr_thp_split, mode, reason);
 
 	if (!swapwrite)
 		current->flags &= ~PF_SWAPWRITE;
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 3fb23a21f6dd..09914a4bfee4 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1234,6 +1234,9 @@  const char * const vmstat_text[] = {
 #ifdef CONFIG_MIGRATION
 	"pgmigrate_success",
 	"pgmigrate_fail",
+	"thp_migration_success",
+	"thp_migration_fail",
+	"thp_migration_split",
 #endif
 #ifdef CONFIG_COMPACTION
 	"compact_migrate_scanned",