Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 30 Jun 2021 14:09:42 -0700
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
Message-ID: <YNzdllg/634Sa6Rt@google.com>
References: <20210630003406.4013668-1-paulburton@google.com>
 <20210630003406.4013668-2-paulburton@google.com>
 <20210630083513.1658a6fb@oasis.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20210630083513.1658a6fb@oasis.local.home>
Precedence: bulk

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