Re: [Xen-devel] [PATCH RFC 1/4] x86/mm: Shadow and p2m changes for PV mem_access

["Aravindh Puthiyaparambil (aravindp)" <aravindp@xxxxxxxxx>]

>> >>>mfn_t gmfn)
>> >>>>          if ( !(shadow_mode_external(v->domain)
>> >>>>                 && (page->count_info & PGC_count_mask) <= 3
>> >>>>                 && ((page->u.inuse.type_info & PGT_count_mask)
>> >>>> -                   == !!is_xen_heap_page(page))) )
>> >>>> +                       == !!is_xen_heap_page(page)))
>> >>>> +                    && !(shadow_mode_mem_access(v->domain)) )
>> >>>
>> >>>You're breaking indentation, there are pointless parentheses again,
>> >>>but most importantly - why?
>> >>
>> >> Sorry, I meant to ask a question about this in my patch message and
>> >> mark this as a workaround. I am seeing the message on all
>> >> sh_remove_all_mappings() and I was not able to figure out why this
>> >> was happening. I just added this as a work around. I was hoping you
>> >> or Tim
>> >would shed more light on this.
>> >
>> >I'm afraid that without detail on which pages the triggers on, and
>> >you at least having spent some time finding out where the stray/extra
>> >references may be coming from it's going to be hard to help.
>>
>> Here are some of the prints I saw. I typically saw it for every set_entry() 
>> call.
>>
>> (XEN) sh error: sh_remove_all_mappings(): can't find all mappings of
>> mfn 33960: c=8000000000000002 t=7400000000000000 <most common>
>>  (XEN) sh error: sh_remove_all_mappings(): can't find all mappings of
>> mfn 134b8: c=8000000000000038 t=7400000000000001
>
>This is because you are not in shadow_mode_refcounts() (i.e. the page's
>refcount and typecount are based on the _guest_ pagetables rather than the
>_shadow_ ones).  That means that sh_remove_all_mappings() can't use the
>refcounts to figure out when it's removed the last shadow mapping.

I take it that I won't be able to run with shadow_mode_refcounts() for PV 
domains.

>That also means that sh_remove_all_mappings() will have had to search every
>sl1e in the system, which is expensive.  If there will be a lot of these 
>updates,
>you might consider batching them and using
>shadow_blow_tables() after each batch instead.

Does this mean batching them in the hypervisor or in the mem_access listener? 
Typically one would want the effect of the new permissions to kick in 
immediately. So I am afraid batching them will delay this. Anyhow, at a minimum 
I will add a comment here as to why I am adding the check here. Once the 
feature has gone in I will revisit to see if I can add some performance 
improvements in this area.

Thanks,
Aravindh


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