Message-ID: <20160728054947.GL12670@dastard>
Date:	Thu, 28 Jul 2016 15:49:47 +1000
From:	Dave Chinner <david@...morbit.com>
To:	Tony Jones <tonyj@...e.de>
Cc:	Michal Hocko <mhocko@...e.cz>,
	Janani Ravichandran <janani.rvchndrn@...il.com>,
	Rik van Riel <riel@...riel.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	hannes@...xchg.org, vdavydov@...tuozzo.com, vbabka@...e.cz,
	mgorman@...hsingularity.net, kirill.shutemov@...ux.intel.com,
	bywxiaobai@....com
Subject: Re: [PATCH 1/3] Add a new field to struct shrinker

[line wrap text at 72 columns, please] 

On Tue, Jul 26, 2016 at 09:40:57AM -0700, Tony Jones wrote:
> On 07/20/2016 07:54 AM, Michal Hocko wrote:
> >On Wed 20-07-16 20:11:09, Janani Ravichandran wrote:
> >>>On Jul 11, 2016, at 8:03 PM, Michal Hocko <mhocko@...nel.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).
> 
> The "shrinker structure" (struct shrinker) isn't a good candidate
> (as it's often embedded as thus no symbol name can be resolved)
> but the callback seems to work fine in my testing.
> 
> I made an earlier suggestion to Janani that it was helpful to have
> the superblock shrinker name constructed to include the fstype.
> This level of specificity would be lost if just the callback is
> used.  I talked briefly to Michal and his view is that more
> specific tracepoints can be added for this case.   This is
> certainly an option as the super_cache_scan callback can access
> the superblock and thus the file_system_type via containing
> record.   It's just more work to later reconcile the output of two
> tracepoints.
> 
> I talked briefly to Mel and we both think being able to have this
> level (of fstype) specificity would be useful and it would be lost
> just using the callback.   Another option which would avoid the
> static overhead of the names would be to add a new shrinker_name()
> callback.  If NULL,  the caller can just perform the default, in
> this case lookup the symbol for the callback, if !NULL it would
> provide additional string information which the caller could use.
> The per-sb shrinker could implement it and return the fstype.
> It's obviously still a +1 word growth of the struct shrinker but
> it avoids the text overhead of the constructed names.
> 
> Opinions?

Seems you're all missing the obvious.

Add a tracepoint for a shrinker callback that includes a "name"
field, have the shrinker callback fill it out appropriately. e.g
in the superblock shrinker:


	trace_shrinker_callback(shrinker, shrink_control, sb->s_type->name);

And generic code that doesn't want to put a specific context name in
there can simply call:

	trace_shrinker_callback(shrinker, shrink_control, __func__);

And now you know exactly what shrinker is being run.

No need to add names to any structures, it's call site defined so is
flexible, and if you're not using tracepoints has no overhead.

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists