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

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

On 29/03/17 15:01, Jan Beulich wrote:
>>>> On 29.03.17 at 15:50, <andrew.cooper3@citrix.com> wrote:
>> On 29/03/17 14:06, Jan Beulich wrote:
>>>>>> On 29.03.17 at 14:29, <andrew.cooper3@citrix.com> 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.

Mixed hex and dec is obviously a problem, but it is also very much a
problem for a number which isn't clear from context how it is
formatted.  *fn are all uniformly formatted as hex everywhere, whereas
slot/entry could easily be either.

>
>>>> @@ -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.

Will do on v2.

~Andrew

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