Messages in this thread | | | Subject | Re: [PATCH 3/3] KVM: x86/vPMU: Add lazy mechanism to release perf_event per vPMC | From | Like Xu <> | Date | Wed, 9 Oct 2019 16:07:13 +0800 |
| |
Hi Paolo, On 2019/10/9 15:15, Paolo Bonzini wrote: > On 09/10/19 05:14, Like Xu wrote: >>> >>> >>>> I'm not sure is this your personal preference or is there a technical >>>> reason such as this usage is not incompatible with union syntax? >>> >>> Apparently it 'works', so there is no hard technical reason, but >>> consider that _Bool is specified as an integer type large enough to >>> store the values 0 and 1, then consider it as a base type for a >>> bitfield. That's just disguisting. >> >> It's reasonable. Thanks. > > /me chimes in since this is KVM code after all... > > For stuff like hardware registers, bitfields are probably a bad idea > anyway, so let's only consider the case of space optimization. > > bool:2 would definitely cause an eyebrow raise, but I don't see why > bool:1 bitfields are a problem. An integer type large enough to store > the values 0 and 1 can be of any size bigger than one bit. > > bool bitfields preserve the magic behavior where something like this: > > foo->x = y; > > (x is a bool bitfield) would be compiled as > > foo->x = (y != 0); > > which can be a plus or a minus depending on the point of view. :) > Either way, bool bitfields are useful if you are using bitfields for > space optimization, especially if you have existing code using bool and > it might rely on the idiom above. > > However, in this patch bitfields are unnecessary and they result in > worse code from the compiler. There is plenty of padding in struct > kvm_pmu, with or without bitfields, so I'd go with "u8 event_count; bool > enable_cleanup;" (or better "need_cleanup").
Thanks. The "u8 event_count; bool need_cleanup;" looks good to me.
So is the lazy release mechanism looks reasonable to you ? If so, I may release the next version based on current feedback.
> > Thanks, > > Paolo >
| |