Fix various remaining issues with EventDescription Rework, streamline interfaces, move configuration of Events in own EventConfig class. #358
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cvonelm
force-pushed
the
fix_perf_event
branch
3 times, most recently
from
bc45d53 to
2f1196c
Compare
Somehow, base_path does not get initialized correctly before read_proc is called, so move it all into read_proc
Also move use-case specific tracepoint code out of TracepointEvent
- Make Event clean base class. Use SimpleEvent when the event configuration needs to be specified manually. - Move almost all logic outside of the Event class. Event Configuration should happen fully from the outside - fix boolean operators - move sysfs specific code from Event to SysfsEvent
print [CPUS 0-0] as [CPU 0]
Designwise, the EventConfig class should handle all the ugly details of configuring the Events from the given Config(), such as setting specific bits. The sample::Reader for example asks the EventConfig for a sample event. The EventConfig then selects the correct sampling event using the EventProvider and the Config, configures all the options, such as sample_period and then gives it back to the sample::Reader. The sample::Reader does not need to change anything about the event. The EventProvider should only provide events, reading them from different sources, such as pre-defined, PFM, PMU events. The CounterProvider did not do anything special enough to warrant its continuing existence, merge it with EventConfig Smaller fixes - delete needless getter in CounterCollection - provide default values for bools in Config() - check exclude_kernel once in EventConfig() instead of for every event
cvonelm
force-pushed
the
fix_perf_event
branch
from
2f1196c to
2e5ef19
Compare
bmario
reviewed
Jan 27, 2025
include/lo2s/perf/event_config.hpp
Outdated
Comment on lines
55
to
58
| void watermark(Event& ev) | ||
| { | ||
| ev.set_watermark(0.8 * config().mmap_pages * sysconf(_SC_PAGESIZE)); | ||
| } |
There was a problem hiding this comment.
Why is this here? It seems to be not worth a function on its own. Especially because the name is not really telling.
There was a problem hiding this comment.
We use this calculation for the watermark at three different spots in the code and I dont want to spread the arcane knowledge that watermark is 80% of config().mmap_pages * page_size all through the code.
|
Also, I hate inheritance. Why is a SimpleEvent an Extension of an Event? How does one become simple by extending? |
{Tracepoint, Breakpoint, Simple}Event -> EventAttr
EventConfig -> EventComposer
EventProvider -> EventResolver
also, make it impossible for invalid EventAttrs to exist
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#Move Event Configuration to EventConfig class
Test for debugfs when specifying tracepoint dependent options,
Refactor Event class
Pass TracepointEvent directly instead of name of event
Add definition of base_path into read_proc()