[PATCH] mm: vmscan: show zone type in kswapd tracepoints

[PATCH] mm: vmscan: show zone type in kswapd tracepoints

[Yafang Shao]

If we want to know the zone type, we have to check whether
CONFIG_ZONE_DMA, CONFIG_ZONE_DMA32 and CONFIG_HIGHMEM are set or not,
that's not so convenient.

We'd better show the zone type directly.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/trace/events/vmscan.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
index a1cb913..4c8880b 100644
--- a/include/trace/events/vmscan.h
+++ b/include/trace/events/vmscan.h
@@ -73,7 +73,10 @@
 		__entry->order	= order;
 	),
 
-	TP_printk("nid=%d zid=%d order=%d", __entry->nid, __entry->zid, __entry->order)
+	TP_printk("nid=%d zid=%-8s order=%d",
+		__entry->nid,
+		__print_symbolic(__entry->zid, ZONE_TYPE),
+		__entry->order)
 );
 
 TRACE_EVENT(mm_vmscan_wakeup_kswapd,
@@ -96,9 +99,9 @@
 		__entry->gfp_flags	= gfp_flags;
 	),
 
-	TP_printk("nid=%d zid=%d order=%d gfp_flags=%s",
+	TP_printk("nid=%d zid=%-8s order=%d gfp_flags=%s",
 		__entry->nid,
-		__entry->zid,
+		__print_symbolic(__entry->zid, ZONE_TYPE),
 		__entry->order,
 		show_gfp_flags(__entry->gfp_flags))
 );
-- 
1.8.3.1

Re: [PATCH] mm: vmscan: show zone type in kswapd tracepoints

[Michal Hocko]

On Fri 01-03-19 15:38:54, Yafang Shao wrote:
> If we want to know the zone type, we have to check whether
> CONFIG_ZONE_DMA, CONFIG_ZONE_DMA32 and CONFIG_HIGHMEM are set or not,
> that's not so convenient.
> 
> We'd better show the zone type directly.

I do agree that zone number is quite PITA to process in general but do
we really need this information in the first place? Why do we even care?

Zones are an MM internal implementation details and the more we export
to the userspace the more we are going to argue about breaking userspace
when touching them. So I would rather not export that information unless
it is terribly useful.

> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/trace/events/vmscan.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> index a1cb913..4c8880b 100644
> --- a/include/trace/events/vmscan.h
> +++ b/include/trace/events/vmscan.h
> @@ -73,7 +73,10 @@
>  		__entry->order	= order;
>  	),
>  
> -	TP_printk("nid=%d zid=%d order=%d", __entry->nid, __entry->zid, __entry->order)
> +	TP_printk("nid=%d zid=%-8s order=%d",
> +		__entry->nid,
> +		__print_symbolic(__entry->zid, ZONE_TYPE),
> +		__entry->order)
>  );
>  
>  TRACE_EVENT(mm_vmscan_wakeup_kswapd,
> @@ -96,9 +99,9 @@
>  		__entry->gfp_flags	= gfp_flags;
>  	),
>  
> -	TP_printk("nid=%d zid=%d order=%d gfp_flags=%s",
> +	TP_printk("nid=%d zid=%-8s order=%d gfp_flags=%s",
>  		__entry->nid,
> -		__entry->zid,
> +		__print_symbolic(__entry->zid, ZONE_TYPE),
>  		__entry->order,
>  		show_gfp_flags(__entry->gfp_flags))
>  );
> -- 
> 1.8.3.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: vmscan: show zone type in kswapd tracepoints

[Yafang Shao]

On Mon, Mar 11, 2019 at 4:47 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Fri 01-03-19 15:38:54, Yafang Shao wrote:
> > If we want to know the zone type, we have to check whether
> > CONFIG_ZONE_DMA, CONFIG_ZONE_DMA32 and CONFIG_HIGHMEM are set or not,
> > that's not so convenient.
> >
> > We'd better show the zone type directly.
>
> I do agree that zone number is quite PITA to process in general but do
> we really need this information in the first place? Why do we even care?
>

Sometimes we want to know this event occurs in which zone, then we can
get the information of this zone,
for example via /proc/zoneinfo.
It could give us more information for debugging.


> Zones are an MM internal implementation details and the more we export
> to the userspace the more we are going to argue about breaking userspace
> when touching them. So I would rather not export that information unless
> it is terribly useful.
>

I 'm not sure whether zone type is  terribly useful or not, but the
'zid' is useless at all.

I don't agree that Zones are MM internal.
We can get the zone type in many ways, for example /proc/zoneinfo.

If we show this event occurs in which zone, we'd better show the zone type,
or we should drop this 'zid'.


> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  include/trace/events/vmscan.h | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> > index a1cb913..4c8880b 100644
> > --- a/include/trace/events/vmscan.h
> > +++ b/include/trace/events/vmscan.h
> > @@ -73,7 +73,10 @@
> >               __entry->order  = order;
> >       ),
> >
> > -     TP_printk("nid=%d zid=%d order=%d", __entry->nid, __entry->zid, __entry->order)
> > +     TP_printk("nid=%d zid=%-8s order=%d",
> > +             __entry->nid,
> > +             __print_symbolic(__entry->zid, ZONE_TYPE),
> > +             __entry->order)
> >  );
> >
> >  TRACE_EVENT(mm_vmscan_wakeup_kswapd,
> > @@ -96,9 +99,9 @@
> >               __entry->gfp_flags      = gfp_flags;
> >       ),
> >
> > -     TP_printk("nid=%d zid=%d order=%d gfp_flags=%s",
> > +     TP_printk("nid=%d zid=%-8s order=%d gfp_flags=%s",
> >               __entry->nid,
> > -             __entry->zid,
> > +             __print_symbolic(__entry->zid, ZONE_TYPE),
> >               __entry->order,
> >               show_gfp_flags(__entry->gfp_flags))
> >  );
> > --
> > 1.8.3.1
> >
>
> --
> Michal Hocko
> SUSE Labs

Re: [PATCH] mm: vmscan: show zone type in kswapd tracepoints

[Michal Hocko]

On Tue 12-03-19 19:04:43, Yafang Shao wrote:
> On Mon, Mar 11, 2019 at 4:47 PM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Fri 01-03-19 15:38:54, Yafang Shao wrote:
> > > If we want to know the zone type, we have to check whether
> > > CONFIG_ZONE_DMA, CONFIG_ZONE_DMA32 and CONFIG_HIGHMEM are set or not,
> > > that's not so convenient.
> > >
> > > We'd better show the zone type directly.
> >
> > I do agree that zone number is quite PITA to process in general but do
> > we really need this information in the first place? Why do we even care?
> >
> 
> Sometimes we want to know this event occurs in which zone, then we can
> get the information of this zone,
> for example via /proc/zoneinfo.
> It could give us more information for debugging.

Could you be more specific please?

> > Zones are an MM internal implementation details and the more we export
> > to the userspace the more we are going to argue about breaking userspace
> > when touching them. So I would rather not export that information unless
> > it is terribly useful.
> >
> 
> I 'm not sure whether zone type is  terribly useful or not, but the
> 'zid' is useless at all.
> 
> I don't agree that Zones are MM internal.
> We can get the zone type in many ways, for example /proc/zoneinfo.
> 
> If we show this event occurs in which zone, we'd better show the zone type,
> or we should drop this 'zid'.

Yes, I am suggesting the later. If somebody really needs it then I would
like to see a _specific_ usecase. Then we can add the proper name.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: vmscan: show zone type in kswapd tracepoints

[Yafang Shao]

On Tue, Mar 12, 2019 at 9:38 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Tue 12-03-19 19:04:43, Yafang Shao wrote:
> > On Mon, Mar 11, 2019 at 4:47 PM Michal Hocko <mhocko@kernel.org> wrote:
> > >
> > > On Fri 01-03-19 15:38:54, Yafang Shao wrote:
> > > > If we want to know the zone type, we have to check whether
> > > > CONFIG_ZONE_DMA, CONFIG_ZONE_DMA32 and CONFIG_HIGHMEM are set or not,
> > > > that's not so convenient.
> > > >
> > > > We'd better show the zone type directly.
> > >
> > > I do agree that zone number is quite PITA to process in general but do
> > > we really need this information in the first place? Why do we even care?
> > >
> >
> > Sometimes we want to know this event occurs in which zone, then we can
> > get the information of this zone,
> > for example via /proc/zoneinfo.
> > It could give us more information for debugging.
>
> Could you be more specific please?
>

Honestly speaking,  this one hasn't help us fix the real issue yet.

> > > Zones are an MM internal implementation details and the more we export
> > > to the userspace the more we are going to argue about breaking userspace
> > > when touching them. So I would rather not export that information unless
> > > it is terribly useful.
> > >
> >
> > I 'm not sure whether zone type is  terribly useful or not, but the
> > 'zid' is useless at all.
> >
> > I don't agree that Zones are MM internal.
> > We can get the zone type in many ways, for example /proc/zoneinfo.
> >
> > If we show this event occurs in which zone, we'd better show the zone type,
> > or we should drop this 'zid'.
>
> Yes, I am suggesting the later. If somebody really needs it then I would
> like to see a _specific_ usecase. Then we can add the proper name.

This 'zid' always seems like a noise currently.
I will send a patch to drop this one.

Thanks
Yafang