Posted Sep 24, 2013 23:54 UTC (Tue) by ncm (subscriber, #165)
Parent article: A perf ABI fix
Without tracing the kernel discussion thread, I don't know if the rich vein of humor in this event has been fully worked out, but I don't see how it ever can be.
The error was not to have put bitfields in a union, the error was to have put bitfields in at all. I gather that the C committee has considered deprecating bitfields so that any header using them will elicit warnings. In the meantime, we depend upon ridicule, ostracism, and the quirky mis-implementation of bitfields in every C compiler ever. Surely all suggestions to add even more bitfields were offered tongue-in-cheek? We can but hope.
As an abstract feature, bitfields are acknowledged to have an eldritch appeal, like ear tufts, 5-cm-thick toenails, or webbed fingers, but (fair warning!) anyone who speaks up for _using_ bitfields must prepare to be taunted.
Posted Sep 25, 2013 3:19 UTC (Wed) by deater (subscriber, #11746)
[Link]
The perf_event interface is full of bitfields for reasons I don't fully understand.
To make things more fun, there are proposals in the works to export the bit offsets in these bitfields (specifically the ones in struct perf_event_attr) via /sys so that the kernel can export event configs to the perf tool more "efficiently". I personally think this will only end in tears. Especially once endianess is factored in.
Good one
Posted Sep 25, 2013 3:41 UTC (Wed) by deater (subscriber, #11746)
[Link]
Also I should probably disclose that I'm the Vince Weaver who apparently has become famous for being grumpy about the perf_event ABI.
In this case I was grumpy because the initial Changelog for the structure re-arrangement did not mention anything at all about the ABI implications or the bit overlap.
It was only by luck that I noticed this issue, because I had updated the perf_event.h header in my perf_event_tests testsuite to 3.12-rc1 but had rebooted back to 3.11 for other reasons. If I hadn't done that it's likely no one would have noticed this issue until after the 3.12 release.
Not that it matters a lot though, as I'm possibly the only person in the world actually using RDPMC for anything right now. It's used by the High Performance Computing people for low-latency self monitoring, but the perf tool doesn't use the interface at all.
Good one
Posted Sep 25, 2013 9:19 UTC (Wed) by luto (subscriber, #39314)
[Link]
Are you using some library or doing this directly? I'd like to do the same thing, but the API seems to be (intentionally) poorly documented.
Good one
Posted Sep 25, 2013 13:39 UTC (Wed) by deater (subscriber, #11746)
[Link]
> Are you using some library or doing this directly? I'd like to do the same
> thing, but the API seems to be (intentionally) poorly documented.
I'm currently doing the RDPMC accesses directly. The eventual goal is to have the PAPI performance library use the interface; there are overhead issues with the interface I was dealing with first (sometimes it is slower to use RDPMC than to just use the read() syscall, for reasons that took me a long time to figure out. Thankfully there are workarounds).
In any case yes, the documentation is awful. I wrote the perf_event_open() manpage in an attempt to address this. I've been working on updating the RDPMC part of that recently, although had to spend time trying to sanely document this ABI issue instead.
Don't go by the example RDPMC code in perf_event.h, it's out of date and possibly never really worked. I've been meaning to send a patch to fix that.
Good one
Posted Sep 25, 2013 19:20 UTC (Wed) by mathstuf (subscriber, #69389)
[Link]
> Don't go by the example RDPMC code in perf_event.h, it's out of date and possibly never really worked. I've been meaning to send a patch to fix that.
Could a patch which replaces it with "TODO: Add an example" (or similar) be pushed for 3.12 at least? If there's anything worse than no documentation, it's bad documentation.
Good one
Posted Sep 25, 2013 12:31 UTC (Wed) by busterb (subscriber, #560)
[Link]
I used to think bitfields were neat, until I found out how badly the performed on an embedded MIPS.
The difference between dereferencing a bitfield and just doing a (flags & FLAG) test was generally a 20-30% speedup on inner loops in an ISA like MIPS due to its insistence on aligned memory accesses. Similar thing with the 'packed' GCC attribute.
Good one
Posted Sep 26, 2013 8:51 UTC (Thu) by etienne (subscriber, #25256)
[Link]
> bitfield ... insistence on aligned memory accesses
There isn't any relation in between bitfields and alignment, so it would probably be better to fix the compiler than fix few random source files, long term...
Good one
Posted Sep 26, 2013 9:30 UTC (Thu) by khim (subscriber, #9252)
[Link]
Wouldn't that break the ABI? Long-term this may be a good idea, but short-term it'll be quite a problem.
Good one
Posted Sep 26, 2013 11:05 UTC (Thu) by etienne (subscriber, #25256)
[Link]
> Wouldn't that break the ABI?
No, instead of reading the 2nd bit of unaligned byte, the compiler emits code to read bit (2+8) of aligned word. Bit stay at the same place.
Good one
Posted Sep 26, 2013 12:56 UTC (Thu) by khim (subscriber, #9252)
[Link]
This will only work for read, not for writes. And even then only if int and not _Bool is used.
Good one
Posted Sep 26, 2013 16:17 UTC (Thu) by deater (subscriber, #11746)
[Link]
One thing not really addressed is how bitfields run opposite ways on little endian and big endian systems.
Not a problem in most cases, but perf_event describes some bitfields
such as struct perf_branch_entry that get written to disk directly.
So if you record a session, then move it to an opposite-endian machine and try to read it back in you have problems.
Good one
Posted Sep 27, 2013 12:05 UTC (Fri) by etienne (subscriber, #25256)
[Link]
> opposite ways on little endian and big endian systems
On my side of the world, you do have little-endian system and BI-endian systems: the processor may be big-endian, but then it always has to interact with at least one little-endian subsystem (could be as simple as a PCI card, more usually most subsystems).
Then, they added stuff at the virtual memory layer to describe that memory mapped area as either little or big endian, which solves a small part of the problem, two bit fields still increment as 0b00, 0b10, 0b01, 0b11.
Then, big endian processor sort of disappeared.
I still prefer:
struct status {
#ifdef LITTLE_ENDIAN
unsigned b1:1, b2:1, b3:1, unused:29;
unsigned xx;
#else
unsigned xx;
unsigned unused:29, b3:1, b2:1, b1:1;
#endif
}
than the 40 equivalent lines of #define, if I have a lot of those status.
