[PATCH] x86/mm: silence a pointless warning

[PATCH] x86/mm: silence a pointless warning

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 851 bytes --]

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);




[-- Attachment #2: x86-mm-silence-warning.patch --]
[-- Type: text/plain, Size: 884 bytes --]

x86/mm: silence a pointless warning

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);

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

>>> 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;
+ 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.

Jan


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

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

[George Dunlap]

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

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

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

[George Dunlap]

On 02/05/17 16:15, Jan Beulich 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>

The other option would be to add "domain == dom_cow" as a condition for
which get_page() doesn't report an error.

But on the whole I think this is probably the better option:

Acked-by: George Dunlap <george.dunlap@citrix.com>


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

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

[Jan Beulich]

>>> 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


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

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

[Jan Beulich]

>>> On 02.05.17 at 19:04, <george.dunlap@citrix.com> wrote:
> On 02/05/17 16:15, Jan Beulich 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>
> 
> The other option would be to add "domain == dom_cow" as a condition for
> which get_page() doesn't report an error.

That would imo be undesirable namely for the currently one case
where get_page(..., dom_cow) is being used from mem_sharing.c.

> But on the whole I think this is probably the better option:
> 
> Acked-by: George Dunlap <george.dunlap@citrix.com>

Thanks.

Jan


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