Re: [Xen-devel] [PATCH 05 of 14] Don't trigger unnecessary shadow scans on p2m entry update

["Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>]

Jan,
>  >>> On 24.11.11 at 10:48, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>>>> On 23.11.11 at 22:11, Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>>>>> wrote:
>>> xen/arch/x86/mm/shadow/common.c |  4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>>
>>> When updating a p2m entry, the hypervisor scans all shadow pte's to
>>> find
>>> mappings of that gfn and tear them down. This is avoided if the page
>>> count
>>> reveals that there are no additional mappings. The current test ignores
>>> the
>>> PGC_allocated flag and its effect on the page count.
>>>
>>> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>>> Signed-off-by: Adin Scannell <adin@xxxxxxxxxxx>
>>>
>>> diff -r 93066bdc1e1c -r e8f0709af2b7 xen/arch/x86/mm/shadow/common.c
>>> --- a/xen/arch/x86/mm/shadow/common.c
>>> +++ b/xen/arch/x86/mm/shadow/common.c
>>> @@ -2501,7 +2501,8 @@ int sh_remove_all_mappings(struct vcpu *
>>>          ;
>>>
>>>      perfc_incr(shadow_mappings);
>>> -    if ( (page->count_info & PGC_count_mask) == 0 )
>>> +    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;
>>> +    if ( (page->count_info & PGC_count_mask) == expected_count )
>>
>> Is that really valid outside the locked region?
We can move it below to be protected by the lock.

>>
>>>          return 0;
>>>
>>>      /* Although this is an externally visible function, we do not know
>>> @@ -2517,7 +2518,6 @@ int sh_remove_all_mappings(struct vcpu *
>>>      hash_foreach(v, callback_mask, callbacks, gmfn);
>>>
>>>      /* If that didn't catch the mapping, something is very wrong */
>>> -    expected_count = (page->count_info & PGC_allocated) ? 1 : 0;
>>
>> This certainly isn't right - iiuc the count would normally have changed
>> during the hash_foreach() above this.

I don't think that's a problem. The expected count is still the same. The
shadow scan callback will not change the PGC_allocated flag in the target
page. Further, the sh_rm_mappings_from_l1 should also be updated. It
breaks out of the loop on a count of zero, but it should also break out on
1 | PGC_allocated.

I'll resend this one.
Andres
>
> Hmm, I was wrong here, you aren't caching the page count. Still, is it
> certain that the state of PGC_allocated doesn't change from where
> you set expected_count now to where it was set before?
>
> Jan
>
>



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel