Re: [PATCH v4 09/13] nEPT: Add nEPT violation/misconfigration support

[Paolo Bonzini <pbonzini@redhat.com>]

Il 29/07/2013 15:20, Gleb Natapov ha scritto:
>> 2) in cases like this you just do not use likely/unlikely; the branch
>> will be very unlikely in the beginning, and very likely once shadow
>> pages are filled or in the no-EPT case.  Just let the branch predictor
>> adjust, it will probably do better than boolean tricks.
>>
> likely/unlikely are usually useless anyway. If you can avoid if()
> altogether this is a win since there is no branch to predict.

However, if the branches are dynamically well-predicted,

   if (simple)
       ...
   if (complex)
       ...

is likely faster than

   if (simple | complex)

because the branches then are very very cheap, and it pays off to not
always evaluate the complex branch.

In this case, the reserved bit test is the relatively complex one, it
has a couple memory accesses and a longish chain of dependencies.

>>>> Especially if you change prefetch_invalid_gpte to do the reserved bits
>>>> test after the present test (so that is_rsvd_bits_set is only called on
>>>> present pagetables), is_rsvd_bits_set's result should be really
>>>> well-predicted. 
>>> Nope, for ept page tables present is not a single bit, it is three bits
>>> which by themselves can have invalid values.
>>
>> We're not checking the validity of the bits in the is_present_gpte test,
>> we're checking it in the is_rsvd_bits_set test (is_present_gpte is doing
>> just "(pte & 7) != 0").  It doesn't change anything in the outcome of
>> prefetch_invalid_gpte, and it makes the ordering consistent with
>> walk_addr_generic which already tests presence before reserved bits.
>>
>> So doing this swap should be a win anyway.
>>
>>>>                   At this point (and especially since function invocation
>>>> is always in "if"s), using boolean logic to avoid branches does not make
>>>> much sense anymore for this function.
>>>
>>> That's true.
>>
>> So are you going to change to "if"s?
>>
> I think it will be better just to check mmu->bad_mt_xwr always. (I
> dislike ifdefs if you haven't noticed :)).

Yeah, I also thought of always checking bad_mt_xwr and even using it to
subsume the present check too, i.e. turning it into
is_rsvd_bits_set_or_nonpresent.  It checks the same bits that are used
in the present check (well, a superset).  You can then check for
presence separately if you care, which you don't in
prefetch_invalid_gpte.  It requires small changes in the callers but
nothing major.

But it still seems to me that we're in the above "if (simple ||
complex)" case and having a separate "if (!present)" check will be faster.

Paolo