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

[Tim Deegan <tim@xxxxxxx>]

At 07:24 -0800 on 24 Nov (1322119466), Andres Lagar-Cavilla wrote:
> >>> @@ -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.

It doesn't need to be protected by the lock.  All we care about is that
at some point after the p2m update, the count has fallen to the correct
value.

It would probably be better to explicitly snapshot 
(page->count_info & (PGC_count_mask | PGC_allocated) and make sure that
that is either 0 or (1 | PGC_allocated).  (Yes, the code you're copying
from does it the other way, and it probably shouldn't either).

> >>
> >>>          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.

I don't think there's any interlock stopping another CPU from changing
PGC_allocated, though.  

> 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.

True.  A helper function that does the snapshot-and-test and is called
from all three places would be an improvement. 

> I'll resend this one.

Thanks.

Tim.

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