From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH] x86: make dump_pageframe_info() slightly
 more verbose for dying domains
Date: Mon, 22 Sep 2014 11:31:29 +0100
Message-ID: <541FFA81.1040109@citrix.com>
References: <54130A920200007800034834@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <Andrew.Cooper3@citrix.com>) id 1XW0u6-0003P0-Ee
	for xen-devel@lists.xenproject.org; Mon, 22 Sep 2014 10:31:34 +0000
In-Reply-To: <54130A920200007800034834@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>, xen-devel <xen-devel@lists.xenproject.org>
Cc: Keir Fraser <keir@xen.org>
List-Id: xen-devel@lists.xenproject.org

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