Re: [PATCH V2] mm/ptdump: Drop GENERIC_PTDUMP

[Mark Rutland]

On Wed, Feb 05, 2025 at 10:30:39AM +0530, Anshuman Khandual wrote:
> GENERIC_PTDUMP does not guard any code but instead just used for platform's
> subscription into core ptdump defined under PTDUMP_CORE, which is selected.

Selected by what?

> Instead use PTDUMP_CORE for platform subscription and drop GENERIC_PTDUMP.
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Jonathan Corbet <corbet@xxxxxxx>
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> Cc: Nicholas Piggin <npiggin@xxxxxxxxx>
> Cc: Paul Walmsley <paul.walmsley@xxxxxxxxxx>
> Cc: Palmer Dabbelt <palmer@xxxxxxxxxxx>
> Cc: Heiko Carstens <hca@xxxxxxxxxxxxx>
> Cc: Vasily Gorbik <gor@xxxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-doc@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: kvmarm@xxxxxxxxxxxxxxx
> Cc: linuxppc-dev@xxxxxxxxxxxxxxxx
> Cc: linux-riscv@xxxxxxxxxxxxxxxxxxx
> Cc: linux-s390@xxxxxxxxxxxxxxx
> Cc: linux-mm@xxxxxxxxx
> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> ---
> This patch applies on v6.14-rc1
> 
> Changes in V2:
> 
> - Keep arch/powerpc/Kconfig alphabetically sorted per Christophe
> 
> Changes in V1:
> 
> https://lore.kernel.org/all/20241217034807.2541349-1-anshuman.khandual@xxxxxxx/
> 
>  Documentation/arch/arm64/ptdump.rst       | 1 -
>  arch/arm64/Kconfig                        | 2 +-
>  arch/arm64/kvm/Kconfig                    | 3 +--
>  arch/powerpc/Kconfig                      | 2 +-
>  arch/powerpc/configs/mpc885_ads_defconfig | 1 -
>  arch/riscv/Kconfig                        | 2 +-
>  arch/s390/Kconfig                         | 2 +-
>  arch/x86/Kconfig                          | 2 +-
>  arch/x86/Kconfig.debug                    | 2 +-
>  kernel/configs/debug.config               | 1 -
>  mm/Kconfig.debug                          | 8 ++------
>  11 files changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/arch/arm64/ptdump.rst b/Documentation/arch/arm64/ptdump.rst
> index 5dcfc5d7cddf..61ca040a885b 100644
> --- a/Documentation/arch/arm64/ptdump.rst
> +++ b/Documentation/arch/arm64/ptdump.rst
> @@ -22,7 +22,6 @@ offlining of memory being accessed by the ptdump code.
>  In order to dump the kernel page tables, enable the following
>  configurations and mount debugfs::
>  
> - CONFIG_GENERIC_PTDUMP=y
>   CONFIG_PTDUMP_CORE=y
>   CONFIG_PTDUMP_DEBUGFS=y
>  
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index fcdd0ed3eca8..1f516bed81dd 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -157,7 +157,7 @@ config ARM64
>  	select GENERIC_IRQ_SHOW_LEVEL
>  	select GENERIC_LIB_DEVMEM_IS_ALLOWED
>  	select GENERIC_PCI_IOMAP
> -	select GENERIC_PTDUMP
> +	select PTDUMP_CORE
>  	select GENERIC_SCHED_CLOCK
>  	select GENERIC_SMP_IDLE_THREAD
>  	select GENERIC_TIME_VSYSCALL

This change means that the ptdump core code will be built regardless of
whether any users are selected:

  [mark@lakrids:~/src/linux]% git grep CONFIG_PTDUMP_CORE
  Documentation/arch/arm64/ptdump.rst: CONFIG_PTDUMP_CORE=y
  arch/arm64/include/asm/ptdump.h:#ifdef CONFIG_PTDUMP_CORE
  arch/arm64/include/asm/ptdump.h:#endif /* CONFIG_PTDUMP_CORE */
  arch/arm64/mm/Makefile:obj-$(CONFIG_PTDUMP_CORE)        += ptdump.o
  arch/powerpc/mm/Makefile:obj-$(CONFIG_PTDUMP_CORE)      += ptdump/
  arch/riscv/mm/Makefile:obj-$(CONFIG_PTDUMP_CORE) += ptdump.o
  arch/s390/mm/Makefile:obj-$(CONFIG_PTDUMP_CORE) += dump_pagetables.o
  arch/x86/mm/Makefile:obj-$(CONFIG_PTDUMP_CORE)  += dump_pagetables.o
  mm/Makefile:obj-$(CONFIG_PTDUMP_CORE) += ptdump.o

GENERIC_PTDUMP means "this architecture uses generic ptdump code for
ptdump", i.e. the architecture implements all the necessary bits for
that to work *IF* it is built.

PTDUMP_CORE means "actually build the core ptdump code".

If everyone uses the generic ptdump code now, maybe it's worth renaming
GENERIC_PTDUMP to ARCH_HAS_PTDUMP or something like that, but I don't
think this change makes sense as-is.

[...]

> diff --git a/kernel/configs/debug.config b/kernel/configs/debug.config
> index 20552f163930..8aafd050b754 100644
> --- a/kernel/configs/debug.config
> +++ b/kernel/configs/debug.config
> @@ -73,7 +73,6 @@ CONFIG_DEBUG_VM=y
>  CONFIG_DEBUG_VM_PGFLAGS=y
>  CONFIG_DEBUG_VM_RB=y
>  CONFIG_DEBUG_VM_VMACACHE=y
> -CONFIG_GENERIC_PTDUMP=y
>  CONFIG_KASAN=y
>  CONFIG_KASAN_GENERIC=y
>  CONFIG_KASAN_INLINE=y

I think this is wrong today, and removing it is the right thing to do.

Architectures with support will select this themselves, and on
architectures without support this either does nothing or causes a build
failure.

Mark.