linux-doc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Jordan <daniel.m.jordan@oracle.com>
To: Anshuman Khandual <anshuman.khandual@arm.com>, Zi Yan <ziy@nvidia.com>
Cc: Randy Dunlap <rdunlap@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org, Jonathan Corbet <corbet@lwn.net>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ingo Molnar <mingo@redhat.com>,
	Daniel Jordan <daniel.m.jordan@oracle.com>,
	Hugh Dickins <hughd@google.com>,
	Matthew Wilcox <willy@infradead.org>,
	John Hubbard <jhubbard@nvidia.com>,
	Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH V4] mm/vmstat: Add events for THP migration without split
Date: Fri, 24 Jul 2020 11:04:08 -0400	[thread overview]
Message-ID: <20200724150408.am4llb3qqjm4sibf@ca-dmjordan1.us.oracle.com> (raw)
In-Reply-To: <C5E3C65C-8253-4638-9D3C-71A61858BB8B@nvidia.com>

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@oracle.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, "

  parent reply	other threads:[~2020-07-24 15:06 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-09  9:39 [PATCH V4] mm/vmstat: Add events for THP migration without split Anshuman Khandual
2020-07-09 15:34 ` Randy Dunlap
2020-07-09 16:34   ` Zi Yan
2020-07-09 16:39     ` Randy Dunlap
2020-07-09 17:42       ` Zi Yan
2020-07-09 20:15         ` John Hubbard
2020-07-09 21:24         ` Randy Dunlap
2020-07-10  3:41         ` Anshuman Khandual
2020-07-24 15:04         ` Daniel Jordan [this message]
2020-07-10  3:30   ` Anshuman Khandual
2020-07-10  3:33     ` Randy Dunlap

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200724150408.am4llb3qqjm4sibf@ca-dmjordan1.us.oracle.com \
    --to=daniel.m.jordan@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=anshuman.khandual@arm.com \
    --cc=corbet@lwn.net \
    --cc=hughd@google.com \
    --cc=jhubbard@nvidia.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mingo@redhat.com \
    --cc=n-horiguchi@ah.jp.nec.com \
    --cc=rdunlap@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=willy@infradead.org \
    --cc=ziy@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).