On Thu, Jul 26, 2012 at 07:08:19PM +0300, Avi Kivity wrote: > On 07/26/2012 06:07 PM, Eduardo Habkost wrote: > > On Thu, Jul 26, 2012 at 05:55:09PM +0300, Avi Kivity wrote: > >> On 07/26/2012 05:16 PM, Eduardo Habkost wrote: > >> >> > > >> >> > It's possible to replace the atomic read of CountCPUs with the bitmap > >> >> > weight > >> >> > calculation on the loop, but: is it really worth it? > >> >> > > >> >> Why not? This eliminates one more global state. > >> > > >> > Maybe we can simply make it stop being global and be used only by the > >> > smp.c initialization code? > >> > > >> > Even if the variable didn't exist yet, I think I would add it myself: > >> > it's simpler and more efficient to calculate the bitmap weight once, > >> > while filling the bitmap, than recalculating it every time on the > >> > while(cmos_smp_count) loop. > >> > >> So you're spinning more efficiently? > > > > No, coding more efficiently. I'm lazy. :-) > > > > I don't want to risk breaking that part of the code to save 2 bytes of > > memory. It's possible to impement it reliably, yes, but it's also very > > easy to introduce a subtle bug, so why touch something that works > > perfectly just to save 2 bytes? > > Like gleb, I prefer avoiding derived state which can get out-of-sync.
It is calculated only once on boot, and only used inside smp_probe() and nowhere else (after applying this series, I mean). Personally I am more afraid of subtle races between the bit-setting and the (non-atomic) bitmap weight calculation + loop, than getting CountCPUs out of sync while smp_probe() runs. Note that I am all for making it not a global variable anymore, and use it only inside smp_probe() and nowhere else (this series does that, the only thing missing is to remove it from util.h). If anybody wants to eliminate it from smp_probe() too, be my guest. :) (I am hoping this is just a suggestion of an additional improvement, not an issue that would block this patch from inclusion) -- Eduardo