Re: [Xen-devel] [PATCH] Don't track all memory when enabling log dirty to track vram

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

George Dunlap wrote on 2014-02-18:
> On 02/18/2014 03:14 AM, Zhang, Yang Z wrote:
>> Jan Beulich wrote on 2014-02-17:
>>>>>> On 17.02.14 at 15:51, George Dunlap
>>>>>> <george.dunlap@xxxxxxxxxxxxx>
>>> wrote:
>>>> On 02/17/2014 10:18 AM, Jan Beulich wrote:
>>>>> Actually I'm afraid there are two problems with this patch:
>>>>> 
>>>>> For one, is enabling "global" log dirty mode still going to work
>>>>> after VRAM-only mode already got enabled? I ask because the
>>>>> paging_mode_log_dirty() check which paging_log_dirty_enable()
>>>>> does first thing suggests otherwise to me (i.e. the now
>>>>> conditional setting of all p2m entries to p2m_ram_logdirty would
>>>>> seem to never get executed). IOW I would think that we're now
>>>>> lacking a control operation allowing the transition from dirty
>>>>> VRAM tracking mode to full log dirty mode.
>>>> Hrm, will so far playing with this I've been unable to get a
>>>> localhost migrate to fail with the vncviewer attached.  Which
>>>> seems a bit
> strange...
>>> Not necessarily - it may depend on how the tools actually do this:
>>> They might temporarily disable log dirty mode altogether, just to
>>> re-enable full mode again right away. But this specific usage of
>>> the hypervisor interface wouldn't (to me) mean that other tool
>>> stacks might not be doing this differently.
>> You are right. Before migration, libxc will disable log dirty mode
>> if it already
> enabled before and re-enable it. So when I am developing this patch, I
> think it is ok for migration.
>> 
>> If there really have other tool stacks also will use this interface
>> (Is it true?),
> perhaps my original patch is better which will check
> paging_mode_log_dirty(d) && log_global:
> 
> It turns out that the reason I couldn't get a crash was because libxc
> was actually paying attention to the -EINVAL return value, and
> disabling and then re-enabling logdirty.  That's what would happen
> before your dirty vram patch, and that's what happens after.  And
> arguably, that's the correct behavior for any toolstack, given that the 
> interface returns an error.

Agree.

> 
> This patch would actually change the interface; if we check this in,
> then if you enable logdirty when dirty vram tracking is enabled, you
> *won't* get an error, and thus *won't* disable and re-enable logdirty mode.
> So actually, this patch would be more disruptive.
> 

Jan, do you have any comment? 

> Thanks for the patch, though. :-)
> 
>   -George
>> 
>> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
>> index
>> ab5eacb..368c975 100644
>> --- a/xen/arch/x86/mm/paging.c
>> +++ b/xen/arch/x86/mm/paging.c
>> @@ -168,7 +168,7 @@ int paging_log_dirty_enable(struct domain *d,
> bool_t log_global)
>>   {
>>       int ret;
>> -    if ( paging_mode_log_dirty(d) )
>> +    if ( paging_mode_log_dirty(d) && !log_global )
>>           return -EINVAL;
>>       domain_pause(d);
>> 
>>> Jan
>> 
>> Best regards,
>> Yang
>> 
>>


Best regards,
Yang



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