| Message ID | 6fac1a89862b6a8bda4314d1f51380862a6fa986.1635936218.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improve the migration stats | expand |
On Wed, 3 Nov 2021 18:51:16 +0800 Baolin Wang <baolin.wang@linux.alibaba.com> wrote: > @@ -2398,10 +2399,10 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order, > > err = migrate_pages(&cc->migratepages, compaction_alloc, > compaction_free, (unsigned long)cc, cc->mode, > - MR_COMPACTION, NULL); > + MR_COMPACTION, &nr_succeeded); > > - trace_mm_compaction_migratepages(cc->nr_migratepages, err, > - &cc->migratepages); > + trace_mm_compaction_migratepages(cc->nr_migratepages, > + nr_succeeded); Also, I'm surprised you don't just pass in 'cc' and do the dereferencing in the trace event macro. Accessing the pointers from the trace event and not dereferencing them to the tracepoint function moves the changes out of line here and helps with I$. You could improve some of the other tracepoints that dereference 'cc' as well in that file. But as for this change, I'm good with it. For the tracing aspect: Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> -- Steve > > /* All pages were either migrated or will be released */ > cc->nr_migratepages = 0;
On 2021/11/3 21:42, Steven Rostedt wrote: > On Wed, 3 Nov 2021 18:51:16 +0800 > Baolin Wang <baolin.wang@linux.alibaba.com> wrote: > >> @@ -2398,10 +2399,10 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order, >> >> err = migrate_pages(&cc->migratepages, compaction_alloc, >> compaction_free, (unsigned long)cc, cc->mode, >> - MR_COMPACTION, NULL); >> + MR_COMPACTION, &nr_succeeded); >> >> - trace_mm_compaction_migratepages(cc->nr_migratepages, err, >> - &cc->migratepages); >> + trace_mm_compaction_migratepages(cc->nr_migratepages, >> + nr_succeeded); > > Also, I'm surprised you don't just pass in 'cc' and do the dereferencing in > the trace event macro. Accessing the pointers from the trace event and not > dereferencing them to the tracepoint function moves the changes out of line > here and helps with I$. > > You could improve some of the other tracepoints that dereference 'cc' as > well in that file. Sure. Will do in a separate patch. > > But as for this change, I'm good with it. > > For the tracing aspect: > > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > Thanks.
diff --git a/include/trace/events/compaction.h b/include/trace/events/compaction.h index 54e5bf0..7d48e70 100644 --- a/include/trace/events/compaction.h +++ b/include/trace/events/compaction.h @@ -68,10 +68,9 @@ TRACE_EVENT(mm_compaction_migratepages, TP_PROTO(unsigned long nr_all, - int migrate_rc, - struct list_head *migratepages), + unsigned int nr_succeeded), - TP_ARGS(nr_all, migrate_rc, migratepages), + TP_ARGS(nr_all, nr_succeeded), TP_STRUCT__entry( __field(unsigned long, nr_migrated) @@ -79,23 +78,8 @@ ), TP_fast_assign( - unsigned long nr_failed = 0; - struct list_head *page_lru; - - /* - * migrate_pages() returns either a non-negative number - * with the number of pages that failed migration, or an - * error code, in which case we need to count the remaining - * pages manually - */ - if (migrate_rc >= 0) - nr_failed = migrate_rc; - else - list_for_each(page_lru, migratepages) - nr_failed++; - - __entry->nr_migrated = nr_all - nr_failed; - __entry->nr_failed = nr_failed; + __entry->nr_migrated = nr_succeeded; + __entry->nr_failed = nr_all - nr_succeeded; ), TP_printk("nr_migrated=%lu nr_failed=%lu", diff --git a/mm/compaction.c b/mm/compaction.c index 6e44609..b4e94cd 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -2280,6 +2280,7 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order, unsigned long last_migrated_pfn; const bool sync = cc->mode != MIGRATE_ASYNC; bool update_cached; + unsigned int nr_succeeded = 0; /* * These counters track activities during zone compaction. Initialize @@ -2398,10 +2399,10 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order, err = migrate_pages(&cc->migratepages, compaction_alloc, compaction_free, (unsigned long)cc, cc->mode, - MR_COMPACTION, NULL); + MR_COMPACTION, &nr_succeeded); - trace_mm_compaction_migratepages(cc->nr_migratepages, err, - &cc->migratepages); + trace_mm_compaction_migratepages(cc->nr_migratepages, + nr_succeeded); /* All pages were either migrated or will be released */ cc->nr_migratepages = 0;
Now the migrate_pages() has changed to return the number of {normal page, THP, hugetlb} instead, thus we should not use the return value to calculate the number of pages migrated successfully. Instead we can just use the 'nr_succeeded' which indicates the number of normal pages migrated successfully to calculate the non-migrated pages in trace_mm_compaction_migratepages(). Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> --- include/trace/events/compaction.h | 24 ++++-------------------- mm/compaction.c | 7 ++++--- 2 files changed, 8 insertions(+), 23 deletions(-)