Re: [PATCH] x86: make dump_pageframe_info() slightly more verbose for dying domains

[Andrew Cooper <andrew.cooper3@citrix.com>]

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