On Thu, 30 Aug 2012 01:51:20 +0300
"Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:
> This text:
> + if (likely(!word_offset && !word[0]))
> + return -1;
> is a left-over from the original implementation.
>
> There we did a ton of gratitious calls to interrupt
> injection so it was important to streamline that path:
> lookups in emoty ISR and IRR.
>
> This is less common nowdays, since we have kvm_make_request,
> additionally, 8680b94b0e6046af2644c17313287ec0cb5843dc
> means for ISR lookups we do not need to scan
> the vector at all, and
> 33e4c68656a2e461b296ce714ec322978de85412
> means we never need to scan IRR if it is empty.
>
> So I think likely() here really became bogus by now.
Yes, thank you for the explanation.
> But optimizing the vector lookups is a wrong thing to do
> I suspect: these basically should almost never trigger.
This patch is not optimizing things at all, just removing
unnatural logic which might be justified only for using that
bogus likely().
I explain this below.
> Besides a single possible case: a single bit in IRR might
> still be somewhat common I think.
Then, the current code is doing very bad thing:
while ((word_offset != 0) && (word[(--word_offset) << 2] == 0))
continue;
if (likely(!word_offset && !word[0]))
return -1;
else
return fls(word[word_offset << 2]) - 1 + (word_offset << 5);
- checking word[0] separately does not make sense
- using fls(), not __fls(), means we doubly check (word[i] == 0)
Actually, how can this likely() work so effectively even when we
return -1? If we do (word[0] == 0) check in the same loop, CPU
should naturally predict the result:
for (word_offset = (MAX_APIC_VECTOR >> 5) - 1;
word_offset >= 0; --word_offset) {
word = p[word_offset << 2];
if (word)
return __fls(word) + (word_offset << 5);
}
return -1;
> If we want to optimize the case of a single bit set in IRR.
> my guess is the best thing is to replace irr_pending with
> generalization of isr_count/highest_isr_cache so we can have
> a highest IRR cache. This will avoid scans completely.
Yes, but let's first fix the wrong code.
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
