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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.09.14 at 12:31, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxx>
>>
>> --- 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


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