Re: [PATCH] KVM: x86: lapic: Fix the misuse of likely() in find_highest_vector()

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Aug 30, 2012 at 07:24:39PM +0900, Takuya Yoshikawa wrote:
> On Thu, 30 Aug 2012 13:10:33 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > OK, I'll do these on top of this patch.
> > 
> > Tweaking these 5 lines for readability across multiple
> > patches is just not worth it.
> > As long as we do random cleanups of this function it's probably easier
> > to just do them all in one patch.
> 
> OK.
> 
> > > > Something like the below pseudo code would do this I think?
> > > > 
> > > > #define APIC_VECTORS_PER_REG 32
> > > > 
> > > > 	int vec;
> > > > 	for (vec = MAX_APIC_VECTOR - APIC_VECTORS_PER_REG;
> > > > 	     vec -= APIC_VECTORS_PER_REG; vec >= 0) {
> > > > 		u32 *reg = bitmap + REG_POS(vec);
> > > 
> > > We want to introduce apic_read_register(bitmap, reg) instead.
> > > u32 reg = apic_read_register(bitmap, REG_POS(vec));
> > 
> > If Marcelo takes it, I don't mind :)
> > 
> > > > 		if (*reg)
> > > > 			return __fls(*reg) - 1 + vec;
> > > 
> > > Because it is not clear that this *reg is the same value
> > > tested before. 
> > 
> > Before - where? Looks like this is the only place where
> > *reg is used.
> 
>   if (*reg)   // BEFORE
>       return __fls(*reg) - 1 + vec;    // AFTER
> 
> Unless the value pointed to by a pointer cannot be updated
> concurrently, it seems a good practice to use a local variable
> explicitely in C level.

This last statement is very wrong.  If you are trying to address concurrent
access on smp, using a varible will never fix it. You need ACCESS_ONCE,
barriers and all that jazz.

If instead you are talking about readability, using a wrapper just to do
'+' looks like a bit of an overkill to me: you almost literally do
#define plus(a,b) (a+b).

> I know that this will not change anything actually, but many
> bitops functions do similar things.
> 
> Takuya

They do a bit more: they avoid duplication by calling
VEC_POS and REG_POS with the same paramater.
But let's not argue theoretically, send a patch and maintainers
will judge it.

> > > > 	}
> > > > 	return -1
> > > > 
> > > > count_vectors similar:
> > > > 
> > > >         for (vec = 0; vec < MAX_APIC_VECTOR; vec += APIC_VECTORS_PER_REG) {
> > > > 		u32 *reg = bitmap + REG_POS(vec);
> > > 
> > > Same here.
> > 
> > Same question :)