[Lldb-commits] [PATCH] D120595: [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

Tue Mar 8 06:43:45 PST 2022

jj10306 marked 41 inline comments as done.
jj10306 added a comment.

Addressed comments locally, waiting to post update until the couple minor discussions with @wallace and @davidca in the comments are complete.

================
Comment at: lldb/docs/lldb-gdb-remote.txt:469
+//
+//    tsc_rate: {
+//      kind: "perf",
----------------
wallace wrote:
> davidca wrote:
> > why is all this called tsc rate? there is also offset in this conversion. What about something like tsc_conversion_params or tsc_conv
> tsc_conversion_params seems like a good idea
Agreed, making that change!

================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:44
+/// TSC to nanosecond conversion.
+class TimestampCounterRate {
+  public:
----------------
wallace wrote:
> davidca wrote:
> > nit, Intel documentation and kernel refers this as TSC, not TimestampCounter, in order to differentiate this awful name from everywhere else that "Timestamp" and "Counter" is used. Perhaps embrace TSC to make it easier to google info?
> Ahh!! Then let's do what David says. Let's call it tsc everywhere
Which do you think is a better name to replace `TimestampCounterRate` with - `TscConversionParams` (as mentioned in the previous comment, "tsc_conversion_params" will be the new key in the TraceGetState packet schema, so this would be consistent with that) or `TscConversioninfo` or `TscConverter` (since the function of the class is to provide functionality to convert Tsc to time)? 
@davidca @wallace wdyt?

================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:47
+    virtual ~TimestampCounterRate() = default;
+    /// Convert TSC value to nanoseconds. This represents the number of nanoseconds since
+    /// the TSC register's reset.
----------------
davidca wrote:
> wallace wrote:
> > 
> To be precise, it's number of nanoseconds since boot. You should also add that this nanoseconds clock is sched_clock https://www.kernel.org/doc/html/latest/timers/timekeeping.html
> 
> In fact, you may want to use sched_clock as the name for this clock, rather than Nanos as in the next lines.
Chapter 17.7 of [[ https://www.intel.com/content/www/us/en/architecture-and-technology/64-ia-32-architectures-software-developer-system-programming-manual-325384.html | Intel's software developer manual (volume 3) ]] states the following:
"The time-stamp counter (as implemented in the P6 family, Pentium, Pentium M, Pentium 4, Intel Xeon, Intel Core
Solo and Intel Core Duo processors and later processors) is a 64-bit counter that is **set to 0 following a RESET of
the processor**."

Does a reset only occur when the system is rebooted? I mentioned the reset in the documentation here to be very specific, but if reset is always the same as boot then I agree it makes sense to change it to "nanoseconds since boot". 

I'll add in the documentation that the nanoseconds being referred to here are from the the sched_clock and add the link you shared for reference.

================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:74
+struct TraceIntelPTGetStateResponse : TraceGetStateResponse {
+  /// `nullptr` if no tsc conversion rate exists.
+  llvm::Optional<TimestampCounterRateSP> tsc_rate;
----------------
labath wrote:
> wallace wrote:
> > avoid using nullptr when you have an Optional. 
> Ideally this would be a plain `TimestampCounterRateSP` variable and nullptr would denote emptyness. Since (shared) pointers already have a natural empty state, it's best to use that. Adding an extra Optional layer just creates ambiguity.
Agreed, will revert back to the plain SP.

================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:257

+  IntelPTManager::GetPerfTscRate(*m_mmap_meta);
+
----------------
wallace wrote:
> davidca wrote:
> > wallace wrote:
> > > sadly you can't put this here. If you do this, then GetState() will only work correctly if StartTrace() was invoked first. What if in the future we have a different flow in which lldb-server uses an additional tracing function besides StartTrace() and that new function forgets to set the tsc_rate?
> > > This is called coupling and should be avoided.
> > > 
> > > What you should do instead is to create a new static function called `llvm::Optional<TimestampCounterRateSP >GetTscRate()` that will perform the perf_event request or reuse a previously saved value. It should work regardless of when it's invoked.
> > 1) this is not a Get* method. This is populating a global. Get* are inexpensive methods that return a value.
> > 
> > 2) if the TSC conversion parameters will be stored in a global, why not to use the Singleton pattern?
> > 
> > 3) TSC parameters change over time. I really think that should be stored within IntelPTManager. Obtaining this parameters is a ~10 microseconds operation, so it's fine if they need to be read next time an IntelPTManager is created.
> Can tsc frequency rates change even in modern cpus?. In this case, it's no brainer we need to calculate this number all the time.
> 
> I also agree with point number 1. 
I agree that in its current state this isn't acting as a Get* function at all. However, after the comments @wallace left above, this function acts more like a Get* method as it returns the value of SP inside of the `g_tsc_rate` to be stored by `tsc_rate`

@davidca in point 3, are you suggesting that we move this field to be a member variable of IntelPTManager as opposed to a static variable because these conversion params change and thus should be fetched for each instance of the class?

================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:213
   }
+  m_tsc_rate = state->tsc_rate;
+
----------------
wallace wrote:
> make sure that everything works when lldb-server doesn't have support for the perf zero cap
Definitely, added that to the list of planned tests (in addition to malformed response JSON)

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120595/new/

https://reviews.llvm.org/D120595