Move a set of functions from pmcstat to libpmcstat allowing us to reuse them in other HWPMC-based applications.
This include symbols lookup, pmc logging, pmc attachment, process creation, ELF parsing, etc.
Details
Details
World build on amd64 host
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
libpmcstat.c must be split into per-function .c file. Also, I strongly suggest to mark the library as internal, since we cannot maintain PMC ABI even for a few weeks.
Comment Actions
- split libpmcstat
- add new function pmcstat_symbol_search_by_name() allowing to find mapped IP for given function name
| lib/libpmcstat/Makefile | ||
|---|---|---|
| 8 | Please put this one file per line, See e.g. the lib/libc/amd64/sys/Makefile.inc as an example. This style makes file additions much easier to read, and merges into branches are less likely to cause conflicts. | |
| lib/libpmcstat/libpmcstat.h | ||
| 39 | Why do you need this ? | |
| 42 | You need BEGIN_DECLS/END_DECLS braces around function declarations in the headers, to make this usable from C++. This is a usual reason to group all function declarations into single group. | |
| 220 | I strongly suggest to not add the min/max symbols into the header. | |
| usr.sbin/pmcstat/pmcstat.h | ||
| 36–37 | Do you still need _cpuset.h before libpmcstat.h ? It seems that you need it for real, then why did not you include the header from libpmcstat.h ? You tried to make libpmcstat.h self-contained ? | |
Comment Actions
- wrap lines
- use BEGIN/END_DECLS and group function declarations
- move min/max
- include _cpuset.h in libpmcstat.h
| lib/libpmcstat/Makefile | ||
|---|---|---|
| 8 | Agree. Thanks! | |
| lib/libpmcstat/libpmcstat.h | ||
| 39 | type WINDOW used in libpmcstat.h, so curses.h included to make it self-contained | |
| 42 | thanks for explanation, I added BEGIN/END_DECLS and grouped function declarations | |
| 220 | OK I moved this to .c files | |
| usr.sbin/pmcstat/pmcstat.h | ||
| 36–37 | type cpuset_t used by libpmstat.h, so I moved this to libpmcstat.h to make it self-contained. | |
| lib/libpmcstat/libpmcstat.h | ||
|---|---|---|
| 299 | Can you pass this parameter as void *, and cast in plugin(s) ? I want to remove curses.h from the header. | |
Comment Actions
remove dependency on curses.h for libpmcstat.h
| lib/libpmcstat/libpmcstat.h | ||
|---|---|---|
| 299 | Sure! | |
Comment Actions
This stuff would greatly benefit from more cleanup, but I think your have other priorities.
Comment Actions
This stuff would greatly benefit from more cleanup
You mean in the existing pmc implementation, not the libification, correct?
Comment Actions
It is moved around, so I am not sure what is your point.
Real place to start fixing is the user/kernel interface, to be able to have stable ABI for pmc events.
Comment Actions
It is moved around, so I am not sure what is your point.
Just trying to confirm my understanding of your comment - pmc needs cleanup both before and after this change. This change doesn't make it better or worse.
Comment Actions
The extraction of internal interfaces into a library certainly increases the amount of work for supposed cleanup. More, since I do not think that the extraction is done just for its own sake, but there will be a new code which uses the exposed interfaces, the overall amount of work is certainly larger.
The library definitely does not provide any consistent set of useful and designed interfaces, it is the internals exposed. This is one of the reason why I asked for it to be internal library, we certainly do not want to claim that even an API of the library is maintained, not to mention ABI (and I did not asked about providing symbol versioning for the library either). But I do not think that it is constructive to block other code because this mess is reused.