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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

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.

 

> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> ---
>  xen/common/grant_table.c   | 6 +++---
>  xen/include/xen/mm-frame.h | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index ab36f45ded..775cd7e065 100644
> --- 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".

 

Jan

I can do it

Frediano