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

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Dmitry,

On Fri, 9 Jun 2023 14:34:15 +0300
Dmitry Rokosov via buildroot <buildroot@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@sberdevices.ru>

Thanks for your patch, and sorry for the delay to get back to you with
a review.


> 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?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot