[Lldb-commits] [PATCH] D125897: [trace][intelpt] Support system-wide tracing [9] - Collect and return context switch traces

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

Submitting my comments so far, will continue my review in a bit



================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:26-28
   static const char *kProcFsCpuInfo;
   static const char *kTraceBuffer;
+  static const char *kPerfContextSwitchTrace;
----------------
nit: these are just pointers to a buffer of bytes right? if so, maybe we could change the types to be `const uint8_t *`.
Obviously it doesn't make a difference since they are the same size, but when I see `const char *` my brain kinda immediately thinks STRING!
maybe it's just me though 🤩,  wdyt?


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:41-45
+  void ProcessDidStop();
+
+  /// To be invoked before the process will resume, so that we can capture the
+  /// first instructions after the resume.
+  void ProcessWillResume();
----------------
Why do we need to separate these out into two separate functions?


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:36
 
+static Expected<PerfEvent> CreateContextSwitchTracePerfEvent(
+    bool disabled, lldb::core_id_t core_id,
----------------
thoughts on moving this to be a "utility" method of the Perf.cpp and then call that utility method from here? Moving it there would make it more accessible to other callers.


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:47
+  attr.size = sizeof(attr);
+  attr.sample_period = 0;
+  attr.sample_type = PERF_SAMPLE_TID | PERF_SAMPLE_TIME;
----------------
what does this do?


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:51-54
+  attr.exclude_kernel = 1;
+  attr.sample_id_all = 1;
+  attr.exclude_hv = 1;
+  attr.disabled = disabled;
----------------
In addition to this context switching event needing to be part of the same group, you also must ensure that some other config bits (exclude_kernel, exclude_hv) are the same.
Also, what would happen if the disabled bit is set here but then the enabled bit of the intel pt event was set?

All of these considerations related to keeping the two events "in sync", are beginning to make me lean towards what I mentioned above about using the same perf event, because that would naturally remove any opportunities for the two events to be "out of sync"


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:56
+
+  // The given perf configuration will product context switch records of 32
+  // bytes each. Assuming that every context switch will be emitted twice (one
----------------



================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:101-109
+    Expected<IntelPTSingleBufferTrace> core_trace =
+        IntelPTSingleBufferTrace::Start(request, /*tid=*/None, core_id,
+                                        /*disabled=*/true);
+    if (!core_trace)
       return IncludePerfEventParanoidMessageInError(core_trace.takeError());
+
+    if (Expected<PerfEvent> context_switch_trace =
----------------
to reduce opportunity for things to get out of sync if this is ever touched in the future?


================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.h:97
+  llvm::DenseMap<lldb::core_id_t,
+                 std::pair<IntelPTSingleBufferTrace, ContextSwitchTrace>>
+      m_traces_per_core;
----------------
perhaps you could make this it's own structure? that would make things a bit less verbose here and in the ForEachCore callback?
also, this may make it clearer that they must be part of the same perf event and potentially would allow enforcing this easier?
Instead of creating an entirely new structure you also could make the ContextSwitchTrace an optional member of IntelPTSingleBuffer trace, but not sure if that's the best place to do the coupling.
Another option would be to just use the same underlying perf_event of IntelPTSingleBufferTrace. That would naturally enforce the constraint of the events being part of the same group and wouldn't require adding this new ContextSwitchTrace notion.

lots of options lol, I think things are fine as is but wanted to share my thoughts.
wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125897