From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DC5F2C433FE for ; Wed, 26 Oct 2022 06:50:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=sTR1xXRMbEWfcmAzP0eLIqVRUR8XjSS5GdYjAvUJW/Q=; b=lqGidfc6Z+MP8g pCu5Q/r7GpNmWAv9DObPXTzssXoaymEHW7uZ1TCMcSr47ZlOQYzBiVifU4pkljMfC6rk53mhKrUcq Je7p45LRufbqL75bEkdQvOZwgnZn1sIdaFfqkcmRTs9gzDHMTt2EJs1oTakFtnXu+dvvSt257hSim tWpEtVSlMciMY0WR+N6LDbu/rYgwXL54JekqWpEqHI6gspicAu9KnjUk1fYikTQhJa2QSJg4arFAS hrnDALLjLdQSZwxVc5+1Y933WDk+KT0vj0bPO1zNWvsPhSVfqn5x8z+khf2wGU30uB1au+hwN+DNz Nk9TWSFYuVDX7Mgl9QRw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1onaE4-008K4u-Pl; Wed, 26 Oct 2022 06:49:20 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1onaE0-008K4C-IV for linux-arm-kernel@lists.infradead.org; Wed, 26 Oct 2022 06:49:18 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 5E06661D1D; Wed, 26 Oct 2022 06:49:15 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3E0E2C433D7; Wed, 26 Oct 2022 06:49:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1666766954; bh=Kub/hD2VgjvGeqvCE2TkXguMs8yCjAWN2wriR9J+Zvg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gKeUd8DZbBRKRQx9Q1pOwqTMr6gpF2VQ6ZZ6dKvUtTSaeQSdZ5iAEd8YCzGHqbZpn PduOQnwmDL9+kT//XFi8t9zp7E07g7XHM3mxpUGrXAqr4ry5hNoBpgJnfESR3GhnH2 yCnkOE1/QhveoH74aTHSxLbRbMeFEGpQ4wMWpPjE= Date: Wed, 26 Oct 2022 08:50:07 +0200 From: Greg Kroah-Hartman To: Catalin Marinas Cc: Linus Torvalds , Arnd Bergmann , Will Deacon , Marc Zyngier , Andrew Morton , Herbert Xu , Ard Biesheuvel , Christoph Hellwig , Isaac Manjarres , Saravana Kannan , linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 2/2] treewide: Add the __GFP_PACKED flag to several non-DMA kmalloc() allocations Message-ID: References: <20221025205247.3264568-1-catalin.marinas@arm.com> <20221025205247.3264568-3-catalin.marinas@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20221025205247.3264568-3-catalin.marinas@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221025_234916_708724_CB0128C1 X-CRM114-Status: GOOD ( 30.36 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Oct 25, 2022 at 09:52:47PM +0100, Catalin Marinas wrote: > Using the ftrace kmalloc histogram shows several hot spots for small > memory allocations that would benefit from the smaller > KMALLOC_PACKED_ALIGN alignment. > > To set up the ftrace events for small kmalloc() allocations (only > showing those not already having the __GFP_PACKED flag): > > echo 'hist:key=call_site.sym-offset:vals=bytes_req,bytes_alloc:sort=bytes_alloc.descending if bytes_req<=96 && !(gfp_flags&0x8000000)' \ > > /sys/kernel/debug/tracing/events/kmem/kmalloc/trigger > > To enable tracing of boot-time allocations, use the following > bootconfig: > > ftrace.event.kmem.kmalloc.hist { > keys = call_site.sym-offset > values = bytes_req, bytes_alloc > sort = bytes_alloc.descending > filter = "bytes_req <= 96 && !(gfp_flags & 0x8000000)" > } > > To view the allocation hot spots: > > head /sys/kernel/debug/tracing/events/kmem/kmalloc/hist > > Signed-off-by: Catalin Marinas > --- > drivers/usb/core/message.c | 3 ++- > fs/binfmt_elf.c | 6 ++++-- > fs/dcache.c | 3 ++- > fs/ext4/dir.c | 4 ++-- > fs/ext4/extents.c | 4 ++-- > fs/file.c | 2 +- > fs/kernfs/file.c | 8 ++++---- > fs/nfs/dir.c | 7 ++++--- > fs/nfs/inode.c | 2 +- > fs/nfs/nfs4state.c | 2 +- > fs/nfs/write.c | 3 ++- > fs/notify/inotify/inotify_fsnotify.c | 3 ++- > fs/proc/self.c | 2 +- > fs/seq_file.c | 5 +++-- > include/acpi/platform/aclinuxex.h | 6 ++++-- > kernel/trace/ftrace.c | 2 +- > kernel/trace/tracing_map.c | 2 +- > lib/kasprintf.c | 2 +- > lib/kobject.c | 4 ++-- > lib/mpi/mpiutil.c | 2 +- > mm/list_lru.c | 6 ++++-- > mm/memcontrol.c | 4 ++-- > mm/util.c | 6 +++--- > mm/vmalloc.c | 6 ++++-- > net/sunrpc/auth_unix.c | 2 +- > net/sunrpc/xdr.c | 2 +- > security/apparmor/lsm.c | 2 +- > security/security.c | 4 ++-- > security/tomoyo/realpath.c | 2 +- > 29 files changed, 60 insertions(+), 46 deletions(-) > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c > index 4d59d927ae3e..bff8901dc426 100644 > --- a/drivers/usb/core/message.c > +++ b/drivers/usb/core/message.c > @@ -525,7 +525,8 @@ int usb_sg_init(struct usb_sg_request *io, struct usb_device *dev, > } > > /* initialize all the urbs we'll use */ > - io->urbs = kmalloc_array(io->entries, sizeof(*io->urbs), mem_flags); > + io->urbs = kmalloc_array(io->entries, sizeof(*io->urbs), > + mem_flags | __GFP_PACKED); Hey look, you just did what I was worried about :( Oh wait, no, this is just the urb structure, not the actual data pointer sent on the urb. Ick, this is going to get really hard to review over time. I feel for the need to want to start to pack things in smaller, but this is going to be really really hard for maintainers to review submissions. Especially if the only way problems show up is in random platforms where the alignment causes a failure. Anyway we can make all arches fail if we submit pointers that are not aligned properly to the DMA engines? > if (!io->urbs) > goto nomem; > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 63c7ebb0da89..e5ad1a3244fb 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -883,7 +883,8 @@ static int load_elf_binary(struct linux_binprm *bprm) > goto out_free_ph; > > retval = -ENOMEM; > - elf_interpreter = kmalloc(elf_ppnt->p_filesz, GFP_KERNEL); > + elf_interpreter = kmalloc(elf_ppnt->p_filesz, > + GFP_KERNEL | __GFP_PACKED); If this is going to now be sprinkled all over the tree, why not make it a "real" flag, "GFP_PACKED"? Or better yet, have it describe the allocation better, "GFP_TINY" or "GFP_I_KNOW_THIS_IS_TINY_WITH_NO_DMA_ISSUES" > --- a/lib/kasprintf.c > +++ b/lib/kasprintf.c > @@ -22,7 +22,7 @@ char *kvasprintf(gfp_t gfp, const char *fmt, va_list ap) > first = vsnprintf(NULL, 0, fmt, aq); > va_end(aq); > > - p = kmalloc_track_caller(first+1, gfp); > + p = kmalloc_track_caller(first+1, gfp | __GFP_PACKED); How do we know this is going to be small? > if (!p) > return NULL; > > diff --git a/lib/kobject.c b/lib/kobject.c > index a0b2dbfcfa23..2c4acb36925d 100644 > --- a/lib/kobject.c > +++ b/lib/kobject.c > @@ -144,7 +144,7 @@ char *kobject_get_path(struct kobject *kobj, gfp_t gfp_mask) > len = get_kobj_path_length(kobj); > if (len == 0) > return NULL; > - path = kzalloc(len, gfp_mask); > + path = kzalloc(len, gfp_mask | __GFP_PACKED); This might not be small, and it's going to be very very short-lived (within a single function call), why does it need to be allocated this way? > if (!path) > return NULL; > fill_kobj_path(kobj, path, len); > @@ -749,7 +749,7 @@ static struct kobject *kobject_create(void) > { > struct kobject *kobj; > > - kobj = kzalloc(sizeof(*kobj), GFP_KERNEL); > + kobj = kzalloc(sizeof(*kobj), GFP_KERNEL | __GFP_PACKED); This might make sense, but what type of size are you wanting to see these packed allocations require? This is not all that tiny, but I guess it falls under the 96 bytes? Where did that number come from? thanks, greg k-h _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel