On Wed, 30 Jun 2021 14:09:42 -0700 Paul Burton paulburton@google.com wrote:
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.
Obviously it wasn't 100% when it went over the PID_MAX_DEFAULT.
It differs from saved_cmdlines in a few key ways that led me to treat it differently:
- 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.
I'll admit that this is a valid point.
- 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.
Double 256K is just 512K which is still much less than 16M.
- 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.
I found a few bugs recently in the saved_cmdlines that were causing a much higher miss rate, but now it's been rather accurate. I wonder how much pain that's been causing you?
Anyway, I'll wait to hear what Joel says on this. If he thinks this is worth 16M of memory when enabled, I may take it.
-- Steve