On Thu, 30 Aug 2012 09:37:02 +0300
"Michael S. Tsirkin" <mst@xxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
