Re: [PATCH] tlsf: Add tracking of added tlsf memory pools

[David Dgien <dgienda125@gmail.com>]

On Wed, Apr 2, 2025 at 5:07 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> Hi David,
>
> On Fri, Mar 28, 2025 at 11:13:40PM -0400, David Dgien via B4 Relay wrote:
> > From: David Dgien <dgienda125@gmail.com>
> >
> > When configured to use the TLSF allocator, the malloc_stats function
> > only walks the initial memory pool, as that is the only pool it is aware
> > of, and TLSF doesn't link pools together in a way that the walker can
> > follow.
> >
> > Add wrapper functions around tlsf_add_pool and tlsf_remove_pool to add
> > additional tracking of added pools, so that they can be walked by the
> > malloc_stats function and meminfo can report accurate heap utilization.
> >
> > Signed-off-by: David Dgien <dgienda125@gmail.com>
> > Reviewed-by: Tyler Reece <mtreece@acm.org>
> > ---
> > For this change I put the wrapper function prototypes in malloc.h, gated
> > by CONFIG_MALLOC_TLSF. I was split between there and memory.h, but I'm
> > not entirely convinced malloc.h was the right decision. I also didn't
> > provide stubs for when CONFIG_MALLOC_TLSF isn't enabled, as I did not
> > see any upstream code that utilizes tlsf_add_pool. If the project has
> > different preferences I can make those changes.
>
> I think it's fine the way you did it for now. We have plans to remove
> dlmalloc support in the near future which gives us an opportunity to
> rework this and make malloc_add_pool() generally available.
>
> Currently barebox only uses the initial memory. With dlmalloc removed we
> could make use of the remaining memory banks using malloc_add_pool().
>
> > ---
> >  common/tlsf_malloc.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/malloc.h     |  5 ++++
> >  2 files changed, 69 insertions(+)
> >
> > diff --git a/common/tlsf_malloc.c b/common/tlsf_malloc.c
> > index 6a90ee47199fa9e8223f843cc95f52eebfec2aee..0e756a13d5a2665eccf8931b19dfa4e56cad15eb 100644
> > --- a/common/tlsf_malloc.c
> > +++ b/common/tlsf_malloc.c
> > @@ -12,8 +12,18 @@
> >  #include <module.h>
> >  #include <tlsf.h>
> >
> > +#include <linux/kasan.h>
> > +#include <linux/list.h>
> > +
> >  extern tlsf_t tlsf_mem_pool;
> >
> > +struct pool_entry {
> > +     pool_t pool;
> > +     struct list_head list;
> > +};
> > +
> > +static LIST_HEAD(mem_pool_list);
> > +
> >  void *malloc(size_t bytes)
> >  {
> >       void *mem;
> > @@ -75,6 +85,7 @@ static void malloc_walker(void* ptr, size_t size, int used, void *user)
> >
> >  void malloc_stats(void)
> >  {
> > +     struct pool_entry *cur_pool;
> >       struct malloc_stats s;
> >
> >       s.used = 0;
> > @@ -82,5 +93,58 @@ void malloc_stats(void)
> >
> >       tlsf_walk_pool(tlsf_get_pool(tlsf_mem_pool), malloc_walker, &s);
> >
> > +     list_for_each_entry(cur_pool, &mem_pool_list, list)
> > +             tlsf_walk_pool(cur_pool->pool, malloc_walker, &s);
>
> Can you create a pool_entry for the initial malloc area as well so that
> the list contains all pools?

I should be able to do that.

Since the initial memory pool would never be able to be removed, I
thought it would be better to not put it in the list at all. But if
I'm removing the ability to remove pools, then I can add the initial
malloc area to the list and simplify things.

>
> > +
> >       printf("used: %zu\nfree: %zu\n", s.used, s.free);
> >  }
> > +
> > +void *malloc_add_pool(void *mem, size_t bytes)
> > +{
> > +     pool_t new_pool;
> > +     struct pool_entry *new_pool_entry;
> > +
> > +     if (!mem)
> > +             return NULL;
> > +
> > +     new_pool_entry = malloc(sizeof(struct pool_entry));
> > +     if (!new_pool_entry)
> > +             return NULL;
> > +
> > +     new_pool = tlsf_add_pool(tlsf_mem_pool, mem, bytes);
> > +     if (!new_pool) {
> > +             free(new_pool_entry);
> > +             return NULL;
> > +     }
> > +
> > +     kasan_poison_shadow(mem, bytes, KASAN_TAG_INVALID);
> > +
> > +     new_pool_entry->pool = new_pool;
> > +     list_add(&new_pool_entry->list, &mem_pool_list);
> > +
> > +     return (void *)new_pool;
> > +}
> > +
> > +int malloc_remove_pool(void *pool)
> > +{
> > +     struct pool_entry *cur_pool;
> > +     struct malloc_stats s;
> > +
> > +     s.used = 0;
> > +     s.free = 0;
> > +
> > +     list_for_each_entry(cur_pool, &mem_pool_list, list) {
> > +             if (cur_pool->pool == (pool_t)pool) {
> > +                     tlsf_walk_pool(cur_pool->pool, malloc_walker, &s);
> > +                     if (s.used)
> > +                             return -EBUSY;
>
> Do we need this function at all? We have no way to move allocations away
> from the pool being removed, so there's no way to make this function
> work reliably. What's the point in having it?

The intent was to match the symmetry of the TLSF add/remove functions.
I will remove it from the next version, assuming removing unused pools
is not a useful feature.

-- David Dgien

>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |