[Buildroot] [PATCH v1] package/linux-tools: introduce linux mm tools

[Dmitry Rokosov]

Hello Thomas,

On Sun, Jul 23, 2023 at 11:14:49AM +0200, Thomas Petazzoni wrote:
> Hello Dmitry,
> 
> On Fri, 9 Jun 2023 14:34:15 +0300
> Dmitry Rokosov via buildroot <buildroot at buildroot.org> wrote:
> 
> > This toolset was designed to facilitate the testing, monitoring, and
> > tracing of various things with virtual memory, pages, and slab objects.
> > It is an invaluable resource for identifying and analyzing
> > memory-related issues, such as leaks and bottlenecks, and can greatly
> > enhance one's understanding of memory utilization within a system.
> > 
> > The mm toolset includes:
> >     - page_owner_sort: userspace helper to sort the output of
> >       /sys/kernel/debug/page_owner, which helps to know who allocates
> >       the page from kernel context
> >     - slabinfo: the tool which gets reports about slabs, for example
> >       show empty slabs, modify of slab debug options at runtime, display
> >       all information about a slabcache
> >     - page-types: a handy tool for querying page flags
> > 
> > Signed-off-by: Dmitry Rokosov <ddrokosov at sberdevices.ru>
> 
> Thanks for your patch, and sorry for the delay to get back to you with
> a review.

Many thanks for taking the time to review this patch series!

Please don't worry about the delay, as I completely understand that you
have a great deal of other review tasks to attend to.

> 
> 
> > diff --git a/package/linux-tools/linux-tool-mm.mk.in b/package/linux-tools/linux-tool-mm.mk.in
> > new file mode 100644
> > index 000000000000..a59f1c46ff97
> > --- /dev/null
> > +++ b/package/linux-tools/linux-tool-mm.mk.in
> > @@ -0,0 +1,59 @@
> > +################################################################################
> > +#
> > +# mm
> > +#
> > +################################################################################
> > +
> > +LINUX_TOOLS += mm
> > +
> > +MM_MAKE_OPTS = $(LINUX_MAKE_FLAGS) CC="$(TARGET_CC)"
> > +
> > +KVER = $(shell echo $(LINUX_VERSION_PROBED))
> > +KVER_MAJOR = $(word 1,$(subst ., ,$(KVER)))
> > +KVER_MINOR = $(word 2,$(subst ., ,$(KVER)))
> 
> All variables in a package must be prefixed with the package name.
> Indeed, all variables in Buildroot are global, so if you define KVER
> and another package defines KVER, they will conflict.
> 
> > +
> > +# For the first time tools/vm was introduced in the 3.4 kernel version
> > +KVER_MAJOR_MIN = 3
> > +KVER_MINOR_MIN = 4
> > +
> > +# Starting from 6.3 kernel version mm tools are located at tools/mm folder
> > +# instead of tools/vm
> > +KVER_MAJOR_MM = 6
> > +KVER_MINOR_MM = 3
> > +
> > +define MM_BUILD_CMDS
> > +	$(Q)if [ $(KVER_MAJOR) -lt $(KVER_MAJOR_MIN) ] || \
> > +		[ $(KVER_MAJOR) -eq $(KVER_MAJOR_MIN) -a \
> > +		  $(KVER_MINOR) -lt $(KVER_MINOR_MIN) ]; then \
> > +		echo -n "Your kernel version $(KVER_MAJOR).$(KVER_MINOR) is "; \
> > +		echo "too old and doesn't have the mm tools." ; \
> > +		echo -n "At least $(KVER_MAJOR_MIN).$(KVER_MINOR_MIN) "; \
> > +		echo "kernel must be used." ; \
> > +		exit 1 ; \
> > +	fi
> 
> I think this is not the approach we should take here, because it's not
> the approach taken by the other makefiles in package/linux-tools/.
> Rather than testing the kernel version, we test the presence/absence of
> a Makefile.
> 
> So something along the lines of:
> 
>         $(Q)if test -f $(LINUX_DIR)/tools/vm/Makefile ; then \
> 		MM_SUBDIR=vm
>         elif test -f $(LINUX_DIR)/tools/mm/Makefile ; then \
> 		MM_SUBDIR=m
> 	else \
>                 echo "Your kernel version is too old and does not have the mm tool." ; \
>                 echo "At least kernel 3.4 must be used." ; \
>                 exit 1 ; \
>         fi ; \
> 	$(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools \
> 		$(MM_MAKE_OPTS) $${MM_SUBDIR}
> 
> or something along those lines. And of course, ditto for the install
> step.
> 
> Could you rework your patch accordingly?


Ah, okay. I had considered that approach but chose KVER testing instead.
No problem, I will rework it in the next version.

-- 
Thank you,
Dmitry