Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440280 > unrolled thread

Re: [PATCH 1/3] Add a new field to struct shrinker

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/3] Add a new field to struct shrinker Michal Hocko <mhocko@kernel.org> - 2016-07-11 08:40 +0200
    Re: [PATCH 1/3] Add a new field to struct shrinker Rik van Riel <riel@surriel.com> - 2016-07-11 16:40 +0200
      Re: [PATCH 1/3] Add a new field to struct shrinker Michal Hocko <mhocko@kernel.org> - 2016-07-11 16:40 +0200
        Re: [PATCH 1/3] Add a new field to struct shrinker Janani Ravichandran <janani.rvchndrn@gmail.com> - 2016-07-20 16:50 +0200
          Re: [PATCH 1/3] Add a new field to struct shrinker Michal Hocko <mhocko@kernel.org> - 2016-07-20 17:00 +0200
            Re: [PATCH 1/3] Add a new field to struct shrinker Tony Jones <tonyj@suse.de> - 2016-07-23 03:30 +0200
              Re: [PATCH 1/3] Add a new field to struct shrinker Tony Jones <tonyj@suse.de> - 2016-07-23 06:10 +0200
                Re: [PATCH 1/3] Add a new field to struct shrinker Rik van Riel <riel@surriel.com> - 2016-07-23 21:50 +0200
                  Re: [PATCH 1/3] Add a new field to struct shrinker Tony Jones <tonyj@suse.de> - 2016-07-24 01:30 +0200

#1440280 — Re: [PATCH 1/3] Add a new field to struct shrinker

On Sat 09-07-16 04:43:31, Janani Ravichandran wrote:
> Struct shrinker does not have a field to uniquely identify the shrinkers
> it represents. It would be helpful to have a new field to hold names of
> shrinkers. This information would be useful while analyzing their
> behavior using tracepoints.

This will however increase the vmlinux size even when no tracing is
enabled. Why cannot we simply print the name of the shrinker callbacks?

