From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH V2 1/2] xen, libxc: Fine grained control of REP emulation optimizations Date: Tue, 22 Sep 2015 18:41:05 +0300 Message-ID: <56017691.7060301@bitdefender.com> References: <1442842294-6123-1-git-send-email-rcojocaru@bitdefender.com> <1442842294-6123-2-git-send-email-rcojocaru@bitdefender.com> <56018D1302000078000A4822@prv-mh.provo.novell.com> <5601739C.6010304@bitdefender.com> <5601926D02000078000A48DC@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5601926D02000078000A48DC@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: tamas@tklengyel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, stefano.stabellini@eu.citrix.co, stefano.stabellini@citrix.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 09/22/2015 06:39 PM, Jan Beulich wrote: >>>> On 22.09.15 at 17:28, wrote: >> On 09/22/2015 06:17 PM, Jan Beulich wrote: >>>>>> On 21.09.15 at 15:31, wrote: >>>> --- a/xen/arch/x86/hvm/emulate.c >>>> +++ b/xen/arch/x86/hvm/emulate.c >>>> @@ -514,7 +514,8 @@ static int hvmemul_virtual_to_linear( >>>> * being triggered for repeated writes to a whole page. >>>> */ >>>> *reps = min_t(unsigned long, *reps, >>>> - unlikely(current->domain->arch.mem_access_emulate_enabled) >>>> + unlikely(current->domain->arch.mem_access_emulate_enabled && >>>> + current->domain->arch.mem_access_emulate_each_rep) >>>> ? 1 : 4096); >>> >>> unlikely() should not wrap compound conditions, or else its effect of >>> eliminating mis-predicted branches from the fast path won't be >>> achieved. In the case here I wonder though whether you couldn't >>> simply test only ->arch.mem_access_emulate_each_rep. >> >> I'll unfold the unlikely(). >> >> Testing only ->arch.mem_access_emulate_each_rep is what I had done in >> the original patch, however on Andrew Cooper's suggestion I've now gated >> this on ->domain->arch.mem_access_emulate_enabled as well. >> >> Otherwise, somebody might set mem_access_emulate_each_rep via its >> xc_monitor_*() call, but then after calling xc_monitor_disable() it >> would still be in effect, even if the guest is no longer being monitored. >> >> If this is not a problem, I'm happy to check just >> ->arch.mem_access_emulate_each_rep. > > Or perhaps "disabled" should just clear that flag, also to not surprise > the next one to "enable"? Fair point, I'll do that. Thanks, Razvan