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 <catalin.marinas@xxxxxxx>
> ---
> 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
