Xen devel - Mar 2009 - [PATCH] Do not set page''s count_info directly

Page offline patch add several flag to page_info->count_info. However,
currently some code will try to set count_info after alloc_domheap_pages without
using "&" or "|" operation, this may cause the new flags
lost, since there are no protection. This patch try to make sure all write to
count_info will only impact specific field.

Also currently shadow code assume count_info is 0 for shadow page, however, this
is invalid after the new flags. Change some assert in shadow code.

Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>

Thanks
Yunhong Jiang

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

On Thu, Mar 5, 2009 at 10:11 AM, Jiang, Yunhong <yunhong.jiang@intel.com>
wrote:
> Page offline patch add several flag to page_info->count_info. However,
currently some code will try to set count_info after alloc_domheap_pages without
using "&" or "|" operation, this may cause the new flags
lost, since there are no protection. This patch try to make sure all write to
count_info will only impact specific field.
Hm, wouldn''t be better to add some comments in mm.h or do this through
a macro? I think that one would normally tend to suppose, after you
allocate a page, that the count_info is all yours to set. It usually
is, since the offlining should be a rare event (I hope?), so catching
this kind of bug would be very hard, and make the whole mechanism very
fragile.
> Also currently shadow code assume count_info is 0 for shadow page, however,
this is invalid after the new flags. Change some assert in shadow code.
Yes, that''s correct. I find, though, (count_info & PGC_count_mask
!0) as a check if the page is a shadow *very* confusing. Could you
define a macro with something like page_is_a_shadow_page() and hide
this mm.c dirty secret?

Thanks,
Gianluca

-- 
It was a type of people I did not know, I found them very strange and
they did not inspire confidence at all. Later I learned that I had been
introduced to electronic engineers.
                                                  E. W. Dijkstra

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

At 12:32 +0000 on 05 Mar (1236256358), Gianluca Guida
wrote:
> Yes, that''s correct. I find, though, (count_info &
PGC_count_mask !> 0) as a check if the page is a shadow *very* confusing.
Could you
> define a macro with something like page_is_a_shadow_page() and hide
> this mm.c dirty secret?
The check doesn''t tell you it''s a shadow page -- it just tells
you it''s
*not* allocated in another way.  Checking with the mask is OK here,
perhaps with a comment change.

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

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

On Thu, Mar 5, 2009 at 1:38 PM, Tim Deegan <Tim.Deegan@citrix.com>
wrote:
> The check doesn''t tell you it''s a shadow page -- it just
tells you it''s
> *not* allocated in another way.  Checking with the mask is OK here,
> perhaps with a comment change.
Yes, but this is what the code (in the shadow code) intends to check.
My complete plan is to have this macro and restore the idea of having
a flag for shadow pages because, as for now, the fact that we can''t
distinguish clearly a shadow page should be considered a sort of bug,
given the code assumptions. Or at least a good basis for future bugs.

Thanks,
Gianluca

-- 
It was a type of people I did not know, I found them very strange and
they did not inspire confidence at all. Later I learned that I had been
introduced to electronic engineers.
                                                  E. W. Dijkstra

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

On 05/03/2009 12:32, "Gianluca Guida" <glguida@gmail.com> wrote:
>> Also currently shadow code assume count_info is 0 for shadow page,
however,
>> this is invalid after the new flags. Change some assert in shadow code.
> 
> Yes, that''s correct. I find, though, (count_info &
PGC_count_mask !> 0) as a check if the page is a shadow *very* confusing.
Could you
> define a macro with something like page_is_a_shadow_page() and hide
> this mm.c dirty secret?
This would be a shadow-code-specific macro. In general count_mask==0 can
also mean the page is free, for example.

 -- Keir



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

On 05/03/2009 12:45, "Gianluca Guida" <glguida@gmail.com> wrote:
> On Thu, Mar 5, 2009 at 1:38 PM, Tim Deegan <Tim.Deegan@citrix.com>
wrote:
>> The check doesn''t tell you it''s a shadow page -- it
just tells you it''s
>> *not* allocated in another way.  Checking with the mask is OK here,
>> perhaps with a comment change.
> 
> Yes, but this is what the code (in the shadow code) intends to check.
> My complete plan is to have this macro and restore the idea of having
> a flag for shadow pages because, as for now, the fact that we
can''t
> distinguish clearly a shadow page should be considered a sort of bug,
> given the code assumptions. Or at least a good basis for future bugs.
I''m happy for shadow code to have another bit in count_info for its own
use.
I don''t think it''s good to have other code (e.g., generic page
offlining
code) peeking at it, is all. The particular case for a new flag described
above sounds fine.

 -- Keir



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

Keir Fraser <mailto:keir.fraser@eu.citrix.com>
wrote:
> On 05/03/2009 12:32, "Gianluca Guida" <glguida@gmail.com>
wrote:
> 
>>> Also currently shadow code assume count_info is 0 for shadow page,
>>> however, this is invalid after the new flags. Change some assert in
>>> shadow code. 
>> 
>> Yes, that''s correct. I find, though, (count_info &
PGC_count_mask !>> 0) as a check if the page is a shadow *very* confusing.
Could you
>> define a macro with something like page_is_a_shadow_page() and hide
>> this mm.c dirty secret?
> 
> This would be a shadow-code-specific macro. In general count_mask==0 can
> also mean the page is free, for example.
Maybe I''m wrong, but anonymous page will have count_mask==0 while they
are allocated.

Currently the only way that we can make sure the guest is free is through the
boot allocator bitmap. When the bit is clear, the page is free.
When the bit is set, and the count_mask!=0, then it is owned by someone like
guest ram, p2m table etc. if the count_mask==0, then it is either a shadow page
or a anoynymous page.

Or do I missed anything?

Thanks
Yunhong Jiang

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

Keir Fraser <mailto:keir.fraser@eu.citrix.com>
wrote:
> On 05/03/2009 12:45, "Gianluca Guida" <glguida@gmail.com>
wrote:
> 
>> On Thu, Mar 5, 2009 at 1:38 PM, Tim Deegan
> <Tim.Deegan@citrix.com> wrote:
>>> The check doesn''t tell you it''s a shadow page --
it just tells you it''s
>>> *not* allocated in another way.  Checking with the mask is OK here,
>>> perhaps with a comment change.
>> 
>> Yes, but this is what the code (in the shadow code) intends to check.
>> My complete plan is to have this macro and restore the idea of having
>> a flag for shadow pages because, as for now, the fact that we
can''t
>> distinguish clearly a shadow page should be considered a sort of bug,
>> given the code assumptions. Or at least a good basis for future bugs.
> 
> I''m happy for shadow code to have another bit in count_info
> for its own use.
> I don''t think it''s good to have other code (e.g., generic
page offlining
> code) peeking at it, is all. The particular case for a new
> flag described
> above sounds fine.
The new code is not using that flag anymore, it is use the
page_get_owner_and_reference() to get the owner.

Thanks
Yunhong Jiang
> 
> -- Keir
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Gianluca Guida <mailto:glguida@gmail.com> wrote:
> On Thu, Mar 5, 2009 at 10:11 AM, Jiang, Yunhong
> <yunhong.jiang@intel.com> wrote:
>> Page offline patch add several flag to
> page_info->count_info. However, currently some code will try
> to set count_info after alloc_domheap_pages without using "&"
> or "|" operation, this may cause the new flags lost, since
> there are no protection. This patch try to make sure all write
> to count_info will only impact specific field.
> 
> Hm, wouldn''t be better to add some comments in mm.h or do this
through
> a macro? I think that one would normally tend to suppose, after you
> allocate a page, that the count_info is all yours to set. It usually
> is, since the offlining should be a rare event (I hope?), so catching
> this kind of bug would be very hard, and make the whole mechanism very
> fragile. 
Yes, I considered how to prevent this kind of mis-use and in the end find it is
impossible. The caller can always set the count_info if they like it.

One option is to use another bitmap to keep the offline information instead of
page_info, but that will wast too many memory, since page offline is rare case
as you pointed out.
Another option is to enable this bitmap only in debug version. When free page,
we will check the bitmap, if the bitmap is set while the count_info is not set,
it means something wrong happens and raise a BUG().

Any suggestion?

