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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.05.17 at 18:54, <george.dunlap@xxxxxxxxxx> wrote:
> 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.

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


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