From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH v4 09/13] nEPT: Add nEPT violation/misconfigration support Date: Mon, 29 Jul 2013 19:24:50 +0300 Message-ID: <20130729162450.GB28372@redhat.com> References: <1374750001-28527-10-git-send-email-gleb@redhat.com> <51F62EF3.6060104@redhat.com> <20130729105245.GD18009@redhat.com> <51F64B2A.6020503@redhat.com> <20130729114323.GG18009@redhat.com> <51F65A98.2040002@redhat.com> <20130729123410.GI18009@redhat.com> <51F66A0B.20108@redhat.com> <20130729132035.GK18009@redhat.com> <51F67851.8070408@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, Xiao Guangrong , Jun Nakajima , Yang Zhang To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:49683 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754301Ab3G2QYy (ORCPT ); Mon, 29 Jul 2013 12:24:54 -0400 Content-Disposition: inline In-Reply-To: <51F67851.8070408@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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.