linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mel Gorman <mgorman@techsingularity.net>
To: Dave Chinner <david@fromorbit.com>
Cc: Tony Jones <tonyj@suse.de>, Michal Hocko <mhocko@suse.cz>,
	Janani Ravichandran <janani.rvchndrn@gmail.com>,
	Rik van Riel <riel@surriel.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	akpm@linux-foundation.org, hannes@cmpxchg.org,
	vdavydov@virtuozzo.com, vbabka@suse.cz,
	kirill.shutemov@linux.intel.com, bywxiaobai@163.com
Subject: Re: [PATCH 1/3] Add a new field to struct shrinker
Date: Thu, 28 Jul 2016 11:25:13 +0100	[thread overview]
Message-ID: <20160728102513.GA2799@techsingularity.net> (raw)
In-Reply-To: <20160728054947.GL12670@dastard>

On Thu, Jul 28, 2016 at 03:49:47PM +1000, Dave Chinner wrote:
> 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);
> 

That misses capturing the latency of the call unless there is a begin/end
tracepoint. I was aware of the function graph tracer but I don't know how
to convince that to give the following information;

1. The length of time spent in a given function
2. The tracepoint information that might explain why the stall occurred

Take the compaction tracepoint for example

        trace_mm_compaction_begin(start_pfn, cc->migrate_pfn,
                                cc->free_pfn, end_pfn, sync);

	...

	trace_mm_compaction_end(start_pfn, cc->migrate_pfn,
                                cc->free_pfn, end_pfn, sync, ret);

The function graph tracer can say that X time is compact_zone() but it
cannot distinguish between a short time spent in that function because
compaction_suitable == false or compaction simply finished quickly.  While
the cc struct parameters could be extracted, end_pfn is much harder to figure
out because a user would have to parse zoneinfo to figure it out and even
*that* would only work if there are no overlapping nodes. Extracting sync
would require making assumptions about the implementation of compact_zone()
that could change.

> And now you know exactly what shrinker is being run.
> 

Sure and it's a good suggestion but does not say how long the shrinker
was running.

My understanding was the point of the tracepoints was to get detailed
information on points where the kernel is known to stall for long periods
of time. I don't actually know how to convince the function graph tracer
to get that type of information. Maybe it's possible and I just haven't
tried recently enough.

It potentially duration could be inferred from using a return probe on
the function but that requires that the function the tracepoint is running
is is known by the tool, has not been inlined and that there are no retry
loops that hit the begin tracepoint.

-- 
Mel Gorman
SUSE Labs

  reply	other threads:[~2016-07-28 10:25 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-07-09  8:33 [PATCH 0/3] Add names of shrinkers and have tracepoints display them Janani Ravichandran
2016-07-09  8:43 ` [PATCH 1/3] Add a new field to struct shrinker Janani Ravichandran
2016-07-11  6:37   ` Michal Hocko
2016-07-11 14:12     ` Rik van Riel
2016-07-11 14:33       ` Michal Hocko
2016-07-20 14:41         ` Janani Ravichandran
2016-07-20 14:54           ` Michal Hocko
2016-07-23  1:27             ` Tony Jones
2016-07-23  4:05               ` Tony Jones
2016-07-23 19:43                 ` Rik van Riel
2016-07-23 23:21                   ` Tony Jones
2016-07-26 16:40             ` Tony Jones
2016-07-28  5:49               ` Dave Chinner
2016-07-28 10:25                 ` Mel Gorman [this message]
2016-07-29  0:13                   ` Dave Chinner
2016-07-29 13:00                     ` Mel Gorman
2016-08-12  3:09                       ` Tony Jones
2016-07-09  8:52 ` [PATCH 2/3] Update name field for all shrinker instances Janani Ravichandran
2016-07-13  0:43   ` Tony Jones
2016-07-09  9:05 ` [PATCH 3/3] Add name fields in shrinker tracepoint definitions Janani Ravichandran
2016-07-09  9:45   ` kbuild test robot
2016-07-11 14:18   ` Vlastimil Babka
2016-07-13  0:35     ` Tony Jones
2016-07-13  6:16       ` Janani Ravichandran
2016-07-13 19:12         ` Tony Jones
2016-07-13 19:48           ` Rik van Riel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160728102513.GA2799@techsingularity.net \
    --to=mgorman@techsingularity.net \
    --cc=akpm@linux-foundation.org \
    --cc=bywxiaobai@163.com \
    --cc=david@fromorbit.com \
    --cc=hannes@cmpxchg.org \
    --cc=janani.rvchndrn@gmail.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.cz \
    --cc=riel@surriel.com \
    --cc=tonyj@suse.de \
    --cc=vbabka@suse.cz \
    --cc=vdavydov@virtuozzo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).