Re: [PATCH] lib/alloc_tag: Remove the sysctl configuration to prevent users from disabling it at runtime

[Kent Overstreet <kent.overstreet@linux.dev>]

On Tue, Nov 12, 2024 at 11:30:39AM +0800, Hao Ge wrote:
> Hi Suren
> 
> 
> Firstly, please forgive me for my improper wording in the commit message.
> 
> After sending it, I realized that I should have used "suggestion" instead of
> "decided".
> 
> Secondly, please forgive me for taking a few days to respond. I've been
> quite busy these days.
> 
> 
> Let's continue to discuss this issue.
> 
> 
> On 11/9/24 02:16, Suren Baghdasaryan wrote:
> > On Thu, Nov 7, 2024 at 11:50 PM Hao Ge <hao.ge@linux.dev> wrote:
> > > From: Hao Ge <gehao@kylinos.cn>
> > > 
> > > After much consideration,I have decided to remove
> > > the "mem_profiling" sysctl interface to prevent
> > > users from dynamically enabling or disabling the
> > > MEMORY ALLOCATION PROFILING feature at runtime.
> > > 
> > > I have taken the following actions: I set
> > > CONFIG_MEM_ALLOC_PROFILING_ENABLED_BY_DEFAULT=y to
> > > enable memory allocation profiling by default,
> > > and then made adjustments to mem_profiling dynamically
> > > during runtime.
> > > 
> > > When I ran the OOM test program, I obtained useful
> > > information that was indeed very helpful for debugging.
> > > 
> > > [ 1023.065402] Memory allocations:
> > > [ 1023.065407]     12.8 GiB     6546 mm/huge_memory.c:1328 func:do_huge_pmd_anonymous_page
> > > [ 1023.065412]      873 MiB   229985 arch/arm64/mm/fault.c:986 func:vma_alloc_zeroed_movable_folio
> > > [ 1023.065415]      187 MiB    29732 mm/slub.c:2412 func:alloc_slab_page
> > > [ 1023.065418]     99.8 MiB    25560 mm/memory.c:1065 func:folio_prealloc
> > > [ 1023.065421]     47.2 MiB     3189 mm/readahead.c:434 func:ra_alloc_folio
> > > [ 1023.065424]     30.0 MiB       15 mm/khugepaged.c:1072 func:alloc_charge_folio
> > > [ 1023.065428]     28.6 MiB      514 mm/compaction.c:1880 func:compaction_alloc
> > > [ 1023.065430]     25.8 MiB     6592 mm/page_ext.c:271 func:alloc_page_ext
> > > [ 1023.065433]     25.6 MiB     6546 mm/huge_memory.c:1161 func:__do_huge_pmd_anonymous_page
> > > [ 1023.065436]     23.5 MiB     6017 mm/shmem.c:1771 func:shmem_alloc_folio
> > > 
> > > After running echo 0 > /proc/sys/vm/mem_profiling
> > > and then executing the same test program,
> > > I obtained the following results
> > > 
> > > [ 1156.509699] Memory allocations:
> > > [ 1156.509703]      187 MiB    29645 mm/slub.c:2412 func:alloc_slab_page
> > > [ 1156.509707]      142 MiB     9357 mm/readahead.c:434 func:ra_alloc_folio
> > > [ 1156.509710]      136 MiB    41325 arch/arm64/mm/fault.c:986 func:vma_alloc_zeroed_movable_folio
> > > [ 1156.509713]     99.7 MiB    25531 mm/memory.c:1065 func:folio_prealloc
> > > [ 1156.509716]     56.0 MiB       28 mm/huge_memory.c:1328 func:do_huge_pmd_anonymous_page
> > > [ 1156.509719]     30.0 MiB       15 mm/khugepaged.c:1072 func:alloc_charge_folio
> > > [ 1156.509723]     28.6 MiB      514 mm/compaction.c:1880 func:compaction_alloc
> > > [ 1156.509725]     26.3 MiB     7460 mm/readahead.c:264 func:page_cache_ra_unbounded
> > > [ 1156.509728]     25.8 MiB     6592 mm/page_ext.c:271 func:alloc_page_ext
> > > [ 1156.509730]     23.5 MiB     6016 mm/shmem.c:1771 func:shmem_alloc_folio
> > > 
> > > Because mem_profiling was disabled by executing
> > > echo 0 > /proc/sys/vm/mem_profiling,we are unable to
> > > record memory allocation information after the disablement.
> > Naturally you are unable to track the allocations after disabling it.
> > You disabled it as root, so I assume you know what you are doing.
> > 
> > > These output logs can mislead users. And similarly, the same
> > > applies to alloc_info.
> > I would understand if you made /proc/allocinfo empty after disabling
> > it to avoid confusing the user, but ripping out the ability to
> > enable/disable profiling at runtime does not make sense to me. Once
> > you collect required data, disabling profiling gets you back the
> > performance that you pay for it. There are usecases when a program on
> > a remote device periodically enables profiling for some time, records
> > the difference in allocations and then disables it. Your change breaks
> > such users.
> 
> 
> Actually, my original intention was also to make /proc/allocinfo empty when
> disabling it,
> 
> but I considered the following scenario: after we disable it and clear
> /proc/allocinfo,
> 
> we then start a memory-intensive application,
> 
> such as our OOM (Out-Of-Memory) test program.
> 
> If we later enable it again, the issue described in my commit message would
> still arise.
> 
> Perhaps we need to further consider how to handle this situation.

Why would you do such a thing?

We put a lot of effort into making memory allocation profiling cheap
enough to leave on, and I haven't seen a single complaint about
performance overhead.