On Thu, 13 Mar 2025 16:26:22 +0100
Petr Mladek <pmladek@xxxxxxxx> wrote:
> This causes regression in the printf selftest:
>
> # modprobe test_printf
> modprobe: ERROR: could not insert 'test_printf': Invalid argument
>
> # dmesg | tail
> [ 46.206779] test_printf: vsnprintf(buf, 256, "%pGg", ...) returned 15, expected 10
> [ 46.208192] test_printf: vsnprintf(buf, 3, "%pGg", ...) returned 15, expected 10
> [ 46.208196] test_printf: vsnprintf(buf, 0, "%pGg", ...) returned 15, expected 10
> [ 46.208199] test_printf: kvasprintf(..., "%pGg", ...) returned 'none|0xfc000000', expected '0xfc000000'
> [ 46.208202] test_printf: vsnprintf(buf, 256, "%pGg", ...) returned 26, expected 21
> [ 46.208204] test_printf: vsnprintf(buf, 17, "%pGg", ...) returned 26, expected 21
> [ 46.208206] test_printf: vsnprintf(buf, 0, "%pGg", ...) returned 26, expected 21
> [ 46.208209] test_printf: kvasprintf(..., "%pGg", ...) returned '__GFP_HIGH|none|0xfc000000', expected '__GFP_HIGH|0xfc000000'
> [ 46.208865] test_printf: failed 8 out of 448 tests
>
> => vprintf() started printing the "none|" string.
>
> It seems to me that "{ 0, "none" }" was added as an "innocent" entry
> to avoid the trailing "," generated by TRACE_GFP_FLAGS. So, it is
> not really needed.
>
> In fact, I think that it probably causes similar regression in the
> trace output because the logic in trace_print_flags_seq()
> seems to be the same as in format_flags() in lib/vsprintf.c.
>
> The following worked for me:
>
> diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
> index 82371177ef79..15aae955a10b 100644
> --- a/include/trace/events/mmflags.h
> +++ b/include/trace/events/mmflags.h
> @@ -101,7 +101,7 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
> gfpflag_string(GFP_DMA32), \
> gfpflag_string(__GFP_RECLAIM), \
> TRACE_GFP_FLAGS \
> - { 0, "none" }
> + { 0, NULL }
>
> #define show_gfp_flags(flags) \
> (flags) ? __print_flags(flags, "|", __def_gfpflag_names \
>
> It seems to be safe because the callers end up the cycle when .name == NULL.
>
> I think that it actually allows to remove similar trailing {} but I am not sure
> if we want it.
Hmm, I could get rid of that last one with this patch. What do you think?
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index 82371177ef79..74af49c33d14 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -37,26 +37,26 @@
TRACE_GFP_EM(HARDWALL) \
TRACE_GFP_EM(THISNODE) \
TRACE_GFP_EM(ACCOUNT) \
- TRACE_GFP_EM(ZEROTAGS)
+ TRACE_GFP_EME(ZEROTAGS)
#ifdef CONFIG_KASAN_HW_TAGS
# define TRACE_GFP_FLAGS_KASAN \
- TRACE_GFP_EM(SKIP_ZERO) \
- TRACE_GFP_EM(SKIP_KASAN)
+ TRACE_COMMA TRACE_GFP_EM(SKIP_ZERO) \
+ TRACE_GFP_EME(SKIP_KASAN)
#else
# define TRACE_GFP_FLAGS_KASAN
#endif
#ifdef CONFIG_LOCKDEP
# define TRACE_GFP_FLAGS_LOCKDEP \
- TRACE_GFP_EM(NOLOCKDEP)
+ TRACE_COMMA TRACE_GFP_EME(NOLOCKDEP)
#else
# define TRACE_GFP_FLAGS_LOCKDEP
#endif
#ifdef CONFIG_SLAB_OBJ_EXT
# define TRACE_GFP_FLAGS_SLAB \
- TRACE_GFP_EM(NO_OBJ_EXT)
+ TRACE_COMMA TRACE_GFP_EME(NO_OBJ_EXT)
#else
# define TRACE_GFP_FLAGS_SLAB
#endif
@@ -69,6 +69,10 @@
#undef TRACE_GFP_EM
#define TRACE_GFP_EM(a) TRACE_DEFINE_ENUM(___GFP_##a##_BIT);
+#undef TRACE_GFP_EME
+#define TRACE_GFP_EME(a) TRACE_DEFINE_ENUM(___GFP_##a##_BIT);
+#undef TRACE_COMMA
+#define TRACE_COMMA
TRACE_GFP_FLAGS
@@ -84,6 +88,10 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
*/
#undef TRACE_GFP_EM
#define TRACE_GFP_EM(a) gfpflag_string(__GFP_##a),
+#undef TRACE_GFP_EME
+#define TRACE_GFP_EME(a) gfpflag_string(__GFP_##a)
+#undef TRACE_COMMA
+#define TRACE_COMMA ,
#define __def_gfpflag_names \
gfpflag_string(GFP_TRANSHUGE), \
@@ -100,8 +108,7 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
gfpflag_string(GFP_DMA), \
gfpflag_string(GFP_DMA32), \
gfpflag_string(__GFP_RECLAIM), \
- TRACE_GFP_FLAGS \
- { 0, "none" }
+ TRACE_GFP_FLAGS
#define show_gfp_flags(flags) \
(flags) ? __print_flags(flags, "|", __def_gfpflag_names \
-- Steve
