Re: [Xen-devel] [PATCH V2 1/2] xen, libxc: Fine grained control of REP emulation optimizations

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 09/22/2015 06:39 PM, Jan Beulich wrote:
>>>> On 22.09.15 at 17:28, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 09/22/2015 06:17 PM, Jan Beulich wrote:
>>>>>> On 21.09.15 at 15:31, <rcojocaru@xxxxxxxxxxxxxxx> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel