From: Paul Burton <paulburton@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
Joel Fernandes <joelaf@google.com>,
stable@vger.kernel.org
Subject: Re: [PATCH 2/2] tracing: Resize tgid_map to PID_MAX_LIMIT, not PID_MAX_DEFAULT
Date: Wed, 30 Jun 2021 14:09:42 -0700 [thread overview]
Message-ID: <YNzdllg/634Sa6Rt@google.com> (raw)
In-Reply-To: <20210630083513.1658a6fb@oasis.local.home>
Hi Steven,
On Wed, Jun 30, 2021 at 08:35:13AM -0400, Steven Rostedt wrote:
> On Tue, 29 Jun 2021 17:34:06 -0700
> Paul Burton <paulburton@google.com> wrote:
>
> > On 64 bit systems this will increase the size of tgid_map from 256KiB to
> > 16MiB. Whilst this 64x increase in memory overhead sounds significant 64
> > bit systems are presumably best placed to accommodate it, and since
> > tgid_map is only allocated when the record-tgid option is actually used
> > presumably the user would rather it spends sufficient memory to actually
> > record the tgids they expect.
>
> NAK. Please see how I fixed this for the saved_cmdlines, and implement
> it the same way.
>
> 785e3c0a3a87 ("tracing: Map all PIDs to command lines")
>
> It's a cache, it doesn't need to save everything.
Well sure, but it's a cache that (modulo pid recycling) previously had a
100% hit rate for tasks observed in sched_switch events.
It differs from saved_cmdlines in a few key ways that led me to treat it
differently:
1) The cost of allocating map_pid_to_cmdline is paid by all users of
ftrace, whilst as I mentioned in my commit description the cost of
allocating tgid_map is only paid by those who actually enable the
record-tgid option.
2) We verify that the data in map_pid_to_cmdline is valid by
cross-referencing it against map_cmdline_to_pid before reporting it.
We don't currently have an equivalent for tgid_map, so we'd need to
add a second array or make tgid_map an array of struct { int pid; int
tgid; } to avoid reporting incorrect tgids. We therefore need to
double the memory we consume or further reduce the effectiveness of
this cache.
3) As mentioned before, with the default pid_max tgid_map/record-tgid
has a 100% hit rate which was never the case for saved_cmdlines. If
we go with a solution that changes this property then I certainly
think the docs need updating - the description of saved_tgids in
Documentation/trace/ftrace.rst makes no mention of this being
anything but a perfect recreation of pid->tgid relationships, and
unlike the description of saved_cmdlines it doesn't use the word
"cache" at all.
Having said that I think taking a similar approach to saved_cmdlines
would be better than what we have now, though I'm not sure whether it'll
be sufficient to actually be usable for me. My use case is grouping
threads into processes when displaying scheduling information, and
experience tells me that if any threads don't get grouped appropriately
the result will be questions.
Thanks,
Paul
next prev parent reply other threads:[~2021-06-30 21:09 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-30 0:34 [PATCH 1/2] tracing: Simplify & fix saved_tgids logic Paul Burton
2021-06-30 0:34 ` [PATCH 2/2] tracing: Resize tgid_map to PID_MAX_LIMIT, not PID_MAX_DEFAULT Paul Burton
2021-06-30 12:35 ` Steven Rostedt
2021-06-30 21:09 ` Paul Burton [this message]
2021-06-30 21:34 ` Steven Rostedt
2021-06-30 22:34 ` Joel Fernandes
2021-06-30 23:11 ` Steven Rostedt
2021-07-01 13:55 ` Steven Rostedt
2021-07-01 17:24 ` [PATCH v2 1/2] tracing: Simplify & fix saved_tgids logic Paul Burton
2021-07-01 17:24 ` [PATCH v2 2/2] tracing: Resize tgid_map to pid_max, not PID_MAX_DEFAULT Paul Burton
2021-07-01 18:12 ` Steven Rostedt
2021-07-01 18:15 ` Paul Burton
2021-07-01 18:27 ` Steven Rostedt
2021-07-01 18:07 ` [PATCH v2 1/2] tracing: Simplify & fix saved_tgids logic Joel Fernandes
2021-06-30 12:31 ` [PATCH " Steven Rostedt
2021-06-30 16:43 ` Joel Fernandes
2021-06-30 22:29 ` Joel Fernandes
2021-07-01 17:31 ` Paul Burton
2021-07-01 18:05 ` Joel Fernandes
2021-07-01 18:07 ` Steven Rostedt
2021-07-01 18:09 ` Joel Fernandes
2021-07-01 18:12 ` Paul Burton
2021-07-01 18:26 ` Steven Rostedt
2021-07-01 19:35 ` Joe Perches
2021-07-01 19:51 ` Steven Rostedt
2021-07-01 21:07 ` Joe Perches
2021-07-01 23:49 ` Joel Fernandes
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=YNzdllg/634Sa6Rt@google.com \
--to=paulburton@google.com \
--cc=joelaf@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=rostedt@goodmis.org \
--cc=stable@vger.kernel.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.