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

[David Carrillo Cisneros via Phabricator via lldb-commits]

davidca added inline comments.
Herald added a project: All.


================
Comment at: lldb/docs/lldb-gdb-remote.txt:469
+//
+//    tsc_rate: {
+//      kind: "perf",
----------------
why is all this called tsc rate? there is also offset in this conversion. What about something like tsc_conversion_params or tsc_conv


================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:44
+/// TSC to nanosecond conversion.
+class TimestampCounterRate {
+  public:
----------------
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?


================
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.
----------------
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.


================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:78-80
+bool fromJSON(const llvm::json::Value &value, TimestampCounterRateSP &tsc_converter, llvm::json::Path path);
+
+bool fromJSON(const llvm::json::Value &value, TraceIntelPTGetStateResponse &packet, llvm::json::Path path);
----------------
should this two be static factory methods?


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.cpp:257
 
+  IntelPTManager::GetPerfTscRate(*m_mmap_meta);
+
----------------
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.


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTManager.h:211
   static bool IsSupported();
+  static void GetPerfTscRate(perf_event_mmap_page &mmap_meta);
+
----------------
wallace wrote:
> don't forget to add documentation here
how is ti


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

https://reviews.llvm.org/D120595