| Message ID | 20210917061432.323777-1-o451686892@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/debug: sync up latest migrate_reason to migrate_reason_names | expand |
Weizhao Ouyang <o451686892@gmail.com> 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 <o451686892@gmail.com> > --- > 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" <ying.huang@intel.com> 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 <o451686892@gmail.com> 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 <o451686892@gmail.com> >> --- >> 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" <ying.huang@intel.com> > > 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,
Thank you both. On 2021/9/17 下午3:03, Huang, Ying wrote: > Weizhao Ouyang <o451686892@gmail.com> 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 <o451686892@gmail.com> >> --- >> 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" <ying.huang@intel.com> > > 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" <ying.huang@intel.com> >> >> 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,
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" <ying.huang@intel.com> >>> >>> 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[] = {
On Fri, 17 Sep 2021 14:14:32 +0800 Weizhao Ouyang <o451686892@gmail.com> 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/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
On 2021/9/20 07:35, Andrew Morton wrote: > On Fri, 17 Sep 2021 14:14:32 +0800 Weizhao Ouyang <o451686892@gmail.com> 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
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[] = {
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 <o451686892@gmail.com> --- mm/debug.c | 2 ++ 1 file changed, 2 insertions(+)