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 06:50:52PM +0900, Takuya Yoshikawa wrote:
> On Thu, 30 Aug 2012 09:37:02 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > After staring at your code for a while it does appear to
> > do the right thing, and looks cleaner than what
> > we have now. commit log could be clearer.
> > It should state something like:
> > 
> > 
> > Clean up code in find_highest_vector:
> >  - likely() is there for historical reasons, it is no longer
> >    clear it's optimizing for the correct branch,
> >    and find_highest_vector might not be worth optimizing at all
> >  - checking word[0] separately does not make sense:
> >    if (word[word_offset << 2]) would be clearer
> >  - since we test word[...] != 0 beforehand, we can use __fls
> >    instead of fls()
> >  - for loop iterating over an array is clearer than while
> 
> Yes, I'll update the patch.
> 
> > Since you are going for cleanups, maybe we could also add:
> > - get rid of ugly >> 5 << 2, switch to using REG_POS instead?
> 
> 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.

> > 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.

> > 	}
> > 	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 :)

> >                 count += hweight32(*reg);
> > 	}
> > 
> > hmm?
> 
> Looks very good!
> 
> Thanks,
> 	Takuya