After related migrate page updates, sync up latest migrate_reason to
migrate_reason_names, page_owner use it to parse the page migrate
reason.
Fixes: d1e153fea2a8 ("mm/gup: migrate pinned pages out of movable zone")
Fixes: 26aa2d199d6f ("mm/migrate: demote pages during reclaim")
Signed-off-by: Weizhao Ouyang <[email protected]>
---
mm/debug.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/debug.c b/mm/debug.c
index e73fe0a8ec3d..733770b0ed0c 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -25,6 +25,8 @@ const char *migrate_reason_names[MR_TYPES] = {
"mempolicy_mbind",
"numa_misplaced",
"cma",
+ "longterm_pin",
+ "demotion",
};
const struct trace_print_flags pageflag_names[] = {
--
2.30.2
Weizhao Ouyang <[email protected]> writes:
> After related migrate page updates, sync up latest migrate_reason to
> migrate_reason_names, page_owner use it to parse the page migrate
> reason.
>
> Fixes: d1e153fea2a8 ("mm/gup: migrate pinned pages out of movable zone")
> Fixes: 26aa2d199d6f ("mm/migrate: demote pages during reclaim")
> Signed-off-by: Weizhao Ouyang <[email protected]>
> ---
> mm/debug.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/debug.c b/mm/debug.c
> index e73fe0a8ec3d..733770b0ed0c 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -25,6 +25,8 @@ const char *migrate_reason_names[MR_TYPES] = {
> "mempolicy_mbind",
> "numa_misplaced",
> "cma",
> + "longterm_pin",
> + "demotion",
> };
>
> const struct trace_print_flags pageflag_names[] = {
Good catch! Thanks!
Reviewed-by: "Huang, Ying" <[email protected]>
It may be better to use BUILD_BUG_ON() to capture similar issue earlier?
Best Regards,
Huang, Ying
On 9/17/21 00:03, Huang, Ying wrote:
> Weizhao Ouyang <[email protected]> writes:
>
>> After related migrate page updates, sync up latest migrate_reason to
>> migrate_reason_names, page_owner use it to parse the page migrate
>> reason.
>>
>> Fixes: d1e153fea2a8 ("mm/gup: migrate pinned pages out of movable zone")
>> Fixes: 26aa2d199d6f ("mm/migrate: demote pages during reclaim")
>> Signed-off-by: Weizhao Ouyang <[email protected]>
>> ---
>> mm/debug.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/debug.c b/mm/debug.c
>> index e73fe0a8ec3d..733770b0ed0c 100644
>> --- a/mm/debug.c
>> +++ b/mm/debug.c
>> @@ -25,6 +25,8 @@ const char *migrate_reason_names[MR_TYPES] = {
>> "mempolicy_mbind",
>> "numa_misplaced",
>> "cma",
>> + "longterm_pin",
>> + "demotion",
>> };
>>
>> const struct trace_print_flags pageflag_names[] = {
>
> Good catch! Thanks!
>
> Reviewed-by: "Huang, Ying" <[email protected]>
>
> It may be better to use BUILD_BUG_ON() to capture similar issue earlier?
Yes! Or if BUILD_BUG_ON() can't work here, then at least a comment in the
various locations, explaining that these must be kept in sync. But
BUILD_BUG_ON() should work, I think.
thanks,
--
John Hubbard
NVIDIA
Thank you both.
On 2021/9/17 下午3:03, Huang, Ying wrote:
> Weizhao Ouyang <[email protected]> writes:
>
>> After related migrate page updates, sync up latest migrate_reason to
>> migrate_reason_names, page_owner use it to parse the page migrate
>> reason.
>>
>> Fixes: d1e153fea2a8 ("mm/gup: migrate pinned pages out of movable zone")
>> Fixes: 26aa2d199d6f ("mm/migrate: demote pages during reclaim")
>> Signed-off-by: Weizhao Ouyang <[email protected]>
>> ---
>> mm/debug.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/debug.c b/mm/debug.c
>> index e73fe0a8ec3d..733770b0ed0c 100644
>> --- a/mm/debug.c
>> +++ b/mm/debug.c
>> @@ -25,6 +25,8 @@ const char *migrate_reason_names[MR_TYPES] = {
>> "mempolicy_mbind",
>> "numa_misplaced",
>> "cma",
>> + "longterm_pin",
>> + "demotion",
>> };
>>
>> const struct trace_print_flags pageflag_names[] = {
> Good catch! Thanks!
>
> Reviewed-by: "Huang, Ying" <[email protected]>
>
> It may be better to use BUILD_BUG_ON() to capture similar issue earlier?
How about move migrate_reason_names into mm/page_owner.c and make it size uninitialized(get rid of MR_TYPES).
Then use BUILD_BUG_ON(ARRAY_SIZE(migrate_reason_names != MR_TYPES)) to check it?
>
> Best Regards,
> Huang, Ying
On 9/17/21 02:48, Weizhao Ouyang wrote:
...
>>> const struct trace_print_flags pageflag_names[] = {
>> Good catch! Thanks!
>>
>> Reviewed-by: "Huang, Ying" <[email protected]>
>>
>> It may be better to use BUILD_BUG_ON() to capture similar issue earlier?
>
> How about move migrate_reason_names into mm/page_owner.c and make it size uninitialized(get rid of MR_TYPES).
> Then use BUILD_BUG_ON(ARRAY_SIZE(migrate_reason_names != MR_TYPES)) to check it?
>
A couple more thoughts:
1) From a naming and location point of view, migrate_reason_names[]
really doesn't want to be located in page_owner.c.
2) There are actually three places to synchronize, not two. And in fact,
sure enough, the MR_CONTIG_RANGE is already drifting out of synch: it
has a string of "cma" in mm/debug.c, versus "contig_range" in
include/trace/events/migrate.h.
So...is it possible to use the macro and enums in
include/trace/events/migrate.h, to define the connection between
migrate_reason and a string, everywhere?
thanks,
--
John Hubbard
NVIDIA
On 2021/9/18 08:30, John Hubbard wrote:
> On 9/17/21 02:48, Weizhao Ouyang wrote:
> ...
>>>> const struct trace_print_flags pageflag_names[] = {
>>> Good catch! Thanks!
>>>
>>> Reviewed-by: "Huang, Ying" <[email protected]>
>>>
>>> It may be better to use BUILD_BUG_ON() to capture similar issue earlier?
>>
>> How about move migrate_reason_names into mm/page_owner.c and make it size uninitialized(get rid of MR_TYPES).
>> Then use BUILD_BUG_ON(ARRAY_SIZE(migrate_reason_names != MR_TYPES)) to check it?
>>
> A couple more thoughts:
>
> 1) From a naming and location point of view, migrate_reason_names[]
> really doesn't want to be located in page_owner.c.
Commit 7cd12b4abfd2 ("mm, page_owner: track and print last migrate reason")
imported migrate_reason_names for page owner in mm/debug.c, and it only used
by page_owner.c now, maybe it's not so sensitive or we can rename it.
> 2) There are actually three places to synchronize, not two. And in fact,
> sure enough, the MR_CONTIG_RANGE is already drifting out of synch: it
> has a string of "cma" in mm/debug.c, versus "contig_range" in
> include/trace/events/migrate.h.
Yes, "cma" is out of synch after commit 310253514bbf ("mm/migrate: rename
migration reason MR_CMA to MR_CONTIG_RANGE"). Update it to "contig_range" in
migrate_reason_names can fix up it.
> So...is it possible to use the macro and enums in
> include/trace/events/migrate.h, to define the connection between
> migrate_reason and a string, everywhere?
As for synchronization, tracepoint use TRACE_DEFINE_ENUM() macro to map enums.
In general, this kind of synch between subsystem and trace event subsystem is
mostly conscious. So it more likes that include/linux/migrate.h is connected to
include/trace/events/migrate.h and migrate_reason_names, the others hasn't
relationship except same reason string.
Anyway, I didn't find a simply way the build the "everywhere" relationship behind
the packaged TRACE_DEFINE_ENUM , what do you think.
Thanks,
Weizhao
On 9/18/21 00:03, Weizhao Ouyang wrote:
...
>
> Anyway, I didn't find a simply way the build the "everywhere" relationship behind
> the packaged TRACE_DEFINE_ENUM , what do you think.
>
It's actually pretty easy, unless I'm unknowingly violating some rule
here. But I did review tracing a bit before diving in, and I think this
is reasonable.
The trace macros EM(), EMe(), and MIGRATE_REASON are flexible enough to
get whatever you want, out of them. So, the trace header can be the one
location for the definition of the enum-to-string mapping.
The key is to move the enum to a common header file that both the trace
system (trace/events/migrate.h) and the migrate header
(include/linux/migrate.h) can include. Fortunately, that's already been
started for enum migrate_mode: there is migrate_mode.h.
So it all works approximately like this, below. (I'll attach a
white-space-correct diff that you can apply directly, too). I've
compiled tested and rebooted with it, but haven't checked much more
than that yet.
---
include/linux/migrate.h | 16 ++--------------
include/linux/migrate_mode.h | 13 +++++++++++++
mm/debug.c | 19 ++++++++++++-------
3 files changed, 27 insertions(+), 21 deletions(-)
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 326250996b4e..cb62fbc3d8d6 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -6,6 +6,7 @@
#include <linux/mempolicy.h>
#include <linux/migrate_mode.h>
#include <linux/hugetlb.h>
+#include <linux/migrate_mode.h>
typedef struct page *new_page_t(struct page *page, unsigned long private);
typedef void free_page_t(struct page *page, unsigned long private);
@@ -19,20 +20,7 @@ struct migration_target_control;
*/
#define MIGRATEPAGE_SUCCESS 0
-enum migrate_reason {
- MR_COMPACTION,
- MR_MEMORY_FAILURE,
- MR_MEMORY_HOTPLUG,
- MR_SYSCALL, /* also applies to cpusets */
- MR_MEMPOLICY_MBIND,
- MR_NUMA_MISPLACED,
- MR_CONTIG_RANGE,
- MR_LONGTERM_PIN,
- MR_DEMOTION,
- MR_TYPES
-};
-
-/* In mm/debug.c; also keep sync with include/trace/events/migrate.h */
+/* In mm/debug.c */
extern const char *migrate_reason_names[MR_TYPES];
#ifdef CONFIG_MIGRATION
diff --git a/include/linux/migrate_mode.h b/include/linux/migrate_mode.h
index 883c99249033..f37cc03f9369 100644
--- a/include/linux/migrate_mode.h
+++ b/include/linux/migrate_mode.h
@@ -19,4 +19,17 @@ enum migrate_mode {
MIGRATE_SYNC_NO_COPY,
};
+enum migrate_reason {
+ MR_COMPACTION,
+ MR_MEMORY_FAILURE,
+ MR_MEMORY_HOTPLUG,
+ MR_SYSCALL, /* also applies to cpusets */
+ MR_MEMPOLICY_MBIND,
+ MR_NUMA_MISPLACED,
+ MR_CONTIG_RANGE,
+ MR_LONGTERM_PIN,
+ MR_DEMOTION,
+ MR_TYPES
+};
+
#endif /* MIGRATE_MODE_H_INCLUDED */
diff --git a/mm/debug.c b/mm/debug.c
index e73fe0a8ec3d..51152ffc1f29 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -17,14 +17,19 @@
#include "internal.h"
+#include <trace/events/migrate.h>
+
+/*
+ * Define EM() and EMe() so that MIGRATE_REASON from trace/events/migrate.h can
+ * be used to populate migrate_reason_names[].
+ */
+#undef EM
+#undef EMe
+#define EM(a, b) b,
+#define EMe(a, b) b
+
const char *migrate_reason_names[MR_TYPES] = {
- "compaction",
- "memory_failure",
- "memory_hotplug",
- "syscall_or_cpuset",
- "mempolicy_mbind",
- "numa_misplaced",
- "cma",
+ MIGRATE_REASON
};
const struct trace_print_flags pageflag_names[] = {
--
2.33.0
thanks,
--
John Hubbard
NVIDIA
On Fri, 17 Sep 2021 14:14:32 +0800 Weizhao Ouyang <[email protected]> wrote:
> After related migrate page updates, sync up latest migrate_reason to
> migrate_reason_names, page_owner use it to parse the page migrate
> reason.
A slight problem.
> Fixes: d1e153fea2a8 ("mm/gup: migrate pinned pages out of movable zone")
> Fixes: 26aa2d199d6f ("mm/migrate: demote pages during reclaim")
d1e153fea2a8 is from May 2021, so a -stable backport would be appropriate.
But 26aa2d199d6f is only in 5.15-rc1, so no cc:stable.
So can you please prepare this as a two-patch series with the first
patch (which fixes d1e153fea2a8) marked cc:stable?
Thanks.
On 2021/9/20 07:35, Andrew Morton wrote:
> On Fri, 17 Sep 2021 14:14:32 +0800 Weizhao Ouyang <[email protected]> wrote:
>
>> After related migrate page updates, sync up latest migrate_reason to
>> migrate_reason_names, page_owner use it to parse the page migrate
>> reason.
> A slight problem.
>
>> Fixes: d1e153fea2a8 ("mm/gup: migrate pinned pages out of movable zone")
>> Fixes: 26aa2d199d6f ("mm/migrate: demote pages during reclaim")
> d1e153fea2a8 is from May 2021, so a -stable backport would be appropriate.
>
> But 26aa2d199d6f is only in 5.15-rc1, so no cc:stable.
>
> So can you please prepare this as a two-patch series with the first
> patch (which fixes d1e153fea2a8) marked cc:stable?
Okay I will send v2 patch soon.
Thanks,
Weizhao
On 2021/9/19 13:38, John Hubbard wrote:
> On 9/18/21 00:03, Weizhao Ouyang wrote:
> ...
>> Anyway, I didn't find a simply way the build the "everywhere" relationship behind
>> the packaged TRACE_DEFINE_ENUM , what do you think.
>>
> It's actually pretty easy, unless I'm unknowingly violating some rule
> here. But I did review tracing a bit before diving in, and I think this
> is reasonable.
>
> The trace macros EM(), EMe(), and MIGRATE_REASON are flexible enough to
> get whatever you want, out of them. So, the trace header can be the one
> location for the definition of the enum-to-string mapping.
>
> The key is to move the enum to a common header file that both the trace
> system (trace/events/migrate.h) and the migrate header
> (include/linux/migrate.h) can include. Fortunately, that's already been
> started for enum migrate_mode: there is migrate_mode.h.
>
> So it all works approximately like this, below. (I'll attach a
> white-space-correct diff that you can apply directly, too). I've
> compiled tested and rebooted with it, but haven't checked much more
> than that yet.
Thanks for your detailed patch! Yeah, if we move the enum migrate_reason to
another header file it will attach it easily. The previous mail I said the
tricky point is that we build the "everywhere relationship" on the basis of
maintaining the original file structure, sorry for the confusing misleading.
I think we should not change a lot for a slight synchronization.
For now we can just apply the update in migrate_reason_name(maybe leave a
comment to notify) on this patch, I will send v2 patch soon include the "cma"
update. As for the trace_event synchronization, we can figure out a more
generic implementation in the future, so that other subsystem can use it to
catch a string info from its trace event header.
Thanks,
Weizhao