Thanks
Yunhong Jiang
> 
>> Also currently shadow code assume count_info is 0 for shadow
> page, however, this is invalid after the new flags. Change
> some assert in shadow code.
> 
> Yes, that''s correct. I find, though, (count_info &
PGC_count_mask !> 0) as a check if the page is a shadow *very* confusing.
Could you
> define a macro with something like page_is_a_shadow_page() and hide this
> mm.c dirty secret? 
> 
> Thanks,
> Gianluca
> 
> --
> It was a type of people I did not know, I found them very strange and
> they did not inspire confidence at all. Later I learned that I had been
> introduced to electronic engineers.
>                                                  E. W. Dijkstra
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 05/03/2009 14:33, "Jiang, Yunhong" <yunhong.jiang@intel.com>
wrote:
>> This would be a shadow-code-specific macro. In general count_mask==0
can
>> also mean the page is free, for example.
> 
> Maybe I''m wrong, but anonymous page will have count_mask==0 while
they are
> allocated.
> 
> Currently the only way that we can make sure the guest is free is through
the
> boot allocator bitmap. When the bit is clear, the page is free.
> When the bit is set, and the count_mask!=0, then it is owned by someone
like
> guest ram, p2m table etc. if the count_mask==0, then it is either a shadow
> page or a anoynymous page.
> 
> Or do I missed anything?
You are correct. I only gave one obvious example where count_info==0.

 -- Keir



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

xen-devel-bounces@lists.xensource.com <> wrote:
> On 05/03/2009 14:33, "Jiang, Yunhong"
<yunhong.jiang@intel.com> wrote:
> 
>>> This would be a shadow-code-specific macro. In general
count_mask==0 can
>>> also mean the page is free, for example.
>> 
>> Maybe I''m wrong, but anonymous page will have count_mask==0
while they are
>> allocated. 
>> 
>> Currently the only way that we can make sure the guest is free is
through
>> the boot allocator bitmap. When the bit is clear, the page is free.
>> When the bit is set, and the count_mask!=0, then it is owned by someone
>> like guest ram, p2m table etc. if the count_mask==0, then it is either
a
>> shadow page or a anoynymous page. 
>> 
>> Or do I missed anything?
> 
> You are correct. I only gave one obvious example where count_info==0.
Thanks for your clarification.

-- Yunhong Jiang
> 
> -- Keir
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jiang, Yunhong wrote:
> Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:
>> On 05/03/2009 12:45, "Gianluca Guida"
<glguida@gmail.com> wrote:
>>
>>> On Thu, Mar 5, 2009 at 1:38 PM, Tim Deegan
>> <Tim.Deegan@citrix.com> wrote:
>>>> The check doesn''t tell you it''s a shadow page
-- it just tells you it''s
>>>> *not* allocated in another way.  Checking with the mask is OK
here,
>>>> perhaps with a comment change.
>>> Yes, but this is what the code (in the shadow code) intends to
check.
>>> My complete plan is to have this macro and restore the idea of
having
>>> a flag for shadow pages because, as for now, the fact that we
can''t
>>> distinguish clearly a shadow page should be considered a sort of
bug,
>>> given the code assumptions. Or at least a good basis for future
bugs.
>> I''m happy for shadow code to have another bit in count_info
>> for its own use.
>> I don''t think it''s good to have other code (e.g.,
generic page offlining
>> code) peeking at it, is all. The particular case for a new
>> flag described
>> above sounds fine.
> 
> The new code is not using that flag anymore, it is use the
page_get_owner_and_reference() to get the owner.
Yes, I was actually thinking about adding it. Turns out that I realized 
that it''s not the best way to do stuff, but I think that I''ll
do the
following:

- add a sh_page_is_a_shadow_page() in shadow code to make the code less 
confusing (with a small caveat comment about not being perfectly precise).
- exercise my English skills by writing a comment in page_info that 
summarizes this thread findings about current use if count_info and 
shadow pages.

Thanks,
Gianluca

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

>> 
>> The new code is not using that flag anymore, it is use the
> page_get_owner_and_reference() to get the owner.
> 
> Yes, I was actually thinking about adding it. Turns out that I realized
> that it''s not the best way to do stuff, but I think that
I''ll do the
> following: 
One clarification, the "new code" I''m talking about is my
page-offline patch.


Thanks
Yunhong Jiang
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel