Re: [Xen-devel] [PATCH] x86/mm: silence a pointless warning

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 02/05/17 16:31, Jan Beulich wrote:
>>>> On 02.05.17 at 17:15, <JBeulich@xxxxxxxx> 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@xxxxxxxx>
>> ---
>> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel