[Lldb-commits] [PATCH] D125897: [trace][intelpt] Support system-wide tracing [9] - Collect and return context switch traces
walter erquinigo via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Thu Jun 9 16:41:11 PDT 2022
wallace added inline comments.
================
Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:26-28
static const char *kProcFsCpuInfo;
static const char *kTraceBuffer;
+ static const char *kPerfContextSwitchTrace;
----------------
jj10306 wrote:
> 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?
oh no, these are just string enums, but it happens that c++ doesn't support natively string enums
================
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();
----------------
jj10306 wrote:
> Why do we need to separate these out into two separate functions?
The "state" thing is an implementation detail, the most important thing for callers is to notify the collector what's happening with the target.
So I'm also going with this approach for clarity, following the standard that LLDB uses everywhere. DidStop and WillResume appear so in the LLDB code often that it's better to keep them here for consistency.
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:36
+static Expected<PerfEvent> CreateContextSwitchTracePerfEvent(
+ bool disabled, lldb::core_id_t core_id,
----------------
jj10306 wrote:
> 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.
good idea
================
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;
----------------
jj10306 wrote:
> what does this do?
this is super redundant because this field is already 0 after the memset
================
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;
----------------
jj10306 wrote:
> 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"
I've moved this function to Perf.h, so given this:
- disabled is dominated by the group perf event state, so I'll use that state instead of asking for the disabled value.
- exclude_hv is needed all the time
- exclude_kernel as well
so, besides the change to disabled, the only two fields that we should really synchronize are exclude_hv and exclude_kernel, but I don't see us creating more perf events, so I don't think if it's worth creating more abstractions just for these two fields. We could do more in the future when we need to trace kernel. You can leave more suggestions in my newer diff
================
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
----------------
jj10306 wrote:
>
+1
================
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 =
----------------
jj10306 wrote:
> to reduce opportunity for things to get out of sync if this is ever touched in the future?
+1
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:122-129
void IntelPTMultiCoreTrace::ForEachCore(
std::function<void(core_id_t core_id, IntelPTSingleBufferTrace &core_trace)>
callback) {
for (auto &it : m_traces_per_core)
- callback(it.first, *it.second);
+ callback(it.first, it.second.first);
}
----------------
jj10306 wrote:
> Is there a way to consolidate these two methods?
not really :(
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.h:97
+ llvm::DenseMap<lldb::core_id_t,
+ std::pair<IntelPTSingleBufferTrace, ContextSwitchTrace>>
+ m_traces_per_core;
----------------
jj10306 wrote:
> 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?
This map is barely used, so I don't want to create more structs for this. The idea of having an optional context switch trace is not very nice to me because that makes the code less composable. In any case, I'm also not a far of my gigantic map, but I don't find it bad enough to change it.
Something I'm doing now is to use the alias ContextSwitchTrace more ubiquitously to make other parts of the code more readable.
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h:43
+ /// \param[in] disabled
+ /// Whether to start the tracing paused.
///
----------------
jj10306 wrote:
> This wording is a bit confusing - maybe provide some additional context here about this flag controlling the whether the perf event is enabled/disabled at perf_event_open time
+1
================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:161
if (Expected<resource_handle::MmapUP> mmap_metadata_data =
- DoMmap(nullptr, mmap_size, PROT_WRITE, MAP_SHARED, 0,
+ DoMmap(nullptr, mmap_size, PROT_READ | PROT_WRITE, MAP_SHARED, 0,
"metadata and data buffer")) {
----------------
jj10306 wrote:
> curious what adding the PROT_READ does here, given that this code was working with just PROT_WRITE it seems like PROT_WRITE implies PROT_READ
it was needed for context switches. I don't remember why though
================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:226
+Expected<std::vector<uint8_t>>
+PerfEvent::ReadFlushedOutAuxCyclicBuffer(size_t offset, size_t size) {
+ CollectionState previous_state = m_collection_state;
----------------
jj10306 wrote:
> It's worth noting that this function should only be called when the aux buffer is configured as a ring buffer (ie the aux buffer was mmaped with PROT_READ). if the aux buffer mmaped with PROT_WRITE then the behavior of the head is different and the consumer is required to update the aux_tail when reading.
> Perhaps we should check this before doing the reading or make it very clear in the docs that this should only be called on perf events with that specific configuration
i'll leave a comment for when we add a way to read the buffer in a non-cyclic manner
================
Comment at: lldb/source/Plugins/Process/Linux/Perf.h:101
class PerfEvent {
+ enum class CollectionState {
+ Enabled,
----------------
jj10306 wrote:
> is an enum needed for this? Since the perf event will only ever be in two states, feels like a bool would suffice
+1
================
Comment at: lldb/source/Plugins/Process/Linux/Perf.h:238
+ llvm::Expected<std::vector<uint8_t>>
+ ReadFlushedOutAuxCyclicBuffer(size_t offset, size_t size);
+
----------------
jj10306 wrote:
> nit: This is wordy. imo the name needs not mention that it's flushed out, just put that in the docs
I want to be super explicit, because this means that while we read, we are pausing the state of the event, so that's non-trivial side effect that should be clear by any caller.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125897/new/
https://reviews.llvm.org/D125897
More information about the lldb-commits
mailing list