Re: [PATCH v5 2/6] mm/vmscan: move vmscan tracepoints to a local header

[Baoquan He]

On 11/02/26 at 06:46pm, Ridong Chen wrote:
> 
> 
> On 11/2/2026 9:37 AM, Baoquan He wrote:
> > On 10/31/26 at 05:00pm, Ridong Chen wrote:
> > > From: Ridong Chen <chenridong@xxxxxxxxxx>
> > > 
> > > Move include/trace/events/vmscan.h to mm/trace_vmscan.h so that the
> > > tracepoints can be extended to reference vmscan-internal structures such
> > > as struct scan_control, which is not visible outside mm/.  This mirrors
> > > what several other subsystems already do with their trace headers (e.g.
> > > fs/fuse/fuse_trace.h, fs/gfs2/trace_gfs2.h, fs/lockd/trace.h).  As they
> > > do, set TRACE_INCLUDE_PATH to "." and add mm/ to the include path of
> > > vmscan.o, the only user that defines CREATE_TRACE_POINTS.
> > > 
> > > No functional change.
> > > 
> > > Signed-off-by: Ridong Chen <chenridong@xxxxxxxxxx>
> > > Reviewed-by: Barry Song <baohua@xxxxxxxxxx>
> > > ---
> > >   MAINTAINERS                                        | 2 ++
> > >   mm/Makefile                                        | 1 +
> > >   mm/shrinker.c                                      | 2 +-
> > >   include/trace/events/vmscan.h => mm/trace_vmscan.h | 4 ++++
> > >   mm/vmscan.c                                        | 2 +-
> > >   5 files changed, 9 insertions(+), 2 deletions(-)
> > >   rename include/trace/events/vmscan.h => mm/trace_vmscan.h (99%)
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index aaf2825ac945..2ad01107fa05 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -17653,6 +17653,7 @@ F:	Documentation/admin-guide/mm/multigen_lru.rst
> > >   F:	Documentation/mm/multigen_lru.rst
> > >   F:	include/linux/mm_inline.h
> > >   F:	include/linux/mmzone.h
> > > +F:	mm/trace_vmscan.h
> > >   F:	mm/vmscan.c
> > >   F:	mm/workingset.c
> > > @@ -17802,6 +17803,7 @@ R:	Shakeel Butt <shakeel.butt@xxxxxxxxx>
> > >   R:	Lorenzo Stoakes <ljs@xxxxxxxxxx>
> > >   L:	linux-mm@xxxxxxxxx
> > >   S:	Maintained
> > > +F:	mm/trace_vmscan.h
> > >   F:	mm/vmscan.c
> > >   F:	mm/workingset.c
> > > diff --git a/mm/Makefile b/mm/Makefile
> > > index 2a3ec53d62ee..277b97f7f8d7 100644
> > > --- a/mm/Makefile
> > > +++ b/mm/Makefile
> > > @@ -32,6 +32,7 @@ KCOV_INSTRUMENT_vmstat.o := n
> > >   KCOV_INSTRUMENT_failslab.o := n
> > >   CFLAGS_init-mm.o += -Wno-override-init
> > > +CFLAGS_vmscan.o += -I$(src)
> > 
> > One nit:
> > 
> > Does it really need the above line? It affects mm/shrinker.c too, did your
> > shrinker.o building fail?
> > 
> > Seems it's redundant because kbuild adds -I$(src) -I$(obj) for every object
> > in the directory since commit cdd750bfb1f7 ("kbuild: remove 'addtree' and
> > 'flags' magic for header search paths"). Please correct me if I am
> > wrong.
> > 
> 
> Hi Baoquan,
> 
> Thanks for the review.
> 
> The line is needed for vmscan.o, and mm/shrinker.c is fine without it,
> because the two files include trace_vmscan.h in different ways.
> 
> mm/shrinker.c only does a plain #include "trace_vmscan.h". The lookup
> starts from mm/, where the header lives, so no -I is needed.
> 
> mm/vmscan.c defines CREATE_TRACE_POINTS before including the header.
> In that case <trace/define_trace.h>, included at the end of
> trace_vmscan.h, includes the header again to generate the tracepoint
> definitions:
> 
> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)  /* -> "./trace_vmscan.h" */
> 
> This #include is in include/trace/define_trace.h, so the lookup starts
> from include/trace/ and only finds the header through -I. Without the
> flag, vmscan.o fails to build:
> 
> ./include/trace/define_trace.h:118:42: fatal error: ./trace_vmscan.h: No
> such file or directory
> 
> As for cdd750bfb1f7, it doesn't add -I$(src) for every build. kbuild
> only adds -I$(src) -I$(obj) automatically when building with O=
> (see "ifdef building_out_of_srctree" in scripts/Makefile.lib). So
> with O= the line is indeed redundant and vmscan.o builds fine without
> it, but a plain in-tree "make" fails with the error above. The
> explicit flag keeps both working.
> 
> To be honest, I had the same worry at first: I thought every .c file
> that includes the header would need a Makefile change. That's why v3
> used a relative TRACE_INCLUDE_PATH (../../mm) instead. Steven corrected
> me and pointed out that the proper way is TRACE_INCLUDE_PATH "." plus
> -I$(src) for the one object that defines CREATE_TRACE_POINTS [1]. That
> turned out to need only this one line. Thanks again to Steven for
> that.

You are right, my nit was wrong. Learned it from you, thanks for the
detailed explanation.

> 
> [1] https://lore.kernel.org/all/20261014124914.21c21559@xxxxxxxxxxxxxxxxxx/
> 
> 
> > Other than the nit, this patch looks good to me,
> > 
> > Reviewed-by: Baoquan He <baoquan.he@xxxxxxxxx>
> > 
> > Thanks.
> > >   mmu-y			:= nommu.o
> > >   mmu-$(CONFIG_MMU)	:= highmem.o memory.o mincore.o \
> > > diff --git a/mm/shrinker.c b/mm/shrinker.c
> > > index 4fb424e721bc..4fbdade6d2f6 100644
> > > --- a/mm/shrinker.c
> > > +++ b/mm/shrinker.c
> > > @@ -3,7 +3,7 @@
> > >   #include <linux/rwsem.h>
> > >   #include <linux/shrinker.h>
> > >   #include <linux/rculist.h>
> > > -#include <trace/events/vmscan.h>
> > > +#include "trace_vmscan.h"
> > >   #include "internal.h"
> > > diff --git a/include/trace/events/vmscan.h b/mm/trace_vmscan.h
> > > similarity index 99%
> > > rename from include/trace/events/vmscan.h
> > > rename to mm/trace_vmscan.h
> > > index 8a872990b4be..ad90f0e9bf2e 100644
> > > --- a/include/trace/events/vmscan.h
> > > +++ b/mm/trace_vmscan.h
> > > @@ -648,4 +648,8 @@ TRACE_EVENT(mm_vmscan_kswapd_clear_hopeless,
> > >   #endif /* _TRACE_VMSCAN_H */
> > >   /* This part must be outside protection */
> > > +#undef TRACE_INCLUDE_FILE
> > > +#undef TRACE_INCLUDE_PATH
> > > +#define TRACE_INCLUDE_PATH .
> > > +#define TRACE_INCLUDE_FILE trace_vmscan
> > >   #include <trace/define_trace.h>
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 9959ca158ce8..79d2d614afd0 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -71,7 +71,7 @@
> > >   #include "swap.h"
> > >   #define CREATE_TRACE_POINTS
> > > -#include <trace/events/vmscan.h>
> > > +#include "trace_vmscan.h"
> > >   struct scan_control {
> > >   	/* How many pages shrink_list() should reclaim */
> > > -- 
> > > 2.43.0
> > > 
> 
> -- 
> Best regards
> Ridong
>