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

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jul 29, 2013 at 04:12:33PM +0200, Paolo Bonzini wrote:
> 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.
> 
Good point about about "|" always evaluating both. Is this the case
with if (simple !=0 | complex != 0) too where theoretically compiler may
see that if simple !=0 is true no need to evaluate the second one?
 

> 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.
I do not get what is_rsvd_bits_set_or_nonpresent() will check exactly
and why do we needed it, there are two places where we check
present/reserved and in one of them we need to know which one it is.

Anyway order of checks in prefetch_invalid_gpte() is not relevant to
that patchset, so lets better leave it to a separate discussion.

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

--
			Gleb.