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

[Andres Lagar-Cavilla <andreslc@xxxxxxxxxxxxxx>]

Not sure about that. We need to check for either of two conditions on 
page->count_info. That it's zero, or that it's PGC_allocated | 1.

(__count & (PGC_count_mask|PGC_allocated)) matched against (1|PGC_allocated) 
would lose the zero case.

Resubmitting shortly.
Andres 

On Nov 25, 2011, at 4:18 AM, Keir Fraser wrote:

> On 25/11/2011 08:45, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>>>>> On 24.11.11 at 19:31, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
>>> On 24/11/2011 17:58, "Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx> wrote:
>>>> @@ -815,6 +815,12 @@ static inline unsigned long vtlb_lookup(
>>>> }
>>>> #endif /* (SHADOW_OPTIMIZATIONS & SHOPT_VIRTUAL_TLB) */
>>>> 
>>>> +static inline int __check_page_no_refs(struct page_info *page)
>>>> +{
>>>> +    unsigned long __count = page->count_info;
>>>> +    return ( (__count & PGC_count_mask) ==
>>>> +             ((__count & PGC_allocated) ? 1 : 0) );
>>> 
>>> It's a fussy point, but it might be better to use
>>> atomic_read_ulong(&page->count_info) here, to ensure that the compiler reads
>>> the field once only. It's cheap (compiles to a single mov instruction), but
>>> atomic_read_ulong doesn't exist so you'd have to add its (obvious)
>>> definition to asm-x86/atomic.h
>> 
>> I think Tim suggested anyway to use
>> 
>> (__count & (PGC_count_mask|PGC_allocated)) matched against
>> (1|PGC_allocated) here, which would eliminate the multiple read
>> potential if used in a switch statement.
> 
> Yes, that would be better.
> 
> -- Keir
> 
>> Also, for clarity the function should probably return bool_t.
>> 
>> Jan
>> 
> 
> 


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