Re: [PATCH] mm: Reuse PRI_gfn macro instead of manual specify the format

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Sep 9, 2024 at 1:58 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:

On 09.09.2024 14:53, Frediano Ziglio wrote:
> On Mon, Sep 9, 2024 at 11:45 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 09.09.2024 12:08, Frediano Ziglio wrote:
>>> Macros are defined to avoid type mismatch in format strings
>>> but also to unify format amongst code.
>>
>> I'm certainly fine with this part.
>>
>>> In the meantime expands to 9 hexadecimal digits.
>>
>> What makes 9 special? What will the extra padding zeroes buy us?
>>
>>
> I think either we want kind of fixed size or dynamic. 9 == (48 - 12) / 4,
> although possibly you would prefer 13 == (64 - 12) / 4.

64 is too much for x86; it would want to be 52 there. The way it is right
now this is (imo deliberately) not arch-specific, though.

Yes, but still given the canonic form of x64 you would need to use 13 digits to have all the same size.

 

>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -1848,7 +1848,7 @@ gnttab_unpopulate_status_frames(struct domain *d,
>> struct grant_table *gt)
>>>              if ( rc )
>>>              {
>>>                  gprintk(XENLOG_ERR,
>>> -                        "Could not remove status frame %u (GFN %#lx)
>> from P2M\n",
>>> +                        "Could not remove status frame %u (GFN
>> %"PRI_gfn") from P2M\n",
>>
>> The lost # means the number won't identify itself as hex anymore. Rather
>> than ...
>>
>>> @@ -3981,7 +3981,7 @@ void grant_table_warn_active_grants(struct domain
>> *d)
>>>          if ( nr_active <= WARN_GRANT_MAX )
>>>              printk(XENLOG_G_DEBUG "d%d has active grant %x ("
>>>  #ifndef NDEBUG
>>> -                   "GFN %lx, "
>>> +                   "GFN %"PRI_gfn", "
>>>  #endif
>>>                     "MFN: %#"PRI_mfn")\n",
>>
>> (note this for below)
>>
>>> --- a/xen/include/xen/mm-frame.h
>>> +++ b/xen/include/xen/mm-frame.h
>>> @@ -5,7 +5,7 @@
>>>  #include <xen/typesafe.h>
>>>
>>>  TYPE_SAFE(unsigned long, mfn);
>>> -#define PRI_mfn          "05lx"
>>> +#define PRI_mfn          "09lx"
>>>  #define INVALID_MFN_RAW  (~0UL)
>>>  #define INVALID_MFN      _mfn(INVALID_MFN_RAW)
>>>  /*
>>> @@ -41,7 +41,7 @@ static inline bool mfn_eq(mfn_t x, mfn_t y)
>>>  }
>>>
>>>  TYPE_SAFE(unsigned long, gfn);
>>> -#define PRI_gfn          "05lx"
>>> +#define PRI_gfn          "09lx"
>>
>> ... moving to 09 (twice) here, how about we move to #? Requiring, of
>> course,
>> to drop already-questionable hashes like the one pointed out in the middle.
>>
> I suppose you are suggesting getting rid of the padding entirely and move
> to prefix, like "%#lx".

Yes, i.e.

#define PRI_mfn          "#lx"

Surely more portable amongst different platforms.

 

Then again I don't really know why "05lx" was chosen originally.

I assume x86 without PAE, 32 bit, so 5 == (32 - 12) / 4.

 

Jan

Sent updated one

Frediano