On Mon, Jul 04, 2022 at 01:56:00PM +0800, Feng Tang wrote:
> On Sun, Jul 03, 2022 at 02:17:37PM +0000, Hyeonggon Yoo wrote:
> > On Fri, Jul 01, 2022 at 11:04:51PM +0800, Feng Tang wrote:
> > > Hi Christoph,
> > >
> > > On Fri, Jul 01, 2022 at 04:37:00PM +0200, Christoph Lameter wrote:
> > > > On Fri, 1 Jul 2022, Feng Tang wrote:
> > > >
> > > > > static void *__slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> > > > > - unsigned long addr, struct kmem_cache_cpu *c)
> > > > > + unsigned long addr, struct kmem_cache_cpu *c, unsigned int orig_size)
> > > > > {
> > > >
> > > > It would be good to avoid expanding the basic slab handling functions for
> > > > kmalloc. Can we restrict the mods to the kmalloc related functions?
> > >
> > > Yes, this is the part that concerned me. I tried but haven't figured
> > > a way.
> > >
> > > I started implemting it several month ago, and stuck with several
> > > kmalloc APIs in a hacky way like dump_stack() when there is a waste
> > > over 1/4 of the object_size of the kmalloc_caches[][].
> > >
> > > Then I found one central API which has all the needed info (object_size &
> > > orig_size) that we can yell about the waste :
> > >
> > > static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_lru *lru,
> > > gfp_t gfpflags, int node, unsigned long addr, size_t orig_size)
> > >
> > > which I thought could be still hacky, as the existing 'alloc_traces'
> > > can't be resued which already has the count/call-stack info. Current
> > > solution leverage it at the cost of adding 'orig_size' parameters, but
> > > I don't know how to pass the 'waste' info through as track/location is
> > > in the lowest level.
> >
> > If adding cost of orig_size parameter for non-debugging case is concern,
> > what about doing this in userspace script that makes use of kmalloc
> > tracepoints?
> >
> > kmalloc: call_site=tty_buffer_alloc+0x43/0x90 ptr=00000000b78761e1
> > bytes_req=1056 bytes_alloc=2048 gfp_flags=GFP_ATOMIC|__GFP_NOWARN
> > accounted=false
> >
> > calculating sum of (bytes_alloc - bytes_req) for each call_site
> > may be an alternative solution.
>
> Yes, this is doable, and it will met some of the problems I met before,
> one is there are currently 2 alloc path: kmalloc and kmalloc_node, also
> we need to consider the free problem to calculate the real waste, and
> the free trace point doesn't have size info (Yes, we could compare
> the pointer with alloc path, and the user script may need to be more
> complexer). That's why I love the current 'alloc_traces' interface,
> which has the count (slove the free counting problem) and full call
> stack info.
Understood.
> And for the extra parameter cost issue, I rethink about it, and we
> can leverage the 'slab_alloc_node()' to solve it, and the patch is
> much simpler now without adding a new parameter:
>
> ---
> diff --git a/mm/slub.c b/mm/slub.c
> index b1281b8654bd3..ce4568dbb0f2d 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -271,6 +271,7 @@ struct track {
> #endif
> int cpu; /* Was running on cpu */
> int pid; /* Pid context */
> + unsigned long waste; /* memory waste for a kmalloc-ed object */
> unsigned long when; /* When did the operation occur */
> };
>
> @@ -3240,6 +3241,16 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_l
> init = slab_want_init_on_alloc(gfpflags, s);
>
> out:
> +
> +#ifdef CONFIG_SLUB_DEBUG
> + if (object && s->object_size != orig_size) {
> + struct track *track;
> +
> + track = get_track(s, object, TRACK_ALLOC);
> + track->waste = s->object_size - orig_size;
> + }
> +#endif
> +
This scares me. It does not check if the cache has
SLAB_STORE_USER flag.
Also CONFIG_SLUB_DEBUG is enabled by default, which means that
it is still against not affecting non-debugging case.
I like v1 more than modified version.
Thanks,
Hyeonggon
> slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init);
>
> return object;
> @@ -5092,6 +5103,7 @@ struct location {
> depot_stack_handle_t handle;
> unsigned long count;
> unsigned long addr;
> + unsigned long waste;
> long long sum_time;
> long min_time;
> long max_time;
> @@ -5142,7 +5154,7 @@ static int add_location(struct loc_track *t, struct kmem_cache *s,
> {
> long start, end, pos;
> struct location *l;
> - unsigned long caddr, chandle;
> + unsigned long caddr, chandle, cwaste;
> unsigned long age = jiffies - track->when;
> depot_stack_handle_t handle = 0;
>
> @@ -5162,11 +5174,13 @@ static int add_location(struct loc_track *t, struct kmem_cache *s,
> if (pos == end)
> break;
>
> - caddr = t->loc[pos].addr;
> - chandle = t->loc[pos].handle;
> - if ((track->addr == caddr) && (handle == chandle)) {
> + l = &t->loc[pos];
> + caddr = l->addr;
> + chandle = l->handle;
> + cwaste = l->waste;
> + if ((track->addr == caddr) && (handle == chandle) &&
> + (track->waste == cwaste)) {
>
> - l = &t->loc[pos];
> l->count++;
> if (track->when) {
> l->sum_time += age;
> @@ -5191,6 +5205,9 @@ static int add_location(struct loc_track *t, struct kmem_cache *s,
> end = pos;
> else if (track->addr == caddr && handle < chandle)
> end = pos;
> + else if (track->addr == caddr && handle == chandle &&
> + track->waste < cwaste)
> + end = pos;
> else
> start = pos;
> }
> @@ -5214,6 +5231,7 @@ static int add_location(struct loc_track *t, struct kmem_cache *s,
> l->min_pid = track->pid;
> l->max_pid = track->pid;
> l->handle = handle;
> + l->waste = track->waste;
> cpumask_clear(to_cpumask(l->cpus));
> cpumask_set_cpu(track->cpu, to_cpumask(l->cpus));
> nodes_clear(l->nodes);
> @@ -6102,6 +6120,10 @@ static int slab_debugfs_show(struct seq_file *seq, void *v)
> else
> seq_puts(seq, "<not-available>");
>
> + if (l->waste)
> + seq_printf(seq, " waste=%lu/%lu",
> + l->count * l->waste, l->waste);
> +
> if (l->sum_time != l->min_time) {
> seq_printf(seq, " age=%ld/%llu/%ld",
> l->min_time, div_u64(l->sum_time, l->count),
>
> Thanks,
> Feng
>
> > Thanks,
> > Hyeonggon
> >
> > > Thanks,
> > > Feng
> > >
> > >
> > >
