* [PATCH] x86: make dump_pageframe_info() slightly more verbose for dying domains
@ 2014-09-12 13:00 Jan Beulich
2014-09-22 10:31 ` Andrew Cooper
0 siblings, 1 reply; 3+ messages in thread
From: Jan Beulich @ 2014-09-12 13:00 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]
Allowing more than just 10 pages to be printed in this case gives a
better chance to fully understand eventual page reference leaks.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -151,15 +151,30 @@ void dump_pageframe_info(struct domain *
printk("Memory pages belonging to domain %u:\n", d->domain_id);
- if ( d->tot_pages >= 10 )
+ if ( d->tot_pages >= 10 && d->is_dying < DOMDYING_dead )
{
printk(" DomPage list too long to display\n");
}
else
{
+ unsigned long total[PGT_type_mask
+ / (PGT_type_mask & -PGT_type_mask) + 1] = {};
+
spin_lock(&d->page_alloc_lock);
page_list_for_each ( page, &d->page_list )
{
+ unsigned int index = (page->u.inuse.type_info & PGT_type_mask)
+ / (PGT_type_mask & -PGT_type_mask);
+
+ if ( ++total[index] > 16 )
+ {
+ switch ( page->u.inuse.type_info & PGT_type_mask )
+ {
+ case PGT_none:
+ case PGT_writable_page:
+ continue;
+ }
+ }
printk(" DomPage %p: caf=%08lx, taf=%" PRtype_info "\n",
_p(page_to_mfn(page)),
page->count_info, page->u.inuse.type_info);
[-- Attachment #2: x86-dom-print.patch --]
[-- Type: text/plain, Size: 1503 bytes --]
x86: make dump_pageframe_info() slightly more verbose for dying domains
Allowing more than just 10 pages to be printed in this case gives a
better chance to fully understand eventual page reference leaks.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -151,15 +151,30 @@ void dump_pageframe_info(struct domain *
printk("Memory pages belonging to domain %u:\n", d->domain_id);
- if ( d->tot_pages >= 10 )
+ if ( d->tot_pages >= 10 && d->is_dying < DOMDYING_dead )
{
printk(" DomPage list too long to display\n");
}
else
{
+ unsigned long total[PGT_type_mask
+ / (PGT_type_mask & -PGT_type_mask) + 1] = {};
+
spin_lock(&d->page_alloc_lock);
page_list_for_each ( page, &d->page_list )
{
+ unsigned int index = (page->u.inuse.type_info & PGT_type_mask)
+ / (PGT_type_mask & -PGT_type_mask);
+
+ if ( ++total[index] > 16 )
+ {
+ switch ( page->u.inuse.type_info & PGT_type_mask )
+ {
+ case PGT_none:
+ case PGT_writable_page:
+ continue;
+ }
+ }
printk(" DomPage %p: caf=%08lx, taf=%" PRtype_info "\n",
_p(page_to_mfn(page)),
page->count_info, page->u.inuse.type_info);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] x86: make dump_pageframe_info() slightly more verbose for dying domains
2014-09-12 13:00 [PATCH] x86: make dump_pageframe_info() slightly more verbose for dying domains Jan Beulich
@ 2014-09-22 10:31 ` Andrew Cooper
2014-09-22 10:46 ` Jan Beulich
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Cooper @ 2014-09-22 10:31 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
On 12/09/14 14:00, Jan Beulich wrote:
> Allowing more than just 10 pages to be printed in this case gives a
> better chance to fully understand eventual page reference leaks.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -151,15 +151,30 @@ void dump_pageframe_info(struct domain *
>
> printk("Memory pages belonging to domain %u:\n", d->domain_id);
>
> - if ( d->tot_pages >= 10 )
> + if ( d->tot_pages >= 10 && d->is_dying < DOMDYING_dead )
> {
> printk(" DomPage list too long to display\n");
> }
> else
> {
> + unsigned long total[PGT_type_mask
> + / (PGT_type_mask & -PGT_type_mask) + 1] = {};
Is it possible to make use of something like:
#define LSB(x) ((x) & -(x))
for code clarity? (and perhaps for those whose bit manipulation
knowledge is a tad rusty)
Furthermore, the divide needs brackets per Xen style. The current
layout makes it easy to mistake the +1 as being part of the divisor.
> +
> spin_lock(&d->page_alloc_lock);
> page_list_for_each ( page, &d->page_list )
> {
> + unsigned int index = (page->u.inuse.type_info & PGT_type_mask)
> + / (PGT_type_mask & -PGT_type_mask);
> +
> + if ( ++total[index] > 16 )
> + {
> + switch ( page->u.inuse.type_info & PGT_type_mask )
> + {
> + case PGT_none:
> + case PGT_writable_page:
> + continue;
> + }
> + }
So, the new behaviour is to print all non none or writable_page, and
print the first 16 of each of those? This is probably fine for
dying/dead domains only, but I feel it warrants a comment in the patch
description.
~Andrew
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] x86: make dump_pageframe_info() slightly more verbose for dying domains
2014-09-22 10:31 ` Andrew Cooper
@ 2014-09-22 10:46 ` Jan Beulich
0 siblings, 0 replies; 3+ messages in thread
From: Jan Beulich @ 2014-09-22 10:46 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 22.09.14 at 12:31, <andrew.cooper3@citrix.com> wrote:
> On 12/09/14 14:00, Jan Beulich wrote:
>> Allowing more than just 10 pages to be printed in this case gives a
>> better chance to fully understand eventual page reference leaks.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -151,15 +151,30 @@ void dump_pageframe_info(struct domain *
>>
>> printk("Memory pages belonging to domain %u:\n", d->domain_id);
>>
>> - if ( d->tot_pages >= 10 )
>> + if ( d->tot_pages >= 10 && d->is_dying < DOMDYING_dead )
>> {
>> printk(" DomPage list too long to display\n");
>> }
>> else
>> {
>> + unsigned long total[PGT_type_mask
>> + / (PGT_type_mask & -PGT_type_mask) + 1] = {};
>
> Is it possible to make use of something like:
>
> #define LSB(x) ((x) & -(x))
>
> for code clarity? (and perhaps for those whose bit manipulation
> knowledge is a tad rusty)
Actually I should make use of MASK_EXTR() here. It's just that the
patch here significantly pre-dates the introduction of that and
MASK_INSR().
> Furthermore, the divide needs brackets per Xen style. The current
> layout makes it easy to mistake the +1 as being part of the divisor.
I don't think so - the C and mathematical meanings agree here.
>> +
>> spin_lock(&d->page_alloc_lock);
>> page_list_for_each ( page, &d->page_list )
>> {
>> + unsigned int index = (page->u.inuse.type_info & PGT_type_mask)
>> + / (PGT_type_mask & -PGT_type_mask);
>> +
>> + if ( ++total[index] > 16 )
>> + {
>> + switch ( page->u.inuse.type_info & PGT_type_mask )
>> + {
>> + case PGT_none:
>> + case PGT_writable_page:
>> + continue;
>> + }
>> + }
>
> So, the new behaviour is to print all non none or writable_page, and
> print the first 16 of each of those? This is probably fine for
> dying/dead domains only, but I feel it warrants a comment in the patch
> description.
Yeah, could do so. I actually further wonder whether the two
thresholds shouldn't become command line controllable (and
possibly get folded into one).
Jan
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-09-22 10:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-12 13:00 [PATCH] x86: make dump_pageframe_info() slightly more verbose for dying domains Jan Beulich
2014-09-22 10:31 ` Andrew Cooper
2014-09-22 10:46 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).