Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Thu 05-01-17 14:56:23, Mel Gorman wrote:
> On Thu, Jan 05, 2017 at 11:16:13AM +0100, Michal Hocko wrote:
> > > > diff --git a/include/trace/events/vmscan.h 
> > > > b/include/trace/events/vmscan.h
> > > > index 36c999f806bf..7ec59e0432c4 100644
> > > > --- a/include/trace/events/vmscan.h
> > > > +++ b/include/trace/events/vmscan.h
> > > > @@ -277,9 +277,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
> > > > unsigned long nr_skipped,
> > > > unsigned long nr_taken,
> > > > isolate_mode_t isolate_mode,
> > > > -   int file),
> > > > +   int lru),
> > > 
> > > It may break trace-vmscan-postprocess.pl. Other than that,
> > 
> > I wasn't aware of the script. And you are right it will break it. The
> > following should fix it. Btw. shrink_inactive_list tracepoint changes
> > will to be synced as well. I do not speak perl much but the following
> > should just work (untested yet).
> 
> It's also optional to remove them. When those were first merged, it was
> done to illustrate how multiple tracepoints can be used to aggregate
> tracepoint information. They are better ways of gathering the same class
> of information. They are of historical interest but not as fully supported
> scripts that can never break.

Yeah, that was my understanding and why I didn't consider it a priority.
But it seemed like an easy thing to fix even with my anti-perl mindset.
Here is the full patch (untested)
---
>From 1b843a180a1436873aab1fe3819dfc7dbf393870 Mon Sep 17 00:00:00 2001
From: Michal Hocko 
Date: Thu, 5 Jan 2017 11:34:03 +0100
Subject: [PATCH] trace-vmscan-postprocess: sync with tracepoints updates

Both mm_vmscan_lru_shrink_active and mm_vmscan_lru_isolate have changed
so the script needs to be update to reflect those changes

Signed-off-by: Michal Hocko 
---
 .../trace/postprocess/trace-vmscan-postprocess.pl  | 26 +++---
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl 
b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
index 8f961ef2b457..ba976805853a 100644
--- a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
+++ b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
@@ -112,8 +112,8 @@ my $regex_direct_end_default = 'nr_reclaimed=([0-9]*)';
 my $regex_kswapd_wake_default = 'nid=([0-9]*) order=([0-9]*)';
 my $regex_kswapd_sleep_default = 'nid=([0-9]*)';
 my $regex_wakeup_kswapd_default = 'nid=([0-9]*) zid=([0-9]*) order=([0-9]*)';
-my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) order=([0-9]*) 
nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_taken=([0-9]*) file=([0-9]*)';
-my $regex_lru_shrink_inactive_default = 'nid=([0-9]*) zid=([0-9]*) 
nr_scanned=([0-9]*) nr_reclaimed=([0-9]*) priority=([0-9]*) flags=([A-Z_|]*)';
+my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) classzone_idx=([0-9]*) 
order=([0-9]*) nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_skipped=([0-9]*) 
nr_taken=([0-9]*) lru=([a-z_]*)';
+my $regex_lru_shrink_inactive_default = 'nid=([0-9]*) nr_scanned=([0-9]*) 
nr_reclaimed=([0-9]*) nr_dirty=([0-9]*) nr_writeback=([0-9]*) 
nr_congested=([0-9]*) nr_immediate=([0-9]*) nr_activate=([0-9]*) 
nr_ref_keep=([0-9]*) nr_unmap_fail=([0-9]*) priority=([0-9]*) flags=([A-Z_|]*)';
 my $regex_lru_shrink_active_default = 'lru=([A-Z_]*) nr_scanned=([0-9]*) 
nr_rotated=([0-9]*) priority=([0-9]*)';
 my $regex_writepage_default = 'page=([0-9a-f]*) pfn=([0-9]*) flags=([A-Z_|]*)';
 
@@ -205,15 +205,15 @@ $regex_wakeup_kswapd = generate_traceevent_regex(
 $regex_lru_isolate = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_isolate",
$regex_lru_isolate_default,
-   "isolate_mode", "order",
-   "nr_requested", "nr_scanned", "nr_taken",
-   "file");
+   "isolate_mode", "classzone_idx", "order",
+   "nr_requested", "nr_scanned", "nr_skipped", "nr_taken",
+   "lru");
 $regex_lru_shrink_inactive = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_shrink_inactive",
$regex_lru_shrink_inactive_default,
-   "nid", "zid",
-   "nr_scanned", "nr_reclaimed", "priority",
-   "flags");
+   "nid", "nr_scanned", "nr_reclaimed", "nr_dirty", 
"nr_writeback",
+   "nr_congested", "nr_immediate", "nr_activate", 
"nr_ref_keep",
+   "nr_unmap_fail", "priority", "flags");
 $regex_lru_shrink_active = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_shrink_active",
$regex_lru_shrink_active_default,
@@ -381,8 +381,8 @@ sub process_events {
next;
}
my $isolate_mode = $1;
-

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Thu 05-01-17 14:56:23, Mel Gorman wrote:
> On Thu, Jan 05, 2017 at 11:16:13AM +0100, Michal Hocko wrote:
> > > > diff --git a/include/trace/events/vmscan.h 
> > > > b/include/trace/events/vmscan.h
> > > > index 36c999f806bf..7ec59e0432c4 100644
> > > > --- a/include/trace/events/vmscan.h
> > > > +++ b/include/trace/events/vmscan.h
> > > > @@ -277,9 +277,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
> > > > unsigned long nr_skipped,
> > > > unsigned long nr_taken,
> > > > isolate_mode_t isolate_mode,
> > > > -   int file),
> > > > +   int lru),
> > > 
> > > It may break trace-vmscan-postprocess.pl. Other than that,
> > 
> > I wasn't aware of the script. And you are right it will break it. The
> > following should fix it. Btw. shrink_inactive_list tracepoint changes
> > will to be synced as well. I do not speak perl much but the following
> > should just work (untested yet).
> 
> It's also optional to remove them. When those were first merged, it was
> done to illustrate how multiple tracepoints can be used to aggregate
> tracepoint information. They are better ways of gathering the same class
> of information. They are of historical interest but not as fully supported
> scripts that can never break.

Yeah, that was my understanding and why I didn't consider it a priority.
But it seemed like an easy thing to fix even with my anti-perl mindset.
Here is the full patch (untested)
---
>From 1b843a180a1436873aab1fe3819dfc7dbf393870 Mon Sep 17 00:00:00 2001
From: Michal Hocko 
Date: Thu, 5 Jan 2017 11:34:03 +0100
Subject: [PATCH] trace-vmscan-postprocess: sync with tracepoints updates

Both mm_vmscan_lru_shrink_active and mm_vmscan_lru_isolate have changed
so the script needs to be update to reflect those changes

Signed-off-by: Michal Hocko 
---
 .../trace/postprocess/trace-vmscan-postprocess.pl  | 26 +++---
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl 
b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
index 8f961ef2b457..ba976805853a 100644
--- a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
+++ b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
@@ -112,8 +112,8 @@ my $regex_direct_end_default = 'nr_reclaimed=([0-9]*)';
 my $regex_kswapd_wake_default = 'nid=([0-9]*) order=([0-9]*)';
 my $regex_kswapd_sleep_default = 'nid=([0-9]*)';
 my $regex_wakeup_kswapd_default = 'nid=([0-9]*) zid=([0-9]*) order=([0-9]*)';
-my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) order=([0-9]*) 
nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_taken=([0-9]*) file=([0-9]*)';
-my $regex_lru_shrink_inactive_default = 'nid=([0-9]*) zid=([0-9]*) 
nr_scanned=([0-9]*) nr_reclaimed=([0-9]*) priority=([0-9]*) flags=([A-Z_|]*)';
+my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) classzone_idx=([0-9]*) 
order=([0-9]*) nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_skipped=([0-9]*) 
nr_taken=([0-9]*) lru=([a-z_]*)';
+my $regex_lru_shrink_inactive_default = 'nid=([0-9]*) nr_scanned=([0-9]*) 
nr_reclaimed=([0-9]*) nr_dirty=([0-9]*) nr_writeback=([0-9]*) 
nr_congested=([0-9]*) nr_immediate=([0-9]*) nr_activate=([0-9]*) 
nr_ref_keep=([0-9]*) nr_unmap_fail=([0-9]*) priority=([0-9]*) flags=([A-Z_|]*)';
 my $regex_lru_shrink_active_default = 'lru=([A-Z_]*) nr_scanned=([0-9]*) 
nr_rotated=([0-9]*) priority=([0-9]*)';
 my $regex_writepage_default = 'page=([0-9a-f]*) pfn=([0-9]*) flags=([A-Z_|]*)';
 
@@ -205,15 +205,15 @@ $regex_wakeup_kswapd = generate_traceevent_regex(
 $regex_lru_isolate = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_isolate",
$regex_lru_isolate_default,
-   "isolate_mode", "order",
-   "nr_requested", "nr_scanned", "nr_taken",
-   "file");
+   "isolate_mode", "classzone_idx", "order",
+   "nr_requested", "nr_scanned", "nr_skipped", "nr_taken",
+   "lru");
 $regex_lru_shrink_inactive = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_shrink_inactive",
$regex_lru_shrink_inactive_default,
-   "nid", "zid",
-   "nr_scanned", "nr_reclaimed", "priority",
-   "flags");
+   "nid", "nr_scanned", "nr_reclaimed", "nr_dirty", 
"nr_writeback",
+   "nr_congested", "nr_immediate", "nr_activate", 
"nr_ref_keep",
+   "nr_unmap_fail", "priority", "flags");
 $regex_lru_shrink_active = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_shrink_active",
$regex_lru_shrink_active_default,
@@ -381,8 +381,8 @@ sub process_events {
next;
}
my $isolate_mode = $1;
-   my $nr_scanned 

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Mel Gorman]

On Thu, Jan 05, 2017 at 11:16:13AM +0100, Michal Hocko wrote:
> > > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > > index 36c999f806bf..7ec59e0432c4 100644
> > > --- a/include/trace/events/vmscan.h
> > > +++ b/include/trace/events/vmscan.h
> > > @@ -277,9 +277,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
> > >   unsigned long nr_skipped,
> > >   unsigned long nr_taken,
> > >   isolate_mode_t isolate_mode,
> > > - int file),
> > > + int lru),
> > 
> > It may break trace-vmscan-postprocess.pl. Other than that,
> 
> I wasn't aware of the script. And you are right it will break it. The
> following should fix it. Btw. shrink_inactive_list tracepoint changes
> will to be synced as well. I do not speak perl much but the following
> should just work (untested yet).

