stirling/bpf_tools/bcc_wrapper.h: Add wrapped BPF maps & arrays.
#1623
Conversation
src/stirling/bpf_tools/bcc_wrapper.h
Outdated
| absl::flat_hash_map<K, V> GetTableOffline(const bool clear_table = false) { | ||
| absl::flat_hash_map<K, V> r; | ||
|
|
||
| for (const auto& k : shadow_keys_) { | ||
| auto s = GetValue(k); | ||
| const auto v = s.ConsumeValueOrDie(); | ||
| r[k] = v; | ||
| if (clear_table) { | ||
| PX_UNUSED(underlying_->remove_value(k)); | ||
| } | ||
| } | ||
| if (clear_table) { | ||
| shadow_keys_.clear(); | ||
| } | ||
| return r; | ||
| } |
There was a problem hiding this comment.
Is there a reason this needs to take on reimplementing a function available on the underlying_ instance? I think it would be best to always defer to the underlying BCC function rather than including BCC implementation details in this shim.
There was a problem hiding this comment.
We updated the wrapped BCC map to use the shadow keys only if template parameter kUserSpaceManaged is set to true. It should be the case that iterating over the keys in user space is faster than through iterated syscalls.
|
|
||
| #ifdef __cplusplus | ||
| template <typename H> | ||
| friend H AbslHashValue(H h, const stack_trace_key_t& k) { | ||
| return H::combine(std::move(h), k.upid, k.user_stack_id, k.kernel_stack_id); | ||
| } | ||
|
|
||
| friend bool operator==(const stack_trace_key_t& lhs, const stack_trace_key_t& rhs) { | ||
| if (lhs.upid != rhs.upid) { | ||
| return false; | ||
| } | ||
| if (lhs.user_stack_id != rhs.user_stack_id) { | ||
| return false; | ||
| } | ||
| return lhs.kernel_stack_id == rhs.kernel_stack_id; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Why was this not needed before with the original map usage?
There was a problem hiding this comment.
The shadow keys in the wrapped bcc map force us to supply the hash impl.
There was a problem hiding this comment.
Thanks for clarifying 👍. Just wanted to make sure I didn't miss something important since I wasn't able to make that connection on my first pass.
src/stirling/source_connectors/perf_profiler/perf_profile_connector.cc
Outdated
Show resolved
Hide resolved
src/stirling/source_connectors/perf_profiler/perf_profile_connector.cc
Outdated
Show resolved
Hide resolved
| const uint32_t sample_count_idx = using_map_set_a ? kSampleCountAIdx : kSampleCountBIdx; | ||
|
|
||
| // Read out the perf buffer that contains the histogram for this iteration. | ||
| // TODO(jps): change PollPerfBuffer() to use std::chrono. | ||
| constexpr int kPollTimeoutMS = 0; | ||
| histo_perf_buf->poll(kPollTimeoutMS); | ||
| PollPerfBuffer(perfbuf_name, kPollTimeoutMS); |
There was a problem hiding this comment.
Can you explain why you chose to rely on bpf_ here rather than creating a WrappedBCCPerfBuffer data type? It feels like an inconsistent interface compared to how it worked previously and it's not clear to me why the perf buffer case is different.
There was a problem hiding this comment.
The main reason we find it this way is legacy. I did not introduce a wrapped perf buffer simply to follow our current setup. Wrapped perf buffer has crossed my mind, and so has moving all the wrapped bcc maps & arrays into bcc wrapper. There are some degrees of freedom here.
There was a problem hiding this comment.
Sounds good. I'm fine with the current implementation, but I wanted to make sure I understood the rationale 👍
src/stirling/source_connectors/socket_tracer/socket_trace_connector.cc
Outdated
Show resolved
Hide resolved
|
|
||
| // "r" our result. | ||
| std::vector<std::pair<K, V>> r; | ||
|
|
||
| // This is a user space managed map: we can iterate over the shadow keys. | ||
| for (const auto& k : shadow_keys_) { | ||
| auto s = GetValue(k); | ||
| const auto v = s.ConsumeValueOrDie(); | ||
| r.push_back({k, v}); | ||
| if (clear_table) { | ||
| PX_UNUSED(underlying_->remove_value(k)); | ||
| } | ||
| } | ||
| if (clear_table) { | ||
| shadow_keys_.clear(); | ||
| } | ||
| return r; | ||
| } |
There was a problem hiding this comment.
Did the previous user space map implementation even provide this implementation? It appears to me this is new API surface area and didn't exist before. Assuming that understanding is correct, my preference would be to only add what is used.
There was a problem hiding this comment.
I'd like to agree with the insight here, but it is tricky. For GetTableOffline (which is not used by any user space managed instantiation) I removed the new impl. and added static_assert(kUserSpaceManaged) before the pass through. However, this static assertion failed (and I'd like to disagree with the compiler insomuch as the offending instantiation was not user space managed, so perhaps it did not really need to compile this method). Here are some other options:
- Return a
StatusOrfrom this method. ECHECK(false)for user space managed.- More template magic to ensure that we don't compile this wrongly.
- Provide two wrapped maps: (1) the normal pass through wrapped map, and (2) The user space managed wrapped map.
- Leave "as is."
I will vote for (4) or (5) above.
There was a problem hiding this comment.
ah, I see. In that case 5 is good with me.
Signed-off-by: Pete Stevenson <[email protected]>
Signed-off-by: Pete Stevenson <[email protected]>
Signed-off-by: Pete Stevenson <[email protected]>
Signed-off-by: Pete Stevenson <[email protected]>
Signed-off-by: Pete Stevenson <[email protected]>
Signed-off-by: Pete Stevenson <[email protected]>
Signed-off-by: Pete Stevenson <[email protected]>
Signed-off-by: Pete Stevenson <[email protected]>
Signed-off-by: Pete Stevenson <[email protected]>
Signed-off-by: Pete Stevenson <[email protected]>
Signed-off-by: Pete Stevenson <[email protected]>
Signed-off-by: Pete Stevenson <[email protected]>
etep
force-pushed
the
rra-wrapped-bpf-maps-and-arrays
branch
from
aaabc51
to
5343bd9
Compare
Signed-off-by: Pete Stevenson <[email protected]>
Summary: We add wrappers for BPF maps and arrays to our BCC wrapper class. These wrappers anticipate our upcoming "record & replay" feature. In specific, they will be used to shim in recording logic and return replayed values, i.e. when either record or replay ("RR") is in use. When "RR" is not in use, they pass through to the underlying BPF map or array.
Relevant Issues: #1163
Type of change: /kind feature
Test Plan: Existing test coverage exercises all of these.