[Xen-devel] RE: [PATCH]Add a flag for shadow pages

["Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>]

Keir Fraser <mailto:keir.fraser@xxxxxxxxxxxxx> wrote:
> I just realised that. You use get_page() to lock down a page's owner.
> Otherwise it can change under your feet anyway. You don't need

With get_page_owner() in get_page() will cause fault if it is a shadow page. Or 
you mean use exception table to protect it?

The page owner will not be changed anymore, since when a page freed, it will 
not be allocated anymore if marked page offlining. The worst situation is the 
page is assigned, but the owner firled is still not set (that action usually 
not protected by lock), then we return it is anonymous, that should be harmless 
as stated in the patch, since page_offline can't handle anonymous page.

Thanks
Yunhong Jiang

> a new flag.
> 
> -- Keir
> 
> On 04/03/2009 09:17, "Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx> wrote:
> 
>> This is mainly for page offline. When we offline a page, we we need turn to
>> the caller the page's owner but we can't distinguish easily if it is a
>> assigned page or a shadow page.
>> 
>> Thanks
>> Yunhong Jiang
>> 
>> Keir Fraser <mailto:keir.fraser@xxxxxxxxxxxxx> wrote:
>>> On 04/03/2009 08:46, "Jiang, Yunhong"
> <yunhong.jiang@xxxxxxxxx> wrote:
>>> 
>>>> Currently we don't know that a page is a shadow page unless we are in
>>>> shadow handler. This cause error when we try to get the page owner for
>>>> the shadow page, this snippet add a flag to it.
>>>> 
>>>> signed-off-by: Jiang, Yunhong <yunhong.jiang@xxxxxxxxx>
>>> 
>>> You only actually use your new flag in a BUG_ON, where it
>>> might avoid false
>>> negatives but hardly looks essential. Your other changes limit count_info
>>> checks to PGC_page_table|PGC_count_info which might be an improvement (I'm
>>> sure Tim or Gianluca can say) but they're existing fields.
>>> 
>>>> I'm not quite sure if the sh_put_ref() and
>>> sh_rm_write_access_from_sl1p() is
>>>> try to checking a page is shadow page (I assume so), because when a
>>>> anonymous page is allocated, the count_info is also 0 (like HVM's vlapic
>>>> page), so I change it like this patch (checking PGC_count_mask is 0).
>>>> Since comments in sh_hash_audit_bucket() has stated clearly it is to
>>>> check if it is shdow, so I replace it with test_bit().
>>>> 
>>>> Also, do we need checking in page_get_owner() also?
>>> 
>>> Usually gets used where we already get_page()ed or are about
>>> to get_page().
>>> Most Xen code knows that page uses can change under its feet
>>> unless it grabs
>>> a reference. So extra checks in page_get_owner() are probably rather
>>> pointless. 
>>> 
>>> -- Keir
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel