On Wed 02-04-25 11:25:12, Vlastimil Babka wrote: > On 4/2/25 10:42, Yafang Shao wrote: > > On Wed, Apr 2, 2025 at 12:15 PM Harry Yoo <harry.yoo@xxxxxxxxxx> wrote: > >> > >> On Tue, Apr 01, 2025 at 07:01:04AM -0700, Kees Cook wrote: > >> > > >> > > >> > On April 1, 2025 12:30:46 AM PDT, Yafang Shao <laoar.shao@xxxxxxxxx> wrote: > >> > >While investigating a kcompactd 100% CPU utilization issue in production, I > >> > >observed frequent costly high-order (order-6) page allocations triggered by > >> > >proc file reads from monitoring tools. This can be reproduced with a simple > >> > >test case: > >> > > > >> > > fd = open(PROC_FILE, O_RDONLY); > >> > > size = read(fd, buff, 256KB); > >> > > close(fd); > >> > > > >> > >Although we should modify the monitoring tools to use smaller buffer sizes, > >> > >we should also enhance the kernel to prevent these expensive high-order > >> > >allocations. > >> > > > >> > >Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx> > >> > >Cc: Josef Bacik <josef@xxxxxxxxxxxxxx> > >> > >--- > >> > > fs/proc/proc_sysctl.c | 10 +++++++++- > >> > > 1 file changed, 9 insertions(+), 1 deletion(-) > >> > > > >> > >diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > >> > >index cc9d74a06ff0..c53ba733bda5 100644 > >> > >--- a/fs/proc/proc_sysctl.c > >> > >+++ b/fs/proc/proc_sysctl.c > >> > >@@ -581,7 +581,15 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter, > >> > > error = -ENOMEM; > >> > > if (count >= KMALLOC_MAX_SIZE) > >> > > goto out; > >> > >- kbuf = kvzalloc(count + 1, GFP_KERNEL); > >> > >+ > >> > >+ /* > >> > >+ * Use vmalloc if the count is too large to avoid costly high-order page > >> > >+ * allocations. > >> > >+ */ > >> > >+ if (count < (PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER)) > >> > >+ kbuf = kvzalloc(count + 1, GFP_KERNEL); > >> > > >> > Why not move this check into kvmalloc family? > >> > >> Hmm should this check really be in kvmalloc family? > > > > Modifying the existing kvmalloc functions risks performance regressions. > > Could we instead introduce a new variant like vkmalloc() (favoring > > vmalloc over kmalloc) or kvmalloc_costless()? > > We have gfp flags and kmalloc_gfp_adjust() to moderate how aggressive > kmalloc() is before the vmalloc() fallback. It does e.g.: > > if (!(flags & __GFP_RETRY_MAYFAIL)) > flags |= __GFP_NORETRY; > > However if your problem is kcompactd utilization then the kmalloc() attempt > would have to avoid ___GFP_KSWAPD_RECLAIM to avoid waking up kswapd and then > kcompactd. Should we remove the flag for costly orders? Dunno. Ideally the > deferred compaction mechanism would limit the issue in the first place. Yes, triggering heavy compation for costly allocations seems to be quite bad. We have GFP_RETRY_MAYFAIL for that purpose if the caller really needs the allocation to try really hard. > The ad-hoc fixing up of a particular place (/proc files reading) or creating > a new vkmalloc() and then spreading its use as you see other places > triggering the issue seems quite suboptimal to me. Yes I absolutely agree. -- Michal Hocko SUSE Labs