It's also optional to remove them. When those were first merged, it was
done to illustrate how multiple tracepoints can be used to aggregate
tracepoint information. They are better ways of gathering the same class
of information. They are of historical interest but not as fully supported
scripts that can never break.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Mel Gorman]

On Thu, Jan 05, 2017 at 11:16:13AM +0100, Michal Hocko wrote:
> > > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > > index 36c999f806bf..7ec59e0432c4 100644
> > > --- a/include/trace/events/vmscan.h
> > > +++ b/include/trace/events/vmscan.h
> > > @@ -277,9 +277,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
> > >   unsigned long nr_skipped,
> > >   unsigned long nr_taken,
> > >   isolate_mode_t isolate_mode,
> > > - int file),
> > > + int lru),
> > 
> > It may break trace-vmscan-postprocess.pl. Other than that,
> 
> I wasn't aware of the script. And you are right it will break it. The
> following should fix it. Btw. shrink_inactive_list tracepoint changes
> will to be synced as well. I do not speak perl much but the following
> should just work (untested yet).

It's also optional to remove them. When those were first merged, it was
done to illustrate how multiple tracepoints can be used to aggregate
tracepoint information. They are better ways of gathering the same class
of information. They are of historical interest but not as fully supported
scripts that can never break.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Thu 05-01-17 15:04:58, Minchan Kim wrote:
> On Wed, Jan 04, 2017 at 11:19:39AM +0100, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > from is file or anonymous but we do not know which LRU this is.
> > 
> > It is useful to know whether the list is active or inactive, since we
> > are using the same function to isolate pages from both of them and it's
> > hard to distinguish otherwise.
> > 
> > Chaneges since v1
> > - drop LRU_ prefix from names and use lowercase as per Vlastimil
> > - move and convert show_lru_name to mmflags.h EM magic as per Vlastimil
> > 
> > Acked-by: Hillf Danton 
> > Acked-by: Mel Gorman 
> > Signed-off-by: Michal Hocko 
> 
> > ---
> >  include/trace/events/mmflags.h |  8 
> >  include/trace/events/vmscan.h  | 12 ++--
> >  mm/vmscan.c|  2 +-
> >  3 files changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> > index aa4caa6914a9..6172afa2fd82 100644
> > --- a/include/trace/events/mmflags.h
> > +++ b/include/trace/events/mmflags.h
> > @@ -240,6 +240,13 @@ IF_HAVE_VM_SOFTDIRTY(VM_SOFTDIRTY, "softdirty" 
> > )   \
> > IFDEF_ZONE_HIGHMEM( EM (ZONE_HIGHMEM,"HighMem"))\
> > EMe(ZONE_MOVABLE,"Movable")
> >  
> > +#define LRU_NAMES  \
> > +   EM (LRU_INACTIVE_ANON, "inactive_anon") \
> > +   EM (LRU_ACTIVE_ANON, "active_anon") \
> > +   EM (LRU_INACTIVE_FILE, "inactive_file") \
> > +   EM (LRU_ACTIVE_FILE, "active_file") \
> > +   EMe(LRU_UNEVICTABLE, "unevictable")
> > +
> >  /*
> >   * First define the enums in the above macros to be exported to userspace
> >   * via TRACE_DEFINE_ENUM().
> > @@ -253,6 +260,7 @@ COMPACTION_STATUS
> >  COMPACTION_PRIORITY
> >  COMPACTION_FEEDBACK
> >  ZONE_TYPE
> > +LRU_NAMES
> >  
> >  /*
> >   * Now redefine the EM() and EMe() macros to map the enums to the strings
> > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > index 36c999f806bf..7ec59e0432c4 100644
> > --- a/include/trace/events/vmscan.h
> > +++ b/include/trace/events/vmscan.h
> > @@ -277,9 +277,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
> > unsigned long nr_skipped,
> > unsigned long nr_taken,
> > isolate_mode_t isolate_mode,
> > -   int file),
> > +   int lru),
> 
> It may break trace-vmscan-postprocess.pl. Other than that,

I wasn't aware of the script. And you are right it will break it. The
following should fix it. Btw. shrink_inactive_list tracepoint changes
will to be synced as well. I do not speak perl much but the following
should just work (untested yet).
---
diff --git a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl 
b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
index 8f961ef2b457..ba976805853a 100644
--- a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
+++ b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
@@ -112,8 +112,8 @@ my $regex_direct_end_default = 'nr_reclaimed=([0-9]*)';
 my $regex_kswapd_wake_default = 'nid=([0-9]*) order=([0-9]*)';
 my $regex_kswapd_sleep_default = 'nid=([0-9]*)';
 my $regex_wakeup_kswapd_default = 'nid=([0-9]*) zid=([0-9]*) order=([0-9]*)';
-my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) order=([0-9]*) 
nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_taken=([0-9]*) file=([0-9]*)';
-my $regex_lru_shrink_inactive_default = 'nid=([0-9]*) zid=([0-9]*) 
nr_scanned=([0-9]*) nr_reclaimed=([0-9]*) priority=([0-9]*) flags=([A-Z_|]*)';
+my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) classzone_idx=([0-9]*) 
order=([0-9]*) nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_skipped=([0-9]*) 
nr_taken=([0-9]*) lru=([a-z_]*)';
+my $regex_lru_shrink_inactive_default = 'nid=([0-9]*) nr_scanned=([0-9]*) 
nr_reclaimed=([0-9]*) nr_dirty=([0-9]*) nr_writeback=([0-9]*) 
nr_congested=([0-9]*) nr_immediate=([0-9]*) nr_activate=([0-9]*) 
nr_ref_keep=([0-9]*) nr_unmap_fail=([0-9]*) priority=([0-9]*) flags=([A-Z_|]*)';
 my $regex_lru_shrink_active_default = 'lru=([A-Z_]*) nr_scanned=([0-9]*) 
nr_rotated=([0-9]*) priority=([0-9]*)';
 my $regex_writepage_default = 'page=([0-9a-f]*) pfn=([0-9]*) flags=([A-Z_|]*)';
 
@@ -205,15 +205,15 @@ $regex_wakeup_kswapd = generate_traceevent_regex(
 $regex_lru_isolate = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_isolate",
$regex_lru_isolate_default,
-   "isolate_mode", "order",
-   "nr_requested", "nr_scanned", "nr_taken",
-   "file");
+   "isolate_mode", "classzone_idx", "order",
+   "nr_requested", "nr_scanned", "nr_skipped", "nr_taken",
+   "lru");
 

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Thu 05-01-17 15:04:58, Minchan Kim wrote:
> On Wed, Jan 04, 2017 at 11:19:39AM +0100, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > from is file or anonymous but we do not know which LRU this is.
> > 
> > It is useful to know whether the list is active or inactive, since we
> > are using the same function to isolate pages from both of them and it's
> > hard to distinguish otherwise.
> > 
> > Chaneges since v1
> > - drop LRU_ prefix from names and use lowercase as per Vlastimil
> > - move and convert show_lru_name to mmflags.h EM magic as per Vlastimil
> > 
> > Acked-by: Hillf Danton 
> > Acked-by: Mel Gorman 
> > Signed-off-by: Michal Hocko 
> 
> > ---
> >  include/trace/events/mmflags.h |  8 
> >  include/trace/events/vmscan.h  | 12 ++--
> >  mm/vmscan.c|  2 +-
> >  3 files changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> > index aa4caa6914a9..6172afa2fd82 100644
> > --- a/include/trace/events/mmflags.h
> > +++ b/include/trace/events/mmflags.h
> > @@ -240,6 +240,13 @@ IF_HAVE_VM_SOFTDIRTY(VM_SOFTDIRTY, "softdirty" 
> > )   \
> > IFDEF_ZONE_HIGHMEM( EM (ZONE_HIGHMEM,"HighMem"))\
> > EMe(ZONE_MOVABLE,"Movable")
> >  
> > +#define LRU_NAMES  \
> > +   EM (LRU_INACTIVE_ANON, "inactive_anon") \
> > +   EM (LRU_ACTIVE_ANON, "active_anon") \
> > +   EM (LRU_INACTIVE_FILE, "inactive_file") \
> > +   EM (LRU_ACTIVE_FILE, "active_file") \
> > +   EMe(LRU_UNEVICTABLE, "unevictable")
> > +
> >  /*
> >   * First define the enums in the above macros to be exported to userspace
> >   * via TRACE_DEFINE_ENUM().
> > @@ -253,6 +260,7 @@ COMPACTION_STATUS
> >  COMPACTION_PRIORITY
> >  COMPACTION_FEEDBACK
> >  ZONE_TYPE
> > +LRU_NAMES
> >  
> >  /*
> >   * Now redefine the EM() and EMe() macros to map the enums to the strings
> > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > index 36c999f806bf..7ec59e0432c4 100644
> > --- a/include/trace/events/vmscan.h
> > +++ b/include/trace/events/vmscan.h
> > @@ -277,9 +277,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
> > unsigned long nr_skipped,
> > unsigned long nr_taken,
> > isolate_mode_t isolate_mode,
> > -   int file),
> > +   int lru),
> 
> It may break trace-vmscan-postprocess.pl. Other than that,

I wasn't aware of the script. And you are right it will break it. The
following should fix it. Btw. shrink_inactive_list tracepoint changes
will to be synced as well. I do not speak perl much but the following
should just work (untested yet).
---
diff --git a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl 
b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
index 8f961ef2b457..ba976805853a 100644
--- a/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
+++ b/Documentation/trace/postprocess/trace-vmscan-postprocess.pl
@@ -112,8 +112,8 @@ my $regex_direct_end_default = 'nr_reclaimed=([0-9]*)';
 my $regex_kswapd_wake_default = 'nid=([0-9]*) order=([0-9]*)';
 my $regex_kswapd_sleep_default = 'nid=([0-9]*)';
 my $regex_wakeup_kswapd_default = 'nid=([0-9]*) zid=([0-9]*) order=([0-9]*)';
-my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) order=([0-9]*) 
nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_taken=([0-9]*) file=([0-9]*)';
-my $regex_lru_shrink_inactive_default = 'nid=([0-9]*) zid=([0-9]*) 
nr_scanned=([0-9]*) nr_reclaimed=([0-9]*) priority=([0-9]*) flags=([A-Z_|]*)';
+my $regex_lru_isolate_default = 'isolate_mode=([0-9]*) classzone_idx=([0-9]*) 
order=([0-9]*) nr_requested=([0-9]*) nr_scanned=([0-9]*) nr_skipped=([0-9]*) 
nr_taken=([0-9]*) lru=([a-z_]*)';
+my $regex_lru_shrink_inactive_default = 'nid=([0-9]*) nr_scanned=([0-9]*) 
nr_reclaimed=([0-9]*) nr_dirty=([0-9]*) nr_writeback=([0-9]*) 
nr_congested=([0-9]*) nr_immediate=([0-9]*) nr_activate=([0-9]*) 
nr_ref_keep=([0-9]*) nr_unmap_fail=([0-9]*) priority=([0-9]*) flags=([A-Z_|]*)';
 my $regex_lru_shrink_active_default = 'lru=([A-Z_]*) nr_scanned=([0-9]*) 
nr_rotated=([0-9]*) priority=([0-9]*)';
 my $regex_writepage_default = 'page=([0-9a-f]*) pfn=([0-9]*) flags=([A-Z_|]*)';
 
@@ -205,15 +205,15 @@ $regex_wakeup_kswapd = generate_traceevent_regex(
 $regex_lru_isolate = generate_traceevent_regex(
"vmscan/mm_vmscan_lru_isolate",
$regex_lru_isolate_default,
-   "isolate_mode", "order",
-   "nr_requested", "nr_scanned", "nr_taken",
-   "file");
+   "isolate_mode", "classzone_idx", "order",
+   "nr_requested", "nr_scanned", "nr_skipped", "nr_taken",
+   "lru");
 $regex_lru_shrink_inactive = generate_traceevent_regex(

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Minchan Kim]

On Wed, Jan 04, 2017 at 11:19:39AM +0100, Michal Hocko wrote:
> From: Michal Hocko 
> 
> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> from is file or anonymous but we do not know which LRU this is.
> 
> It is useful to know whether the list is active or inactive, since we
> are using the same function to isolate pages from both of them and it's
> hard to distinguish otherwise.
> 
> Chaneges since v1
> - drop LRU_ prefix from names and use lowercase as per Vlastimil
> - move and convert show_lru_name to mmflags.h EM magic as per Vlastimil
> 
> Acked-by: Hillf Danton 
> Acked-by: Mel Gorman 
> Signed-off-by: Michal Hocko 

> ---
>  include/trace/events/mmflags.h |  8 
>  include/trace/events/vmscan.h  | 12 ++--
>  mm/vmscan.c|  2 +-
>  3 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> index aa4caa6914a9..6172afa2fd82 100644
> --- a/include/trace/events/mmflags.h
> +++ b/include/trace/events/mmflags.h
> @@ -240,6 +240,13 @@ IF_HAVE_VM_SOFTDIRTY(VM_SOFTDIRTY,   "softdirty" 
> )   \
>   IFDEF_ZONE_HIGHMEM( EM (ZONE_HIGHMEM,"HighMem"))\
>   EMe(ZONE_MOVABLE,"Movable")
>  
> +#define LRU_NAMES\
> + EM (LRU_INACTIVE_ANON, "inactive_anon") \
> + EM (LRU_ACTIVE_ANON, "active_anon") \
> + EM (LRU_INACTIVE_FILE, "inactive_file") \
> + EM (LRU_ACTIVE_FILE, "active_file") \
> + EMe(LRU_UNEVICTABLE, "unevictable")
> +
>  /*
>   * First define the enums in the above macros to be exported to userspace
>   * via TRACE_DEFINE_ENUM().
> @@ -253,6 +260,7 @@ COMPACTION_STATUS
>  COMPACTION_PRIORITY
>  COMPACTION_FEEDBACK
>  ZONE_TYPE
> +LRU_NAMES
>  
>  /*
>   * Now redefine the EM() and EMe() macros to map the enums to the strings
> diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> index 36c999f806bf..7ec59e0432c4 100644
> --- a/include/trace/events/vmscan.h
> +++ b/include/trace/events/vmscan.h
> @@ -277,9 +277,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   unsigned long nr_skipped,
>   unsigned long nr_taken,
>   isolate_mode_t isolate_mode,
> - int file),
> + int lru),

It may break trace-vmscan-postprocess.pl. Other than that,

Acked-by: Minchan Kim 

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Minchan Kim]

On Wed, Jan 04, 2017 at 11:19:39AM +0100, Michal Hocko wrote:
> From: Michal Hocko 
> 
> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> from is file or anonymous but we do not know which LRU this is.
> 
> It is useful to know whether the list is active or inactive, since we
> are using the same function to isolate pages from both of them and it's
> hard to distinguish otherwise.
> 
> Chaneges since v1
> - drop LRU_ prefix from names and use lowercase as per Vlastimil
> - move and convert show_lru_name to mmflags.h EM magic as per Vlastimil
> 
> Acked-by: Hillf Danton 
> Acked-by: Mel Gorman 
> Signed-off-by: Michal Hocko 

> ---
>  include/trace/events/mmflags.h |  8 
>  include/trace/events/vmscan.h  | 12 ++--
>  mm/vmscan.c|  2 +-
>  3 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> index aa4caa6914a9..6172afa2fd82 100644
> --- a/include/trace/events/mmflags.h
> +++ b/include/trace/events/mmflags.h
> @@ -240,6 +240,13 @@ IF_HAVE_VM_SOFTDIRTY(VM_SOFTDIRTY,   "softdirty" 
> )   \
>   IFDEF_ZONE_HIGHMEM( EM (ZONE_HIGHMEM,"HighMem"))\
>   EMe(ZONE_MOVABLE,"Movable")
>  
> +#define LRU_NAMES\
> + EM (LRU_INACTIVE_ANON, "inactive_anon") \
> + EM (LRU_ACTIVE_ANON, "active_anon") \
> + EM (LRU_INACTIVE_FILE, "inactive_file") \
> + EM (LRU_ACTIVE_FILE, "active_file") \
> + EMe(LRU_UNEVICTABLE, "unevictable")
> +
>  /*
>   * First define the enums in the above macros to be exported to userspace
>   * via TRACE_DEFINE_ENUM().
> @@ -253,6 +260,7 @@ COMPACTION_STATUS
>  COMPACTION_PRIORITY
>  COMPACTION_FEEDBACK
>  ZONE_TYPE
> +LRU_NAMES
>  
>  /*
>   * Now redefine the EM() and EMe() macros to map the enums to the strings
> diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> index 36c999f806bf..7ec59e0432c4 100644
> --- a/include/trace/events/vmscan.h
> +++ b/include/trace/events/vmscan.h
> @@ -277,9 +277,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   unsigned long nr_skipped,
>   unsigned long nr_taken,
>   isolate_mode_t isolate_mode,
> - int file),
> + int lru),

It may break trace-vmscan-postprocess.pl. Other than that,

Acked-by: Minchan Kim 

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Tue 03-01-17 21:52:44, Michal Hocko wrote:
> On Tue 03-01-17 21:47:45, Michal Hocko wrote:
> > On Tue 03-01-17 18:08:58, Vlastimil Babka wrote:
> > > On 12/28/2016 04:30 PM, Michal Hocko wrote:
> > > > From: Michal Hocko 
> > > > 
> > > > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > > > from is file or anonymous but we do not know which LRU this is. It is
> > > > useful to know whether the list is file or anonymous as well. Change
> > > > the tracepoint to show symbolic names of the lru rather.
> > > > 
> > > > Signed-off-by: Michal Hocko 
> > > > ---
> > > >  include/trace/events/vmscan.h | 20 ++--
> > > >  mm/vmscan.c   |  2 +-
> > > >  2 files changed, 15 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/include/trace/events/vmscan.h 
> > > > b/include/trace/events/vmscan.h
> > > > index 6af4dae46db2..cc0b4c456c78 100644
> > > > --- a/include/trace/events/vmscan.h
> > > > +++ b/include/trace/events/vmscan.h
> > > > @@ -36,6 +36,14 @@
> > > > (RECLAIM_WB_ASYNC) \
> > > > )
> > > > 
> > > > +#define show_lru_name(lru) \
> > > > +   __print_symbolic(lru, \
> > > > +   {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> > > > +   {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> > > > +   {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> > > > +   {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> > > > +   {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> > > > +
> > > 
> > > Does this work with external tools such as trace-cmd, i.e. does it export
> > > the correct format file?
> > 
> > How do I find out?
> 
> Well, I've just checked the format file and it says
> print fmt: "isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s", REC->isolate_mode, 
> REC->classzone_idx, REC->order, REC->nr_requested, REC->nr_scanned, 
> REC->nr_skipped, REC->nr_taken, __print_symbolic(REC->lru, 
> {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, {LRU_ACTIVE_ANON, 
> "LRU_ACTIVE_ANON"}, {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, 
> {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> 
> So the tool should be OK as long as it can find values for LRU_*
> constants. Is this what is the problem?

OK, I got it. We need enum->value translation and all the EM stuff to do
that, right?

I will rework the patch and move the definition to the rest of the EM
family...
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Tue 03-01-17 21:52:44, Michal Hocko wrote:
> On Tue 03-01-17 21:47:45, Michal Hocko wrote:
> > On Tue 03-01-17 18:08:58, Vlastimil Babka wrote:
> > > On 12/28/2016 04:30 PM, Michal Hocko wrote:
> > > > From: Michal Hocko 
> > > > 
> > > > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > > > from is file or anonymous but we do not know which LRU this is. It is
> > > > useful to know whether the list is file or anonymous as well. Change
> > > > the tracepoint to show symbolic names of the lru rather.
> > > > 
> > > > Signed-off-by: Michal Hocko 
> > > > ---
> > > >  include/trace/events/vmscan.h | 20 ++--
> > > >  mm/vmscan.c   |  2 +-
> > > >  2 files changed, 15 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/include/trace/events/vmscan.h 
> > > > b/include/trace/events/vmscan.h
> > > > index 6af4dae46db2..cc0b4c456c78 100644
> > > > --- a/include/trace/events/vmscan.h
> > > > +++ b/include/trace/events/vmscan.h
> > > > @@ -36,6 +36,14 @@
> > > > (RECLAIM_WB_ASYNC) \
> > > > )
> > > > 
> > > > +#define show_lru_name(lru) \
> > > > +   __print_symbolic(lru, \
> > > > +   {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> > > > +   {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> > > > +   {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> > > > +   {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> > > > +   {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> > > > +
> > > 
> > > Does this work with external tools such as trace-cmd, i.e. does it export
> > > the correct format file?
> > 
> > How do I find out?
> 
> Well, I've just checked the format file and it says
> print fmt: "isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s", REC->isolate_mode, 
> REC->classzone_idx, REC->order, REC->nr_requested, REC->nr_scanned, 
> REC->nr_skipped, REC->nr_taken, __print_symbolic(REC->lru, 
> {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, {LRU_ACTIVE_ANON, 
> "LRU_ACTIVE_ANON"}, {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, 
> {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> 
> So the tool should be OK as long as it can find values for LRU_*
> constants. Is this what is the problem?

OK, I got it. We need enum->value translation and all the EM stuff to do
that, right?

I will rework the patch and move the definition to the rest of the EM
family...
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Tue 03-01-17 22:40:23, Vlastimil Babka wrote:
> On 01/03/2017 10:24 PM, Michal Hocko wrote:
[...]
> > > So the tool should be OK as long as it can find values for LRU_*
> > > constants. Is this what is the problem?
> 
> Exactly.

So this should make it work (it compiles it has to be correct, right?).
---
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index aa4caa6914a9..6172afa2fd82 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -240,6 +240,13 @@ IF_HAVE_VM_SOFTDIRTY(VM_SOFTDIRTY, "softdirty" )   
\
IFDEF_ZONE_HIGHMEM( EM (ZONE_HIGHMEM,"HighMem"))\
EMe(ZONE_MOVABLE,"Movable")
 
+#define LRU_NAMES  \
+   EM (LRU_INACTIVE_ANON, "inactive_anon") \
+   EM (LRU_ACTIVE_ANON, "active_anon") \
+   EM (LRU_INACTIVE_FILE, "inactive_file") \
+   EM (LRU_ACTIVE_FILE, "active_file") \
+   EMe(LRU_UNEVICTABLE, "unevictable")
+
 /*
  * First define the enums in the above macros to be exported to userspace
  * via TRACE_DEFINE_ENUM().
@@ -253,6 +260,7 @@ COMPACTION_STATUS
 COMPACTION_PRIORITY
 COMPACTION_FEEDBACK
 ZONE_TYPE
+LRU_NAMES
 
 /*
  * Now redefine the EM() and EMe() macros to map the enums to the strings
diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
index 8e7c4c56499a..3c38d9315b43 100644
--- a/include/trace/events/vmscan.h
+++ b/include/trace/events/vmscan.h
@@ -36,14 +36,6 @@
(RECLAIM_WB_ASYNC) \
)
 
-#define show_lru_name(lru) \
-   __print_symbolic(lru, \
-   {LRU_INACTIVE_ANON, "inactive_anon"}, \
-   {LRU_ACTIVE_ANON, "active_anon"}, \
-   {LRU_INACTIVE_FILE, "inactive_file"}, \
-   {LRU_ACTIVE_FILE, "active_file"}, \
-   {LRU_UNEVICTABLE, "unevictable"})
-
 TRACE_EVENT(mm_vmscan_kswapd_sleep,
 
TP_PROTO(int nid),
@@ -319,7 +311,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
__entry->nr_scanned,
__entry->nr_skipped,
__entry->nr_taken,
-   show_lru_name(__entry->lru))
+   __print_symbolic(__entry->lru, LRU_NAMES))
 );
 
 TRACE_EVENT(mm_vmscan_writepage,

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Tue 03-01-17 22:40:23, Vlastimil Babka wrote:
> On 01/03/2017 10:24 PM, Michal Hocko wrote:
[...]
> > > So the tool should be OK as long as it can find values for LRU_*
> > > constants. Is this what is the problem?
> 
> Exactly.

So this should make it work (it compiles it has to be correct, right?).
---
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index aa4caa6914a9..6172afa2fd82 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -240,6 +240,13 @@ IF_HAVE_VM_SOFTDIRTY(VM_SOFTDIRTY, "softdirty" )   
\
IFDEF_ZONE_HIGHMEM( EM (ZONE_HIGHMEM,"HighMem"))\
EMe(ZONE_MOVABLE,"Movable")
 
+#define LRU_NAMES  \
+   EM (LRU_INACTIVE_ANON, "inactive_anon") \
+   EM (LRU_ACTIVE_ANON, "active_anon") \
+   EM (LRU_INACTIVE_FILE, "inactive_file") \
+   EM (LRU_ACTIVE_FILE, "active_file") \
+   EMe(LRU_UNEVICTABLE, "unevictable")
+
 /*
  * First define the enums in the above macros to be exported to userspace
  * via TRACE_DEFINE_ENUM().
@@ -253,6 +260,7 @@ COMPACTION_STATUS
 COMPACTION_PRIORITY
 COMPACTION_FEEDBACK
 ZONE_TYPE
+LRU_NAMES
 
 /*
  * Now redefine the EM() and EMe() macros to map the enums to the strings
diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
index 8e7c4c56499a..3c38d9315b43 100644
--- a/include/trace/events/vmscan.h
+++ b/include/trace/events/vmscan.h
@@ -36,14 +36,6 @@
(RECLAIM_WB_ASYNC) \
)
 
-#define show_lru_name(lru) \
-   __print_symbolic(lru, \
-   {LRU_INACTIVE_ANON, "inactive_anon"}, \
-   {LRU_ACTIVE_ANON, "active_anon"}, \
-   {LRU_INACTIVE_FILE, "inactive_file"}, \
-   {LRU_ACTIVE_FILE, "active_file"}, \
-   {LRU_UNEVICTABLE, "unevictable"})
-
 TRACE_EVENT(mm_vmscan_kswapd_sleep,
 
TP_PROTO(int nid),
@@ -319,7 +311,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
__entry->nr_scanned,
__entry->nr_skipped,
__entry->nr_taken,
-   show_lru_name(__entry->lru))
+   __print_symbolic(__entry->lru, LRU_NAMES))
 );
 
 TRACE_EVENT(mm_vmscan_writepage,

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Vlastimil Babka]

On 01/03/2017 10:24 PM, Michal Hocko wrote:

On Tue 03-01-17 21:52:44, Michal Hocko wrote:

On Tue 03-01-17 21:47:45, Michal Hocko wrote:
> On Tue 03-01-17 18:08:58, Vlastimil Babka wrote:
> > On 12/28/2016 04:30 PM, Michal Hocko wrote:
> > > From: Michal Hocko 
> > >
> > > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > > from is file or anonymous but we do not know which LRU this is. It is
> > > useful to know whether the list is file or anonymous as well. Change
> > > the tracepoint to show symbolic names of the lru rather.
> > >
> > > Signed-off-by: Michal Hocko 
> > > ---
> > >  include/trace/events/vmscan.h | 20 ++--
> > >  mm/vmscan.c   |  2 +-
> > >  2 files changed, 15 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > > index 6af4dae46db2..cc0b4c456c78 100644
> > > --- a/include/trace/events/vmscan.h
> > > +++ b/include/trace/events/vmscan.h
> > > @@ -36,6 +36,14 @@
> > >  (RECLAIM_WB_ASYNC) \
> > >  )
> > >
> > > +#define show_lru_name(lru) \
> > > +__print_symbolic(lru, \
> > > +{LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> > > +{LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> > > +{LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> > > +{LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> > > +{LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> > > +
> >
> > Does this work with external tools such as trace-cmd, i.e. does it export
> > the correct format file?
>
> How do I find out?


You did :) Another way to verify is to use trace-cmd tool instead of manual 
sysfs operations and see if the output looks as expected. The tool gets the raw 
records from kernel and does the printing in userspace, unlike "cat trace_pipe".



Well, I've just checked the format file and it says
print fmt: "isolate_mode=%d classzone=%d order=%d nr_requested=%lu nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s", REC->isolate_mode, 
REC->classzone_idx, REC->order, REC->nr_requested, REC->nr_scanned, REC->nr_skipped, REC->nr_taken, __print_symbolic(REC->lru, 
{LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, 
{LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})

So the tool should be OK as long as it can find values for LRU_*
constants. Is this what is the problem?


Exactly.


OK, I got it. We need enum->value translation and all the EM stuff to do
that, right?


Yep.


I will rework the patch and move the definition to the rest of the EM
family...


Thanks!

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Vlastimil Babka]

On 01/03/2017 10:24 PM, Michal Hocko wrote:

On Tue 03-01-17 21:52:44, Michal Hocko wrote:

On Tue 03-01-17 21:47:45, Michal Hocko wrote:
> On Tue 03-01-17 18:08:58, Vlastimil Babka wrote:
> > On 12/28/2016 04:30 PM, Michal Hocko wrote:
> > > From: Michal Hocko 
> > >
> > > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > > from is file or anonymous but we do not know which LRU this is. It is
> > > useful to know whether the list is file or anonymous as well. Change
> > > the tracepoint to show symbolic names of the lru rather.
> > >
> > > Signed-off-by: Michal Hocko 
> > > ---
> > >  include/trace/events/vmscan.h | 20 ++--
> > >  mm/vmscan.c   |  2 +-
> > >  2 files changed, 15 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > > index 6af4dae46db2..cc0b4c456c78 100644
> > > --- a/include/trace/events/vmscan.h
> > > +++ b/include/trace/events/vmscan.h
> > > @@ -36,6 +36,14 @@
> > >  (RECLAIM_WB_ASYNC) \
> > >  )
> > >
> > > +#define show_lru_name(lru) \
> > > +__print_symbolic(lru, \
> > > +{LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> > > +{LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> > > +{LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> > > +{LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> > > +{LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> > > +
> >
> > Does this work with external tools such as trace-cmd, i.e. does it export
> > the correct format file?
>
> How do I find out?


You did :) Another way to verify is to use trace-cmd tool instead of manual 
sysfs operations and see if the output looks as expected. The tool gets the raw 
records from kernel and does the printing in userspace, unlike "cat trace_pipe".



Well, I've just checked the format file and it says
print fmt: "isolate_mode=%d classzone=%d order=%d nr_requested=%lu nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s", REC->isolate_mode, 
REC->classzone_idx, REC->order, REC->nr_requested, REC->nr_scanned, REC->nr_skipped, REC->nr_taken, __print_symbolic(REC->lru, 
{LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, 
{LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})

So the tool should be OK as long as it can find values for LRU_*
constants. Is this what is the problem?


Exactly.


OK, I got it. We need enum->value translation and all the EM stuff to do
that, right?


Yep.


I will rework the patch and move the definition to the rest of the EM
family...


Thanks!

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Tue 03-01-17 21:47:45, Michal Hocko wrote:
> On Tue 03-01-17 18:08:58, Vlastimil Babka wrote:
> > On 12/28/2016 04:30 PM, Michal Hocko wrote:
> > > From: Michal Hocko 
> > > 
> > > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > > from is file or anonymous but we do not know which LRU this is. It is
> > > useful to know whether the list is file or anonymous as well. Change
> > > the tracepoint to show symbolic names of the lru rather.
> > > 
> > > Signed-off-by: Michal Hocko 
> > > ---
> > >  include/trace/events/vmscan.h | 20 ++--
> > >  mm/vmscan.c   |  2 +-
> > >  2 files changed, 15 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > > index 6af4dae46db2..cc0b4c456c78 100644
> > > --- a/include/trace/events/vmscan.h
> > > +++ b/include/trace/events/vmscan.h
> > > @@ -36,6 +36,14 @@
> > >   (RECLAIM_WB_ASYNC) \
> > >   )
> > > 
> > > +#define show_lru_name(lru) \
> > > + __print_symbolic(lru, \
> > > + {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> > > + {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> > > + {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> > > + {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> > > + {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> > > +
> > 
> > Does this work with external tools such as trace-cmd, i.e. does it export
> > the correct format file?
> 
> How do I find out?

Well, I've just checked the format file and it says
print fmt: "isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s", REC->isolate_mode, 
REC->classzone_idx, REC->order, REC->nr_requested, REC->nr_scanned, 
REC->nr_skipped, REC->nr_taken, __print_symbolic(REC->lru, {LRU_INACTIVE_ANON, 
"LRU_INACTIVE_ANON"}, {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, {LRU_INACTIVE_FILE, 
"LRU_INACTIVE_FILE"}, {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, {LRU_UNEVICTABLE, 
"LRU_UNEVICTABLE"})

So the tool should be OK as long as it can find values for LRU_*
constants. Is this what is the problem?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Tue 03-01-17 21:47:45, Michal Hocko wrote:
> On Tue 03-01-17 18:08:58, Vlastimil Babka wrote:
> > On 12/28/2016 04:30 PM, Michal Hocko wrote:
> > > From: Michal Hocko 
> > > 
> > > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > > from is file or anonymous but we do not know which LRU this is. It is
> > > useful to know whether the list is file or anonymous as well. Change
> > > the tracepoint to show symbolic names of the lru rather.
> > > 
> > > Signed-off-by: Michal Hocko 
> > > ---
> > >  include/trace/events/vmscan.h | 20 ++--
> > >  mm/vmscan.c   |  2 +-
> > >  2 files changed, 15 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > > index 6af4dae46db2..cc0b4c456c78 100644
> > > --- a/include/trace/events/vmscan.h
> > > +++ b/include/trace/events/vmscan.h
> > > @@ -36,6 +36,14 @@
> > >   (RECLAIM_WB_ASYNC) \
> > >   )
> > > 
> > > +#define show_lru_name(lru) \
> > > + __print_symbolic(lru, \
> > > + {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> > > + {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> > > + {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> > > + {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> > > + {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> > > +
> > 
> > Does this work with external tools such as trace-cmd, i.e. does it export
> > the correct format file?
> 
> How do I find out?

Well, I've just checked the format file and it says
print fmt: "isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s", REC->isolate_mode, 
REC->classzone_idx, REC->order, REC->nr_requested, REC->nr_scanned, 
REC->nr_skipped, REC->nr_taken, __print_symbolic(REC->lru, {LRU_INACTIVE_ANON, 
"LRU_INACTIVE_ANON"}, {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, {LRU_INACTIVE_FILE, 
"LRU_INACTIVE_FILE"}, {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, {LRU_UNEVICTABLE, 
"LRU_UNEVICTABLE"})

So the tool should be OK as long as it can find values for LRU_*
constants. Is this what is the problem?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Tue 03-01-17 18:08:58, Vlastimil Babka wrote:
> On 12/28/2016 04:30 PM, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > from is file or anonymous but we do not know which LRU this is. It is
> > useful to know whether the list is file or anonymous as well. Change
> > the tracepoint to show symbolic names of the lru rather.
> > 
> > Signed-off-by: Michal Hocko 
> > ---
> >  include/trace/events/vmscan.h | 20 ++--
> >  mm/vmscan.c   |  2 +-
> >  2 files changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > index 6af4dae46db2..cc0b4c456c78 100644
> > --- a/include/trace/events/vmscan.h
> > +++ b/include/trace/events/vmscan.h
> > @@ -36,6 +36,14 @@
> > (RECLAIM_WB_ASYNC) \
> > )
> > 
> > +#define show_lru_name(lru) \
> > +   __print_symbolic(lru, \
> > +   {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> > +   {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> > +   {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> > +   {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> > +   {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> > +
> 
> Does this work with external tools such as trace-cmd, i.e. does it export
> the correct format file?

How do I find out?

> I wouldn't expect it to be that easy to avoid the EM()/EMe() dance :)

Well, I will not pretend I understand the EM dances...

> Also can we make the symbolic names lower_case and without the LRU_ prefix?
> I think it's more consistent with other mm tracepoints, shorter and nicer.

OK, will make it lowercase without the LRU_ prefix.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Tue 03-01-17 18:08:58, Vlastimil Babka wrote:
> On 12/28/2016 04:30 PM, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > from is file or anonymous but we do not know which LRU this is. It is
> > useful to know whether the list is file or anonymous as well. Change
> > the tracepoint to show symbolic names of the lru rather.
> > 
> > Signed-off-by: Michal Hocko 
> > ---
> >  include/trace/events/vmscan.h | 20 ++--
> >  mm/vmscan.c   |  2 +-
> >  2 files changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > index 6af4dae46db2..cc0b4c456c78 100644
> > --- a/include/trace/events/vmscan.h
> > +++ b/include/trace/events/vmscan.h
> > @@ -36,6 +36,14 @@
> > (RECLAIM_WB_ASYNC) \
> > )
> > 
> > +#define show_lru_name(lru) \
> > +   __print_symbolic(lru, \
> > +   {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> > +   {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> > +   {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> > +   {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> > +   {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> > +
> 
> Does this work with external tools such as trace-cmd, i.e. does it export
> the correct format file?

How do I find out?

> I wouldn't expect it to be that easy to avoid the EM()/EMe() dance :)

Well, I will not pretend I understand the EM dances...

> Also can we make the symbolic names lower_case and without the LRU_ prefix?
> I think it's more consistent with other mm tracepoints, shorter and nicer.

OK, will make it lowercase without the LRU_ prefix.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Vlastimil Babka]

On 12/28/2016 04:30 PM, Michal Hocko wrote:

From: Michal Hocko 

mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
from is file or anonymous but we do not know which LRU this is. It is
useful to know whether the list is file or anonymous as well. Change
the tracepoint to show symbolic names of the lru rather.

Signed-off-by: Michal Hocko 
---
 include/trace/events/vmscan.h | 20 ++--
 mm/vmscan.c   |  2 +-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
index 6af4dae46db2..cc0b4c456c78 100644
--- a/include/trace/events/vmscan.h
+++ b/include/trace/events/vmscan.h
@@ -36,6 +36,14 @@
(RECLAIM_WB_ASYNC) \
)

+#define show_lru_name(lru) \
+   __print_symbolic(lru, \
+   {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
+   {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
+   {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
+   {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
+   {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
+


Does this work with external tools such as trace-cmd, i.e. does it export the 
correct format file? I wouldn't expect it to be that easy to avoid the 
EM()/EMe() dance :)


Also can we make the symbolic names lower_case and without the LRU_ prefix? I 
think it's more consistent with other mm tracepoints, shorter and nicer.



 TRACE_EVENT(mm_vmscan_kswapd_sleep,

TP_PROTO(int nid),
@@ -277,9 +285,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
unsigned long nr_skipped,
unsigned long nr_taken,
isolate_mode_t isolate_mode,
-   int file),
+   int lru),

-   TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
nr_taken, isolate_mode, file),
+   TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
nr_taken, isolate_mode, lru),

TP_STRUCT__entry(
__field(int, classzone_idx)
@@ -289,7 +297,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
__field(unsigned long, nr_skipped)
__field(unsigned long, nr_taken)
__field(isolate_mode_t, isolate_mode)
-   __field(int, file)
+   __field(int, lru)
),

TP_fast_assign(
@@ -300,10 +308,10 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
__entry->nr_skipped = nr_skipped;
__entry->nr_taken = nr_taken;
__entry->isolate_mode = isolate_mode;
-   __entry->file = file;
+   __entry->lru = lru;
),

-   TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
nr_scanned=%lu nr_skipped=%lu nr_taken=%lu file=%d",
+   TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s",
__entry->isolate_mode,
__entry->classzone_idx,
__entry->order,
@@ -311,7 +319,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
__entry->nr_scanned,
__entry->nr_skipped,
__entry->nr_taken,
-   __entry->file)
+   show_lru_name(__entry->lru))
 );

 TRACE_EVENT(mm_vmscan_writepage,
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 4f7c0d66d629..3f0774f30a42 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1500,7 +1500,7 @@ static unsigned long isolate_lru_pages(unsigned long 
nr_to_scan,
}
*nr_scanned = scan + total_skipped;
trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan, 
scan,
-   skipped, nr_taken, mode, is_file_lru(lru));
+   skipped, nr_taken, mode, lru);
update_lru_sizes(lruvec, lru, nr_zone_taken, nr_taken);
return nr_taken;
 }

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Vlastimil Babka]

On 12/28/2016 04:30 PM, Michal Hocko wrote:

From: Michal Hocko 

mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
from is file or anonymous but we do not know which LRU this is. It is
useful to know whether the list is file or anonymous as well. Change
the tracepoint to show symbolic names of the lru rather.

Signed-off-by: Michal Hocko 
---
 include/trace/events/vmscan.h | 20 ++--
 mm/vmscan.c   |  2 +-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
index 6af4dae46db2..cc0b4c456c78 100644
--- a/include/trace/events/vmscan.h
+++ b/include/trace/events/vmscan.h
@@ -36,6 +36,14 @@
(RECLAIM_WB_ASYNC) \
)

+#define show_lru_name(lru) \
+   __print_symbolic(lru, \
+   {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
+   {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
+   {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
+   {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
+   {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
+


Does this work with external tools such as trace-cmd, i.e. does it export the 
correct format file? I wouldn't expect it to be that easy to avoid the 
EM()/EMe() dance :)


Also can we make the symbolic names lower_case and without the LRU_ prefix? I 
think it's more consistent with other mm tracepoints, shorter and nicer.



 TRACE_EVENT(mm_vmscan_kswapd_sleep,

TP_PROTO(int nid),
@@ -277,9 +285,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
unsigned long nr_skipped,
unsigned long nr_taken,
isolate_mode_t isolate_mode,
-   int file),
+   int lru),

-   TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
nr_taken, isolate_mode, file),
+   TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
nr_taken, isolate_mode, lru),

TP_STRUCT__entry(
__field(int, classzone_idx)
@@ -289,7 +297,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
__field(unsigned long, nr_skipped)
__field(unsigned long, nr_taken)
__field(isolate_mode_t, isolate_mode)
-   __field(int, file)
+   __field(int, lru)
),

TP_fast_assign(
@@ -300,10 +308,10 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
__entry->nr_skipped = nr_skipped;
__entry->nr_taken = nr_taken;
__entry->isolate_mode = isolate_mode;
-   __entry->file = file;
+   __entry->lru = lru;
),

-   TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
nr_scanned=%lu nr_skipped=%lu nr_taken=%lu file=%d",
+   TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s",
__entry->isolate_mode,
__entry->classzone_idx,
__entry->order,
@@ -311,7 +319,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
__entry->nr_scanned,
__entry->nr_skipped,
__entry->nr_taken,
-   __entry->file)
+   show_lru_name(__entry->lru))
 );

 TRACE_EVENT(mm_vmscan_writepage,
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 4f7c0d66d629..3f0774f30a42 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1500,7 +1500,7 @@ static unsigned long isolate_lru_pages(unsigned long 
nr_to_scan,
}
*nr_scanned = scan + total_skipped;
trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan, 
scan,
-   skipped, nr_taken, mode, is_file_lru(lru));
+   skipped, nr_taken, mode, lru);
update_lru_sizes(lruvec, lru, nr_zone_taken, nr_taken);
return nr_taken;
 }

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Fri 30-12-16 10:56:25, Minchan Kim wrote:
> On Thu, Dec 29, 2016 at 08:56:49AM +0100, Michal Hocko wrote:
> > On Thu 29-12-16 15:02:04, Minchan Kim wrote:
> > > On Wed, Dec 28, 2016 at 04:30:29PM +0100, Michal Hocko wrote:
> > > > From: Michal Hocko 
> > > > 
> > > > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > > > from is file or anonymous but we do not know which LRU this is. It is
> > > > useful to know whether the list is file or anonymous as well. Change
> > > > the tracepoint to show symbolic names of the lru rather.
> > > > 
> > > > Signed-off-by: Michal Hocko 
> > > 
> > > Not exactly same with this but idea is almost same.
> > > I used almost same tracepoint to investigate agging(i.e., deactivating) 
> > > problem
> > > in 32b kernel with node-lru.
> > > It was enough. Namely, I didn't need tracepoint in shrink_active_list 
> > > like your
> > > first patch.
> > > Your first patch is more straightforwad and information. But as you 
> > > introduced
> > > this patch, I want to ask in here.
> > > Isn't it enough with this patch without your first one to find a such 
> > > problem?
> > 
> > I assume this should be a reply to
> > http://lkml.kernel.org/r/20161228153032.10821-8-mho...@kernel.org, right?
> 
> I don't know my browser says "No such Message-ID known"

Hmm, not sure why it didn't get archived at lkml.kernel.org.
I meant https://lkml.org/lkml/2016/12/28/167
 
> > And you are right that for the particular problem it was enough to have
> > a tracepoint inside inactive_list_is_low and shrink_active_list one
> > wasn't really needed. On the other hand aging issues are really hard to
> 
> What kinds of aging issue? What's the problem? How such tracepoint can help?
> Please describe.

If you do not see that active list is shrunk then you do not know why it
is not shrunk. It might be a active/inactive ratio or just a plan bug
like the 32b issue me and you were debugging.

> > debug as well and so I think that both are useful. The first one tell us
> > _why_ we do aging while the later _how_ we do that.
> 
> Solve reported problem first you already knew. It would be no doubt
> to merge and then send other patches about "it might be useful" with
> useful scenario.

I am not sure I understand. The point of tracepoints is to be
pro-actively helpful not only to add something that has been useful in
one-off cases. A particular debugging session might be really helpful to
tell us what we are missing and this was the case here to a large part.
Once I was looking there I just wanted to save the pain of adding more
debugging information in future and allow people to debug their issue
without forcing them to recompile the kernel. I believe this is one of
the strong usecases for tracepoints in the first place.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Fri 30-12-16 10:56:25, Minchan Kim wrote:
> On Thu, Dec 29, 2016 at 08:56:49AM +0100, Michal Hocko wrote:
> > On Thu 29-12-16 15:02:04, Minchan Kim wrote:
> > > On Wed, Dec 28, 2016 at 04:30:29PM +0100, Michal Hocko wrote:
> > > > From: Michal Hocko 
> > > > 
> > > > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > > > from is file or anonymous but we do not know which LRU this is. It is
> > > > useful to know whether the list is file or anonymous as well. Change
> > > > the tracepoint to show symbolic names of the lru rather.
> > > > 
> > > > Signed-off-by: Michal Hocko 
> > > 
> > > Not exactly same with this but idea is almost same.
> > > I used almost same tracepoint to investigate agging(i.e., deactivating) 
> > > problem
> > > in 32b kernel with node-lru.
> > > It was enough. Namely, I didn't need tracepoint in shrink_active_list 
> > > like your
> > > first patch.
> > > Your first patch is more straightforwad and information. But as you 
> > > introduced
> > > this patch, I want to ask in here.
> > > Isn't it enough with this patch without your first one to find a such 
> > > problem?
> > 
> > I assume this should be a reply to
> > http://lkml.kernel.org/r/20161228153032.10821-8-mho...@kernel.org, right?
> 
> I don't know my browser says "No such Message-ID known"

Hmm, not sure why it didn't get archived at lkml.kernel.org.
I meant https://lkml.org/lkml/2016/12/28/167
 
> > And you are right that for the particular problem it was enough to have
> > a tracepoint inside inactive_list_is_low and shrink_active_list one
> > wasn't really needed. On the other hand aging issues are really hard to
> 
> What kinds of aging issue? What's the problem? How such tracepoint can help?
> Please describe.

If you do not see that active list is shrunk then you do not know why it
is not shrunk. It might be a active/inactive ratio or just a plan bug
like the 32b issue me and you were debugging.

> > debug as well and so I think that both are useful. The first one tell us
> > _why_ we do aging while the later _how_ we do that.
> 
> Solve reported problem first you already knew. It would be no doubt
> to merge and then send other patches about "it might be useful" with
> useful scenario.

I am not sure I understand. The point of tracepoints is to be
pro-actively helpful not only to add something that has been useful in
one-off cases. A particular debugging session might be really helpful to
tell us what we are missing and this was the case here to a large part.
Once I was looking there I just wanted to save the pain of adding more
debugging information in future and allow people to debug their issue
without forcing them to recompile the kernel. I believe this is one of
the strong usecases for tracepoints in the first place.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Minchan Kim]

On Thu, Dec 29, 2016 at 08:56:49AM +0100, Michal Hocko wrote:
> On Thu 29-12-16 15:02:04, Minchan Kim wrote:
> > On Wed, Dec 28, 2016 at 04:30:29PM +0100, Michal Hocko wrote:
> > > From: Michal Hocko 
> > > 
> > > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > > from is file or anonymous but we do not know which LRU this is. It is
> > > useful to know whether the list is file or anonymous as well. Change
> > > the tracepoint to show symbolic names of the lru rather.
> > > 
> > > Signed-off-by: Michal Hocko 
> > 
> > Not exactly same with this but idea is almost same.
> > I used almost same tracepoint to investigate agging(i.e., deactivating) 
> > problem
> > in 32b kernel with node-lru.
> > It was enough. Namely, I didn't need tracepoint in shrink_active_list like 
> > your
> > first patch.
> > Your first patch is more straightforwad and information. But as you 
> > introduced
> > this patch, I want to ask in here.
> > Isn't it enough with this patch without your first one to find a such 
> > problem?
> 
> I assume this should be a reply to
> http://lkml.kernel.org/r/20161228153032.10821-8-mho...@kernel.org, right?

I don't know my browser says "No such Message-ID known"

> And you are right that for the particular problem it was enough to have
> a tracepoint inside inactive_list_is_low and shrink_active_list one
> wasn't really needed. On the other hand aging issues are really hard to

What kinds of aging issue? What's the problem? How such tracepoint can help?
Please describe.

> debug as well and so I think that both are useful. The first one tell us
> _why_ we do aging while the later _how_ we do that.

Solve reported problem first you already knew. It would be no doubt
to merge and then send other patches about "it might be useful" with
useful scenario.

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Minchan Kim]

On Thu, Dec 29, 2016 at 08:56:49AM +0100, Michal Hocko wrote:
> On Thu 29-12-16 15:02:04, Minchan Kim wrote:
> > On Wed, Dec 28, 2016 at 04:30:29PM +0100, Michal Hocko wrote:
> > > From: Michal Hocko 
> > > 
> > > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > > from is file or anonymous but we do not know which LRU this is. It is
> > > useful to know whether the list is file or anonymous as well. Change
> > > the tracepoint to show symbolic names of the lru rather.
> > > 
> > > Signed-off-by: Michal Hocko 
> > 
> > Not exactly same with this but idea is almost same.
> > I used almost same tracepoint to investigate agging(i.e., deactivating) 
> > problem
> > in 32b kernel with node-lru.
> > It was enough. Namely, I didn't need tracepoint in shrink_active_list like 
> > your
> > first patch.
> > Your first patch is more straightforwad and information. But as you 
> > introduced
> > this patch, I want to ask in here.
> > Isn't it enough with this patch without your first one to find a such 
> > problem?
> 
> I assume this should be a reply to
> http://lkml.kernel.org/r/20161228153032.10821-8-mho...@kernel.org, right?

I don't know my browser says "No such Message-ID known"

> And you are right that for the particular problem it was enough to have
> a tracepoint inside inactive_list_is_low and shrink_active_list one
> wasn't really needed. On the other hand aging issues are really hard to

What kinds of aging issue? What's the problem? How such tracepoint can help?
Please describe.

> debug as well and so I think that both are useful. The first one tell us
> _why_ we do aging while the later _how_ we do that.

Solve reported problem first you already knew. It would be no doubt
to merge and then send other patches about "it might be useful" with
useful scenario.

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Hillf Danton]

On Wednesday, December 28, 2016 11:30 PM Michal Hocko wrote: 
> From: Michal Hocko 
> 
> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> from is file or anonymous but we do not know which LRU this is. It is
> useful to know whether the list is file or anonymous as well. Change
> the tracepoint to show symbolic names of the lru rather.
> 
> Signed-off-by: Michal Hocko 
> ---
Acked-by: Hillf Danton  

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Hillf Danton]

On Wednesday, December 28, 2016 11:30 PM Michal Hocko wrote: 
> From: Michal Hocko 
> 
> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> from is file or anonymous but we do not know which LRU this is. It is
> useful to know whether the list is file or anonymous as well. Change
> the tracepoint to show symbolic names of the lru rather.
> 
> Signed-off-by: Michal Hocko 
> ---
Acked-by: Hillf Danton  

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Thu 29-12-16 15:02:04, Minchan Kim wrote:
> On Wed, Dec 28, 2016 at 04:30:29PM +0100, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > from is file or anonymous but we do not know which LRU this is. It is
> > useful to know whether the list is file or anonymous as well. Change
> > the tracepoint to show symbolic names of the lru rather.
> > 
> > Signed-off-by: Michal Hocko 
> 
> Not exactly same with this but idea is almost same.
> I used almost same tracepoint to investigate agging(i.e., deactivating) 
> problem
> in 32b kernel with node-lru.
> It was enough. Namely, I didn't need tracepoint in shrink_active_list like 
> your
> first patch.
> Your first patch is more straightforwad and information. But as you introduced
> this patch, I want to ask in here.
> Isn't it enough with this patch without your first one to find a such problem?

I assume this should be a reply to
http://lkml.kernel.org/r/20161228153032.10821-8-mho...@kernel.org, right?
And you are right that for the particular problem it was enough to have
a tracepoint inside inactive_list_is_low and shrink_active_list one
wasn't really needed. On the other hand aging issues are really hard to
debug as well and so I think that both are useful. The first one tell us
_why_ we do aging while the later _how_ we do that.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Thu 29-12-16 15:02:04, Minchan Kim wrote:
> On Wed, Dec 28, 2016 at 04:30:29PM +0100, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > from is file or anonymous but we do not know which LRU this is. It is
> > useful to know whether the list is file or anonymous as well. Change
> > the tracepoint to show symbolic names of the lru rather.
> > 
> > Signed-off-by: Michal Hocko 
> 
> Not exactly same with this but idea is almost same.
> I used almost same tracepoint to investigate agging(i.e., deactivating) 
> problem
> in 32b kernel with node-lru.
> It was enough. Namely, I didn't need tracepoint in shrink_active_list like 
> your
> first patch.
> Your first patch is more straightforwad and information. But as you introduced
> this patch, I want to ask in here.
> Isn't it enough with this patch without your first one to find a such problem?

I assume this should be a reply to
http://lkml.kernel.org/r/20161228153032.10821-8-mho...@kernel.org, right?
And you are right that for the particular problem it was enough to have
a tracepoint inside inactive_list_is_low and shrink_active_list one
wasn't really needed. On the other hand aging issues are really hard to
debug as well and so I think that both are useful. The first one tell us
_why_ we do aging while the later _how_ we do that.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Minchan Kim]

On Wed, Dec 28, 2016 at 04:30:29PM +0100, Michal Hocko wrote:
> From: Michal Hocko 
> 
> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> from is file or anonymous but we do not know which LRU this is. It is
> useful to know whether the list is file or anonymous as well. Change
> the tracepoint to show symbolic names of the lru rather.
> 
> Signed-off-by: Michal Hocko 

Not exactly same with this but idea is almost same.
I used almost same tracepoint to investigate agging(i.e., deactivating) problem
in 32b kernel with node-lru.
It was enough. Namely, I didn't need tracepoint in shrink_active_list like your
first patch.
Your first patch is more straightforwad and information. But as you introduced
this patch, I want to ask in here.
Isn't it enough with this patch without your first one to find a such problem?

Thanks.

> ---
>  include/trace/events/vmscan.h | 20 ++--
>  mm/vmscan.c   |  2 +-
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> index 6af4dae46db2..cc0b4c456c78 100644
> --- a/include/trace/events/vmscan.h
> +++ b/include/trace/events/vmscan.h
> @@ -36,6 +36,14 @@
>   (RECLAIM_WB_ASYNC) \
>   )
>  
> +#define show_lru_name(lru) \
> + __print_symbolic(lru, \
> + {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> + {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> + {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> + {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> + {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> +
>  TRACE_EVENT(mm_vmscan_kswapd_sleep,
>  
>   TP_PROTO(int nid),
> @@ -277,9 +285,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   unsigned long nr_skipped,
>   unsigned long nr_taken,
>   isolate_mode_t isolate_mode,
> - int file),
> + int lru),
>  
> - TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
> nr_taken, isolate_mode, file),
> + TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
> nr_taken, isolate_mode, lru),
>  
>   TP_STRUCT__entry(
>   __field(int, classzone_idx)
> @@ -289,7 +297,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __field(unsigned long, nr_skipped)
>   __field(unsigned long, nr_taken)
>   __field(isolate_mode_t, isolate_mode)
> - __field(int, file)
> + __field(int, lru)
>   ),
>  
>   TP_fast_assign(
> @@ -300,10 +308,10 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __entry->nr_skipped = nr_skipped;
>   __entry->nr_taken = nr_taken;
>   __entry->isolate_mode = isolate_mode;
> - __entry->file = file;
> + __entry->lru = lru;
>   ),
>  
> - TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu file=%d",
> + TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s",
>   __entry->isolate_mode,
>   __entry->classzone_idx,
>   __entry->order,
> @@ -311,7 +319,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __entry->nr_scanned,
>   __entry->nr_skipped,
>   __entry->nr_taken,
> - __entry->file)
> + show_lru_name(__entry->lru))
>  );
>  
>  TRACE_EVENT(mm_vmscan_writepage,
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 4f7c0d66d629..3f0774f30a42 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1500,7 +1500,7 @@ static unsigned long isolate_lru_pages(unsigned long 
> nr_to_scan,
>   }
>   *nr_scanned = scan + total_skipped;
>   trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan, 
> scan,
> - skipped, nr_taken, mode, is_file_lru(lru));
> + skipped, nr_taken, mode, lru);
>   update_lru_sizes(lruvec, lru, nr_zone_taken, nr_taken);
>   return nr_taken;
>  }
> -- 
> 2.10.2
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Minchan Kim]

On Wed, Dec 28, 2016 at 04:30:29PM +0100, Michal Hocko wrote:
> From: Michal Hocko 
> 
> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> from is file or anonymous but we do not know which LRU this is. It is
> useful to know whether the list is file or anonymous as well. Change
> the tracepoint to show symbolic names of the lru rather.
> 
> Signed-off-by: Michal Hocko 

Not exactly same with this but idea is almost same.
I used almost same tracepoint to investigate agging(i.e., deactivating) problem
in 32b kernel with node-lru.
It was enough. Namely, I didn't need tracepoint in shrink_active_list like your
first patch.
Your first patch is more straightforwad and information. But as you introduced
this patch, I want to ask in here.
Isn't it enough with this patch without your first one to find a such problem?

Thanks.

> ---
>  include/trace/events/vmscan.h | 20 ++--
>  mm/vmscan.c   |  2 +-
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> index 6af4dae46db2..cc0b4c456c78 100644
> --- a/include/trace/events/vmscan.h
> +++ b/include/trace/events/vmscan.h
> @@ -36,6 +36,14 @@
>   (RECLAIM_WB_ASYNC) \
>   )
>  
> +#define show_lru_name(lru) \
> + __print_symbolic(lru, \
> + {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> + {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> + {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> + {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> + {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> +
>  TRACE_EVENT(mm_vmscan_kswapd_sleep,
>  
>   TP_PROTO(int nid),
> @@ -277,9 +285,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   unsigned long nr_skipped,
>   unsigned long nr_taken,
>   isolate_mode_t isolate_mode,
> - int file),
> + int lru),
>  
> - TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
> nr_taken, isolate_mode, file),
> + TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
> nr_taken, isolate_mode, lru),
>  
>   TP_STRUCT__entry(
>   __field(int, classzone_idx)
> @@ -289,7 +297,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __field(unsigned long, nr_skipped)
>   __field(unsigned long, nr_taken)
>   __field(isolate_mode_t, isolate_mode)
> - __field(int, file)
> + __field(int, lru)
>   ),
>  
>   TP_fast_assign(
> @@ -300,10 +308,10 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __entry->nr_skipped = nr_skipped;
>   __entry->nr_taken = nr_taken;
>   __entry->isolate_mode = isolate_mode;
> - __entry->file = file;
> + __entry->lru = lru;
>   ),
>  
> - TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu file=%d",
> + TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s",
>   __entry->isolate_mode,
>   __entry->classzone_idx,
>   __entry->order,
> @@ -311,7 +319,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __entry->nr_scanned,
>   __entry->nr_skipped,
>   __entry->nr_taken,
> - __entry->file)
> + show_lru_name(__entry->lru))
>  );
>  
>  TRACE_EVENT(mm_vmscan_writepage,
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 4f7c0d66d629..3f0774f30a42 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1500,7 +1500,7 @@ static unsigned long isolate_lru_pages(unsigned long 
> nr_to_scan,
>   }
>   *nr_scanned = scan + total_skipped;
>   trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan, 
> scan,
> - skipped, nr_taken, mode, is_file_lru(lru));
> + skipped, nr_taken, mode, lru);
>   update_lru_sizes(lruvec, lru, nr_zone_taken, nr_taken);
>   return nr_taken;
>  }
> -- 
> 2.10.2
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Wed 28-12-16 18:40:16, Nikolay Borisov wrote:
[...]
> "
> It is useful to know whether the list is active or inactive, since we
> are using the same function to isolate pages from both of them and it's
> hard to distinguish otherwise.
> "

OK, updated. Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Wed 28-12-16 18:40:16, Nikolay Borisov wrote:
[...]
> "
> It is useful to know whether the list is active or inactive, since we
> are using the same function to isolate pages from both of them and it's
> hard to distinguish otherwise.
> "

OK, updated. Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Nikolay Borisov]

On 28.12.2016 18:00, Michal Hocko wrote:
> On Wed 28-12-16 17:50:31, Nikolay Borisov wrote:
>>
>>
>> On 28.12.2016 17:30, Michal Hocko wrote:
>>> From: Michal Hocko 
>>>
>>> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
>>> from is file or anonymous but we do not know which LRU this is. It is
>>> useful to know whether the list is file or anonymous as well. Change
>>
>> Maybe you wanted to say whether the list is ACTIVE/INACTIVE ?
> 
> You are right. I will update the wording to:
> "
> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> from is file or anonymous but we do not know which LRU this is. It is
> useful to know whether the list is active or inactive as well as we
> use the same function to isolate pages for both of them. Change
> the tracepoint to show symbolic names of the lru rather.
> "
> 
> Does it sound better?

It's better. Just one more nit about the " as well as we
use the same function to isolate pages for both of them"

I think this can be reworded better. The way I understand is - it's
better to know whether it's active/inactive since we are using the same
function to do both, correct? If so then then perhaps the following is a
bit more clear:

"
It is useful to know whether the list is active or inactive, since we
are using the same function to isolate pages from both of them and it's
hard to distinguish otherwise.
"

But as I said - it's a minor nit.


> 
> Thanks!
> 

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Nikolay Borisov]

On 28.12.2016 18:00, Michal Hocko wrote:
> On Wed 28-12-16 17:50:31, Nikolay Borisov wrote:
>>
>>
>> On 28.12.2016 17:30, Michal Hocko wrote:
>>> From: Michal Hocko 
>>>
>>> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
>>> from is file or anonymous but we do not know which LRU this is. It is
>>> useful to know whether the list is file or anonymous as well. Change
>>
>> Maybe you wanted to say whether the list is ACTIVE/INACTIVE ?
> 
> You are right. I will update the wording to:
> "
> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> from is file or anonymous but we do not know which LRU this is. It is
> useful to know whether the list is active or inactive as well as we
> use the same function to isolate pages for both of them. Change
> the tracepoint to show symbolic names of the lru rather.
> "
> 
> Does it sound better?

It's better. Just one more nit about the " as well as we
use the same function to isolate pages for both of them"

I think this can be reworded better. The way I understand is - it's
better to know whether it's active/inactive since we are using the same
function to do both, correct? If so then then perhaps the following is a
bit more clear:

"
It is useful to know whether the list is active or inactive, since we
are using the same function to isolate pages from both of them and it's
hard to distinguish otherwise.
"

But as I said - it's a minor nit.


> 
> Thanks!
> 

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Wed 28-12-16 17:50:31, Nikolay Borisov wrote:
> 
> 
> On 28.12.2016 17:30, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > from is file or anonymous but we do not know which LRU this is. It is
> > useful to know whether the list is file or anonymous as well. Change
>
> Maybe you wanted to say whether the list is ACTIVE/INACTIVE ?

You are right. I will update the wording to:
"
mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
from is file or anonymous but we do not know which LRU this is. It is
useful to know whether the list is active or inactive as well as we
use the same function to isolate pages for both of them. Change
the tracepoint to show symbolic names of the lru rather.
"

Does it sound better?

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Michal Hocko]

On Wed 28-12-16 17:50:31, Nikolay Borisov wrote:
> 
> 
> On 28.12.2016 17:30, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> > from is file or anonymous but we do not know which LRU this is. It is
> > useful to know whether the list is file or anonymous as well. Change
>
> Maybe you wanted to say whether the list is ACTIVE/INACTIVE ?

You are right. I will update the wording to:
"
mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
from is file or anonymous but we do not know which LRU this is. It is
useful to know whether the list is active or inactive as well as we
use the same function to isolate pages for both of them. Change
the tracepoint to show symbolic names of the lru rather.
"

Does it sound better?

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Nikolay Borisov]

On 28.12.2016 17:30, Michal Hocko wrote:
> From: Michal Hocko 
> 
> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> from is file or anonymous but we do not know which LRU this is. It is
> useful to know whether the list is file or anonymous as well. Change
Maybe you wanted to say whether the list is ACTIVE/INACTIVE ?

> the tracepoint to show symbolic names of the lru rather.
> 
> Signed-off-by: Michal Hocko 
> ---
>  include/trace/events/vmscan.h | 20 ++--
>  mm/vmscan.c   |  2 +-
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> index 6af4dae46db2..cc0b4c456c78 100644
> --- a/include/trace/events/vmscan.h
> +++ b/include/trace/events/vmscan.h
> @@ -36,6 +36,14 @@
>   (RECLAIM_WB_ASYNC) \
>   )
>  
> +#define show_lru_name(lru) \
> + __print_symbolic(lru, \
> + {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> + {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> + {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> + {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> + {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> +
>  TRACE_EVENT(mm_vmscan_kswapd_sleep,
>  
>   TP_PROTO(int nid),
> @@ -277,9 +285,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   unsigned long nr_skipped,
>   unsigned long nr_taken,
>   isolate_mode_t isolate_mode,
> - int file),
> + int lru),
>  
> - TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
> nr_taken, isolate_mode, file),
> + TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
> nr_taken, isolate_mode, lru),
>  
>   TP_STRUCT__entry(
>   __field(int, classzone_idx)
> @@ -289,7 +297,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __field(unsigned long, nr_skipped)
>   __field(unsigned long, nr_taken)
>   __field(isolate_mode_t, isolate_mode)
> - __field(int, file)
> + __field(int, lru)
>   ),
>  
>   TP_fast_assign(
> @@ -300,10 +308,10 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __entry->nr_skipped = nr_skipped;
>   __entry->nr_taken = nr_taken;
>   __entry->isolate_mode = isolate_mode;
> - __entry->file = file;
> + __entry->lru = lru;
>   ),
>  
> - TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu file=%d",
> + TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s",
>   __entry->isolate_mode,
>   __entry->classzone_idx,
>   __entry->order,
> @@ -311,7 +319,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __entry->nr_scanned,
>   __entry->nr_skipped,
>   __entry->nr_taken,
> - __entry->file)
> + show_lru_name(__entry->lru))
>  );
>  
>  TRACE_EVENT(mm_vmscan_writepage,
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 4f7c0d66d629..3f0774f30a42 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1500,7 +1500,7 @@ static unsigned long isolate_lru_pages(unsigned long 
> nr_to_scan,
>   }
>   *nr_scanned = scan + total_skipped;
>   trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan, 
> scan,
> - skipped, nr_taken, mode, is_file_lru(lru));
> + skipped, nr_taken, mode, lru);
>   update_lru_sizes(lruvec, lru, nr_zone_taken, nr_taken);
>   return nr_taken;
>  }
> 

Re: [PATCH 4/7] mm, vmscan: show LRU name in mm_vmscan_lru_isolate tracepoint

[Nikolay Borisov]

On 28.12.2016 17:30, Michal Hocko wrote:
> From: Michal Hocko 
> 
> mm_vmscan_lru_isolate currently prints only whether the LRU we isolate
> from is file or anonymous but we do not know which LRU this is. It is
> useful to know whether the list is file or anonymous as well. Change
Maybe you wanted to say whether the list is ACTIVE/INACTIVE ?

> the tracepoint to show symbolic names of the lru rather.
> 
> Signed-off-by: Michal Hocko 
> ---
>  include/trace/events/vmscan.h | 20 ++--
>  mm/vmscan.c   |  2 +-
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> index 6af4dae46db2..cc0b4c456c78 100644
> --- a/include/trace/events/vmscan.h
> +++ b/include/trace/events/vmscan.h
> @@ -36,6 +36,14 @@
>   (RECLAIM_WB_ASYNC) \
>   )
>  
> +#define show_lru_name(lru) \
> + __print_symbolic(lru, \
> + {LRU_INACTIVE_ANON, "LRU_INACTIVE_ANON"}, \
> + {LRU_ACTIVE_ANON, "LRU_ACTIVE_ANON"}, \
> + {LRU_INACTIVE_FILE, "LRU_INACTIVE_FILE"}, \
> + {LRU_ACTIVE_FILE, "LRU_ACTIVE_FILE"}, \
> + {LRU_UNEVICTABLE, "LRU_UNEVICTABLE"})
> +
>  TRACE_EVENT(mm_vmscan_kswapd_sleep,
>  
>   TP_PROTO(int nid),
> @@ -277,9 +285,9 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   unsigned long nr_skipped,
>   unsigned long nr_taken,
>   isolate_mode_t isolate_mode,
> - int file),
> + int lru),
>  
> - TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
> nr_taken, isolate_mode, file),
> + TP_ARGS(classzone_idx, order, nr_requested, nr_scanned, nr_skipped, 
> nr_taken, isolate_mode, lru),
>  
>   TP_STRUCT__entry(
>   __field(int, classzone_idx)
> @@ -289,7 +297,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __field(unsigned long, nr_skipped)
>   __field(unsigned long, nr_taken)
>   __field(isolate_mode_t, isolate_mode)
> - __field(int, file)
> + __field(int, lru)
>   ),
>  
>   TP_fast_assign(
> @@ -300,10 +308,10 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __entry->nr_skipped = nr_skipped;
>   __entry->nr_taken = nr_taken;
>   __entry->isolate_mode = isolate_mode;
> - __entry->file = file;
> + __entry->lru = lru;
>   ),
>  
> - TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu file=%d",
> + TP_printk("isolate_mode=%d classzone=%d order=%d nr_requested=%lu 
> nr_scanned=%lu nr_skipped=%lu nr_taken=%lu lru=%s",
>   __entry->isolate_mode,
>   __entry->classzone_idx,
>   __entry->order,
> @@ -311,7 +319,7 @@ TRACE_EVENT(mm_vmscan_lru_isolate,
>   __entry->nr_scanned,
>   __entry->nr_skipped,
>   __entry->nr_taken,
> - __entry->file)
> + show_lru_name(__entry->lru))
>  );
>  
>  TRACE_EVENT(mm_vmscan_writepage,
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 4f7c0d66d629..3f0774f30a42 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1500,7 +1500,7 @@ static unsigned long isolate_lru_pages(unsigned long 
> nr_to_scan,
>   }
>   *nr_scanned = scan + total_skipped;
>   trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan, 
> scan,
> - skipped, nr_taken, mode, is_file_lru(lru));
> + skipped, nr_taken, mode, lru);
>   update_lru_sizes(lruvec, lru, nr_zone_taken, nr_taken);
>   return nr_taken;
>  }
>