linux-kernel - Re: [PATCH V4] mm/vmstat: Add events for THP migration without split

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 24 Jul 2020 11:04:08 -0400
From:   Daniel Jordan <daniel.m.jordan@...cle.com>
To:     Anshuman Khandual <anshuman.khandual@....com>,
        Zi Yan <ziy@...dia.com>
Cc:     Randy Dunlap <rdunlap@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        Jonathan Corbet <corbet@....net>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Daniel Jordan <daniel.m.jordan@...cle.com>,
        Hugh Dickins <hughd@...gle.com>,
        Matthew Wilcox <willy@...radead.org>,
        John Hubbard <jhubbard@...dia.com>,
        Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4] mm/vmstat: Add events for THP migration without split

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 <daniel.m.jordan@...cle.com>

> 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, "