If no, it''s OK, or we should have a safe exit path. -Xin _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
And it seems better to let it return page_info * instead of mfn_t. -Xin>-----Original Message----- >From: xen-devel-bounces@lists.xensource.com >[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Li, Xin B >Sent: Thursday, July 12, 2007 5:32 PM >To: Tim Deegan; Huang2, Wei >Cc: xen-devel@lists.xensource.com >Subject: [Xen-devel] Will hap_alloc fail? > >If no, it''s OK, or we should have a safe exit path. >-Xin > >_______________________________________________ >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
At 17:37 +0800 on 12 Jul (1184261836), Li, Xin B wrote:> And it seems better to let it return page_info * instead of mfn_t.? Most of its call-sites want an MFN. But yes, it ought to handle running out of pages. Cheers, Tim. -- Tim Deegan <Tim.Deegan@xensource.com>, XenSource UK Limited Registered office c/o EC2Y 5EB, UK; company number 05334508 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan wrote:> At 17:37 +0800 on 12 Jul (1184261836), Li, Xin B wrote: >> And it seems better to let it return page_info * instead of mfn_t. > > ? Most of its call-sites want an MFN. > > But yes, it ought to handle running out of pages. > > Cheers, > > Tim.How does this fix sound? diff -r 552bfb5f589b xen/arch/x86/mm/hap/hap.c --- a/xen/arch/x86/mm/hap/hap.c Wed Jul 11 12:09:10 2007 -0500 +++ b/xen/arch/x86/mm/hap/hap.c Thu Jul 12 01:15:14 2007 -0500 @@ -94,9 +94,13 @@ mfn_t hap_alloc(struct domain *d) ASSERT(hap_locked_by_me(d)); + if ( list_empty(&d->arch.paging.hap.freelists) ) { + HAP_PRINTK("Can not allocate hap page!\n"); + BUG(); + } sp = list_entry(d->arch.paging.hap.freelists.next, struct page_info, list); list_del(&sp->list); - d->arch.paging.hap.free_pages -= 1; + d->arch.paging.hap.free_pages--; /* Now safe to clear the page for reuse */ p = hap_map_domain_page(page_to_mfn(sp)); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
At 11:34 -0500 on 12 Jul (1184240066), Huang2, Wei wrote:> How does this fix sound?I think we need somaething that crashes just the guest in question -- this patch just replaces a probable pagefault crash with an explicit BUG(). (The shadow code does BUG() if it can''t allocate a page but it has checks in place to make sure there''s always enough in the pool. It will refuse to give the p2m any memory if it''s running low, for example.) Cheers, Tim. -- Tim Deegan <Tim.Deegan@xensource.com>, XenSource UK Limited Registered office c/o EC2Y 5EB, UK; company number 05334508 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan wrote:> At 11:34 -0500 on 12 Jul (1184240066), Huang2, Wei wrote: >> How does this fix sound? > > I think we need somaething that crashes just the guest in question -- > this patch just replaces a probable pagefault crash with an explicit > BUG().Do you think domain_crash() work here? Or you want some way to propagate error information from hap_alloc() to upper levels? Thanks, -Wei _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
At 18:14 12/07/2007, Huang2, Wei wrote:>Tim Deegan wrote: > > At 11:34 -0500 on 12 Jul (1184240066), Huang2, Wei wrote: > >> How does this fix sound? > > > > I think we need somaething that crashes just the guest in question -- > > this patch just replaces a probable pagefault crash with an explicit > > BUG(). > >Do you think domain_crash() work here? Or you want some way to propagate >error information from hap_alloc() to upper levels?Note: I''m not speaking on behalf of ANYONE here. I would have thought that domain_crash() is the right thing to do - there''s nothing "better" that can be done elsewhere, as far as I can understand, and there''s really no point in propagating an error unless there''s something that can be done about it (or it can be ignored, which isn''t the case in this instance), as this only leads to potential misses of the propagated error, making it harder to debug. -- Mats>Thanks, > >-Wei > > > >_______________________________________________ >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
At 18:19 +0100 on 12 Jul (1184264340), Mats Petersson wrote:> I would have thought that domain_crash() is the right thing to do - > there''s nothing "better" that can be done elsewhere, as far as I can > understand, and there''s really no point in propagating an error > unless there''s something that can be done about it (or it can be > ignored, which isn''t the case in this instance), as this only leads > to potential misses of the propagated error, making it harder to debug.You need to do both, unfortunately. domain_crash() just marks the domain as crashed; we still need to survive the rest of the code path for the action we''re taking without following a null pointer or similar. Cheers, Tim. -- Tim Deegan <Tim.Deegan@xensource.com>, XenSource UK Limited Registered office c/o EC2Y 5EB, UK; company number 05334508 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 13/7/07 09:23, "Tim Deegan" <Tim.Deegan@xensource.com> wrote:> At 18:19 +0100 on 12 Jul (1184264340), Mats Petersson wrote: >> I would have thought that domain_crash() is the right thing to do - >> there''s nothing "better" that can be done elsewhere, as far as I can >> understand, and there''s really no point in propagating an error >> unless there''s something that can be done about it (or it can be >> ignored, which isn''t the case in this instance), as this only leads >> to potential misses of the propagated error, making it harder to debug. > > You need to do both, unfortunately. domain_crash() just marks the > domain as crashed; we still need to survive the rest of the code path > for the action we''re taking without following a null pointer or similar.Yes, we pretty much killed off usage of domain_crash_synchronous() because it was being used as the lazy way out at the expense of correctness. These low-level fallible routines often get called in spinlock contexts, for example. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel