Re: [Xen-devel] [PATCH] x86/mm: Drop MEM_LOG() and correct some printed information

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.03.17 at 15:50, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 29/03/17 14:06, Jan Beulich wrote:
>>>>> On 29.03.17 at 14:29, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> @@ -1068,10 +1073,10 @@ get_page_from_l1e(
>>>      return 0;
>>>  
>>>   could_not_pin:
>>> -    MEM_LOG("Error getting mfn %lx (pfn %lx) from L1 entry %" PRIpte
>>> -            " for l1e_owner=%d, pg_owner=%d",
>>> -            mfn, get_gpfn_from_mfn(mfn),
>>> -            l1e_get_intpte(l1e), l1e_owner->domain_id, 
>>> pg_owner->domain_id);
>>> +    gdprintk(XENLOG_WARNING, "Error getting mfn %" PRI_mfn " (pfn %" 
>>> PRI_pfn
>>> +             ") from L1 entry %" PRIpte " for l1e_owner d%d, pg_owner d%d",
>>> +             mfn, get_gpfn_from_mfn(mfn),
>>> +             l1e_get_intpte(l1e), l1e_owner->domain_id, 
>>> pg_owner->domain_id);
>> Especially here the wrapping of the format string is rather
>> unfortunate. Didn't we agree to allow format strings to exceed
>> the 80 column restriction anyway?
> 
> It is split at a formatting boundary, which doesn't affect grep-ability.
> 
> Putting this all on one line is 123 characters, which IMO is too long.

Hmm, you're right, 123 seems a little excessive.

>>> @@ -1388,7 +1398,7 @@ static int alloc_l1_table(struct page_info *page)
>>>      return 0;
>>>  
>>>   fail:
>>> -    MEM_LOG("Failure in alloc_l1_table: entry %d", i);
>>> +    gdprintk(XENLOG_WARNING, "Failure in alloc_l1_table: entry %d\n", i);
>> %u (or even %03x; same in alloc_l[234]_table())
> 
> Actually, "slot %#x" would be clearer here.  I though I fixed the 0x
> prefix in alloc_l[]_table(), and I am not sure the leading zeroes are
> helpful.

I'm not too fussed about the leading zeros, but I do actively
dislike 0x prefixes except when a message mixes hex and dec
numbers.

>>> @@ -4459,10 +4512,11 @@ int steal_page(
>>>  
>>>   fail:
>>>      spin_unlock(&d->page_alloc_lock);
>>> -    MEM_LOG("Bad page %lx: ed=%d sd=%d caf=%08lx taf=%" PRtype_info,
>>> -            page_to_mfn(page), d->domain_id,
>>> -            owner ? owner->domain_id : DOMID_INVALID,
>>> -            page->count_info, page->u.inuse.type_info);
>>> +    gdprintk(XENLOG_WARNING, "Bad mfn %" PRI_mfn
>>> +             ": ed=%d sd=%d caf=%08lx taf=%" PRtype_info "\n",
>>> +             page_to_mfn(page), d->domain_id,
>>> +             owner ? owner->domain_id : DOMID_INVALID,
>>> +             page->count_info, page->u.inuse.type_info);
>> Same here.
>>
>> Is this intended for 4.9?
> 
> At this point, yes.

In which case you should Cc Julien.

Jan


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