> 
> ---
>  include/linux/shrinker.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> index 4fcacd9..431125c 100644
> --- a/include/linux/shrinker.h
> +++ b/include/linux/shrinker.h
> @@ -52,6 +52,7 @@ struct shrinker {
>  	unsigned long (*scan_objects)(struct shrinker *,
>  				      struct shrink_control *sc);
>  
> +	const char *name;
>  	int seeks;	/* seeks to recreate an obj */
>  	long batch;	/* reclaim batch size, 0 = default */
>  	unsigned long flags;
> -- 
> 2.7.0
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1440660

[Multipart message — attachments visible in raw view] — view raw

On Mon, 2016-07-11 at 08:37 +0200, Michal Hocko wrote:
> On Sat 09-07-16 04:43:31, Janani Ravichandran wrote:
> > Struct shrinker does not have a field to uniquely identify the
> > shrinkers
> > it represents. It would be helpful to have a new field to hold
> > names of
> > shrinkers. This information would be useful while analyzing their
> > behavior using tracepoints.
> 
> This will however increase the vmlinux size even when no tracing is
> enabled. Why cannot we simply print the name of the shrinker
> callbacks?

What mechanism do you have in mind for obtaining the name,
Michal?

> > ---
> >  include/linux/shrinker.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
> > index 4fcacd9..431125c 100644
> > --- a/include/linux/shrinker.h
> > +++ b/include/linux/shrinker.h
> > @@ -52,6 +52,7 @@ struct shrinker {
> >  	unsigned long (*scan_objects)(struct shrinker *,
> >  				      struct shrink_control *sc);
> >  
> > +	const char *name;
> >  	int seeks;	/* seeks to recreate an obj */
> >  	long batch;	/* reclaim batch size, 0 = default */
> >  	unsigned long flags;
> > -- 
> > 2.7.0
> > 
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@kvack.org.  For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 
-- 

All Rights Reversed.

[toc] | [prev] | [next] | [standalone]

#1440663

On Mon 11-07-16 10:12:51, Rik van Riel wrote:
> On Mon, 2016-07-11 at 08:37 +0200, Michal Hocko wrote:
> > On Sat 09-07-16 04:43:31, Janani Ravichandran wrote:
> > > Struct shrinker does not have a field to uniquely identify the
> > > shrinkers
> > > it represents. It would be helpful to have a new field to hold
> > > names of
> > > shrinkers. This information would be useful while analyzing their
> > > behavior using tracepoints.
> > 
> > This will however increase the vmlinux size even when no tracing is
> > enabled. Why cannot we simply print the name of the shrinker
> > callbacks?
> 
> What mechanism do you have in mind for obtaining the name,
> Michal?

Not sure whether tracing infrastructure allows printk like %ps. If not
then it doesn't sound too hard to add.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1447268

> On Jul 11, 2016, at 8:03 PM, Michal Hocko <mhocko@kernel.org> wrote:
> 
> On Mon 11-07-16 10:12:51, Rik van Riel wrote:
>> 
>> What mechanism do you have in mind for obtaining the name,
>> Michal?
> 
> Not sure whether tracing infrastructure allows printk like %ps. If not
> then it doesn't sound too hard to add.

It does allow %ps. Currently what is being printed is the function symbol 
of the callback using %pF. I’d like to know why %pF is used instead of
%ps in this case.

Michal, just to make sure I understand you correctly, do you mean that we
could infer the names of the shrinkers by looking at the names of their callbacks?

Janani.

[toc] | [prev] | [next] | [standalone]

#1447275

On Wed 20-07-16 20:11:09, Janani Ravichandran wrote:
> 
> > On Jul 11, 2016, at 8:03 PM, Michal Hocko <mhocko@kernel.org> wrote:
> > 
> > On Mon 11-07-16 10:12:51, Rik van Riel wrote:
> >> 
> >> What mechanism do you have in mind for obtaining the name,
> >> Michal?
> > 
> > Not sure whether tracing infrastructure allows printk like %ps. If not
> > then it doesn't sound too hard to add.
> 
> It does allow %ps. Currently what is being printed is the function symbol 
> of the callback using %pF. I’d like to know why %pF is used instead of
> %ps in this case.

From a quick look into the code %pF should be doing the same thing as
%ps in the end. Some architectures just need some magic to get a proper
address of the function.

> Michal, just to make sure I understand you correctly, do you mean that we
> could infer the names of the shrinkers by looking at the names of their callbacks?

Yes, %ps can then be used for the name of the shrinker structure
(assuming it is available).

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1448859

On 07/20/2016 07:54 AM, Michal Hocko wrote:

>> Michal, just to make sure I understand you correctly, do you mean that we
>> could infer the names of the shrinkers by looking at the names of their callbacks?
> 
> Yes, %ps can then be used for the name of the shrinker structure
> (assuming it is available).

This is fine for emitting via the ftrace /sys interface,  but in order to have the data [name] get 
marshalled thru to perf (for example) you need to add it to the TP_fast_assign entry.

tony

[toc] | [prev] | [next] | [standalone]

#1448872

On 07/22/2016 06:27 PM, Tony Jones wrote:
> On 07/20/2016 07:54 AM, Michal Hocko wrote:
> 
>>> Michal, just to make sure I understand you correctly, do you mean that we
>>> could infer the names of the shrinkers by looking at the names of their callbacks?
>>
>> Yes, %ps can then be used for the name of the shrinker structure
>> (assuming it is available).
> 
> This is fine for emitting via the ftrace /sys interface,  but in order to have the data [name] get 
> marshalled thru to perf (for example) you need to add it to the TP_fast_assign entry.
> 
> tony

Unfortunately, %ps/%pF doesn't do much (re:  Michal's comment "assuming it is available"):

-       TP_printk("%pF %p: nid: %d objects to shrink %ld gfp_flags %s pgs_scanned %ld lru_pgs %ld cache items %ld delta %lld total_scan %ld",
+       TP_printk("%pF %p(%ps): nid: %d objects to shrink %ld gfp_flags %s pgs_scanned %ld lru_pgs %ld cache items %ld delta %lld total_scan %ld",
                __entry->shrink,
                __entry->shr,
+               __entry->shr,
                __entry->nid,
                __entry->nr_objects_to_shrink,

# cat trace_pipe
            bash-1917  [003] ...1  2925.941062: mm_shrink_slab_start: super_cache_scan+0x0/0x1a0 ffff88042bb60cc0(0xffff88042bb60cc0): nid: 0 objects to shrink 0 gfp_flags GFP_KERNEL pgs_scanned 1000 lru_pgs 1000 cache items 4 delta 7 total_scan 7


Otherwise what I was suggesting was something like this to ensure it was correctly marshaled for perf/etc:

diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
--- a/include/trace/events/vmscan.h
+++ b/include/trace/events/vmscan.h
@@ -16,6 +16,8 @@
 #define RECLAIM_WB_SYNC		0x0004u /* Unused, all reclaim async */
 #define RECLAIM_WB_ASYNC	0x0008u
 
+#define SHRINKER_NAME_LEN 	(size_t)32
+
 #define show_reclaim_flags(flags)				\
 	(flags) ? __print_flags(flags, "|",			\
 		{RECLAIM_WB_ANON,	"RECLAIM_WB_ANON"},	\
@@ -191,6 +193,7 @@ TRACE_EVENT(mm_shrink_slab_start,
 	TP_STRUCT__entry(
 		__field(struct shrinker *, shr)
 		__field(void *, shrink)
+		__array(char, name, SHRINKER_NAME_LEN);
 		__field(int, nid)
 		__field(long, nr_objects_to_shrink)
 		__field(gfp_t, gfp_flags)
@@ -202,6 +205,11 @@ TRACE_EVENT(mm_shrink_slab_start,
 	),
 
 	TP_fast_assign(
+		char sym[KSYM_SYMBOL_LEN];
+
+		sprint_symbol(sym, (unsigned long)shr);
+		strlcpy(__entry->name, sym, SHRINKER_NAME_LEN);
+
 		__entry->shr = shr;
 		__entry->shrink = shr->scan_objects;
 		__entry->nid = sc->nid;
@@ -214,9 +222,10 @@ TRACE_EVENT(mm_shrink_slab_start,
 		__entry->total_scan = total_scan;
 	),
 
-	TP_printk("%pF %p: nid: %d objects to shrink %ld gfp_flags %s pgs_scanned %ld lru_pgs %ld cache items %ld delta %lld total_scan %ld",
+	TP_printk("%pF %p(%s): nid: %d objects to shrink %ld gfp_flags %s pgs_scanned %ld lru_pgs %ld cache items %ld delta %lld total_scan %ld",
 		__entry->shrink,
 		__entry->shr,
+		__entry->name,
 		__entry->nid,
 		__entry->nr_objects_to_shrink,
 		show_gfp_flags(__entry->gfp_flags),
@@ -236,6 +245,7 @@ TRACE_EVENT(mm_shrink_slab_end,
 
 	TP_STRUCT__entry(
 		__field(struct shrinker *, shr)
+		__array(char, name, SHRINKER_NAME_LEN);
 		__field(int, nid)
 		__field(void *, shrink)
 		__field(long, unused_scan)
@@ -245,6 +255,11 @@ TRACE_EVENT(mm_shrink_slab_end,
 	),
 
 	TP_fast_assign(
+		char sym[KSYM_SYMBOL_LEN];
+
+		sprint_symbol(sym, (unsigned long)shr);
+		strlcpy(__entry->name, sym, SHRINKER_NAME_LEN);
+
 		__entry->shr = shr;
 		__entry->nid = nid;
 		__entry->shrink = shr->scan_objects;
@@ -254,9 +269,10 @@ TRACE_EVENT(mm_shrink_slab_end,
 		__entry->total_scan = total_scan;
 	),
 
-	TP_printk("%pF %p: nid: %d unused scan count %ld new scan count %ld total_scan %ld last shrinker return val %d",
+	TP_printk("%pF %p(%pF): nid: %d unused scan count %ld new scan count %ld total_scan %ld last shrinker return val %d",
 		__entry->shrink,
 		__entry->shr,
+		__entry->shr,
 		__entry->nid,
 		__entry->unused_scan,
 		__entry->new_scan,

[toc] | [prev] | [next] | [standalone]

#1448998

[Multipart message — attachments visible in raw view] — view raw

On Fri, 2016-07-22 at 21:05 -0700, Tony Jones wrote:
> On 07/22/2016 06:27 PM, Tony Jones wrote:
> > On 07/20/2016 07:54 AM, Michal Hocko wrote:
> > 
> > > > Michal, just to make sure I understand you correctly, do you
> > > > mean that we
> > > > could infer the names of the shrinkers by looking at the names
> > > > of their callbacks?
> > > 
> > > Yes, %ps can then be used for the name of the shrinker structure
> > > (assuming it is available).
> > 
> > This is fine for emitting via the ftrace /sys interface,  but in
> > order to have the data [name] get 
> > marshalled thru to perf (for example) you need to add it to the
> > TP_fast_assign entry.
> > 
> > tony
> 
> Unfortunately, %ps/%pF doesn't do much (re:  Michal's comment
> "assuming it is available"):
> 
> -       TP_printk("%pF %p: nid: %d objects to shrink %ld gfp_flags %s
> pgs_scanned %ld lru_pgs %ld cache items %ld delta %lld total_scan
> %ld",
> +       TP_printk("%pF %p(%ps): nid: %d objects to shrink %ld
> gfp_flags %s pgs_scanned %ld lru_pgs %ld cache items %ld delta %lld
> total_scan %ld",
>                 __entry->shrink,
>                 __entry->shr,
> +               __entry->shr,
>                 __entry->nid,
>                 __entry->nr_objects_to_shrink,
> 
> # cat trace_pipe
>             bash-1917  [003] ...1  2925.941062: mm_shrink_slab_start:
> super_cache_scan+0x0/0x1a0 ffff88042bb60cc0(0xffff88042bb60cc0): nid:
> 0 objects to shrink 0 gfp_flags GFP_KERNEL pgs_scanned 1000 lru_pgs
> 1000 cache items 4 delta 7 total_scan 7
> 
> 
> Otherwise what I was suggesting was something like this to ensure it
> was correctly marshaled for perf/etc:
> 
Janani,

it may make sense to have the code Tony posted be part of
your patch series. Just have both of your Signed-off-by:
lines on that patch.

-- 

All Rights Reversed.

[toc] | [prev] | [next] | [standalone]

#1449018

On 07/23/2016 12:43 PM, Rik van Riel wrote:

> Janani,

> it may make sense to have the code Tony posted be part of
> your patch series. Just have both of your Signed-off-by:
> lines on that patch.

Rik

Unfortunately the previous patch doesn't work on my system, which was the point I was trying to make. None of the shrinker symbols appear known so nothing usefully symbolic is displayed either with %ps 
or in the case of code I attached (calling sprint_symbol so it's visible to perf).   

Tony

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web