| Message ID | 5908C2800200007800155F20@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/05/17 16:31, Jan Beulich wrote: >>>> On 02.05.17 at 17:15, <JBeulich@suse.com> wrote: >> get_page() logs a message when it fails (dom_cow is never dying or >> paging_mode_external()), so better avoid the call when it's pointless >> to do anyway. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> Possibly we could be even more rigid and bail right away if ->is_dying >> is set. >> >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -501,9 +501,9 @@ struct page_info *get_page_from_gfn_p2m( >> if ( fdom == NULL ) >> page = NULL; >> } >> - else if ( !get_page(page, d) >> + else if ( !get_page(page, d) && >> /* Page could be shared */ >> - && !get_page(page, dom_cow) ) >> + (!p2m_is_shared(*t) || !get_page(page, dom_cow)) ) >> page = NULL; >> } >> p2m_read_unlock(p2m); > > The downside of this change is that they will turn silent what may > be a hint towards a reason for one of the long standing migration > issues we have (these warnings have appeared in recent osstest > logs always in conjunction with a failed migration test). Locally I've > used > > --- unstable.orig/xen/arch/x86/mm/p2m.c > +++ unstable/xen/arch/x86/mm/p2m.c > @@ -480,6 +480,12 @@ struct page_info *get_page_from_gfn_p2m( > p2m_access_t _a; > p2m_type_t _t; > mfn_t mfn; > +static unsigned long cnt, thr;//temp > +if(d->is_dying && ++cnt > thr) {//temp > + cnt |= thr; Did you mean to reverse these here? As it is, unless you're modifying thr somewhere else, this will always be "cnt |= 0;" which will have no effect. > + printk("%pv: d%d dying (look up %lx)\n", current, d->domain_id, gfn); > + dump_execution_state(); > +} > > /* Allow t or a to be NULL */ > t = t ?: &_t; > > but with about a dozen migrations I didn't get this to trigger. I > therefore wonder whether we shouldn't, for a while, have > something like this in master. I haven't looked into the migration failure issue. If it was surrounded by #ifndef NDEBUG, it might be a reasonable approach. -George
>>> On 02.05.17 at 18:54, <george.dunlap@citrix.com> wrote: > On 02/05/17 16:31, Jan Beulich wrote: >>>>> On 02.05.17 at 17:15, <JBeulich@suse.com> wrote: >>> get_page() logs a message when it fails (dom_cow is never dying or >>> paging_mode_external()), so better avoid the call when it's pointless >>> to do anyway. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> Possibly we could be even more rigid and bail right away if ->is_dying >>> is set. >>> >>> --- a/xen/arch/x86/mm/p2m.c >>> +++ b/xen/arch/x86/mm/p2m.c >>> @@ -501,9 +501,9 @@ struct page_info *get_page_from_gfn_p2m( >>> if ( fdom == NULL ) >>> page = NULL; >>> } >>> - else if ( !get_page(page, d) >>> + else if ( !get_page(page, d) && >>> /* Page could be shared */ >>> - && !get_page(page, dom_cow) ) >>> + (!p2m_is_shared(*t) || !get_page(page, dom_cow)) ) >>> page = NULL; >>> } >>> p2m_read_unlock(p2m); >> >> The downside of this change is that they will turn silent what may >> be a hint towards a reason for one of the long standing migration >> issues we have (these warnings have appeared in recent osstest >> logs always in conjunction with a failed migration test). Locally I've >> used >> >> --- unstable.orig/xen/arch/x86/mm/p2m.c >> +++ unstable/xen/arch/x86/mm/p2m.c >> @@ -480,6 +480,12 @@ struct page_info *get_page_from_gfn_p2m( >> p2m_access_t _a; >> p2m_type_t _t; >> mfn_t mfn; >> +static unsigned long cnt, thr;//temp >> +if(d->is_dying && ++cnt > thr) {//temp >> + cnt |= thr; > > Did you mean to reverse these here? As it is, unless you're modifying > thr somewhere else, this will always be "cnt |= 0;" which will have no > effect. Oh, yes, of course. I must have been typing this in too mechanically, as I use this construct quite frequently when I'm unsure whether a message might trigger often. >> + printk("%pv: d%d dying (look up %lx)\n", current, d->domain_id, gfn); >> + dump_execution_state(); >> +} >> >> /* Allow t or a to be NULL */ >> t = t ?: &_t; >> >> but with about a dozen migrations I didn't get this to trigger. I >> therefore wonder whether we shouldn't, for a while, have >> something like this in master. > > I haven't looked into the migration failure issue. If it was surrounded > by #ifndef NDEBUG, it might be a reasonable approach. Yes, putting it inside such a conditional (and removing the //temp markers, which I use just for myself to make debugging code stand out, just like the seemingly bogus indentation) was of course the plan if we agreed to have this in master for a while. Jan
--- unstable.orig/xen/arch/x86/mm/p2m.c +++ unstable/xen/arch/x86/mm/p2m.c @@ -480,6 +480,12 @@ struct page_info *get_page_from_gfn_p2m( p2m_access_t _a; p2m_type_t _t; mfn_t mfn; +static unsigned long cnt, thr;//temp +if(d->is_dying && ++cnt > thr) {//temp + cnt |= thr; + printk("%pv: d%d dying (look up %lx)\n", current, d->domain_id, gfn); + dump_execution_state(); +} /* Allow t or a to be NULL */ t = t ?: &_t;