On Thu, 30 Aug 2012 09:37:02 +0300 "Michael S. Tsirkin" <m...@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. > 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 (*reg) > return __fls(*reg) - 1 + vec; Because it is not clear that this *reg is the same value tested before. > } > 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. > count += hweight32(*reg); > } > > hmm? Looks very good! Thanks, Takuya -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html