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)