Re: [PATCH] lib/genalloc: Set chunk size to real size which gen_pool managed.

[chi wu]

Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> 于2022年6月14日周二 02:14写道：
>
> On Sun, 12 Jun 2022 18:59:37 +0800 wuchi <wuchi.zero@xxxxxxxxx> wrote:
>
> > The demand size (chunk->avail > size > round_down(chunk->avail)) will
> > lead to meaningless algo calls in gen_pool_alloc_algo_owner without the
> > patch, alse move the follow code:
> >       size = nbits << order
> > out of read-side critical section.
> >
>
> Nobody has seriously worked on this code in a long time :(
>
> Please expand more on the flaw.  What are "algo calls"?  Why are they
> meaningless, etc?  What are the runtime effects of this error?
>
The following case may be far away from the real use scenario:
1. In function gen_pool_create, we get:
    pool->min_alloc_order = 6;       //(64)
    pool->algo = gen_pool_first_fit;

2. In function gen_pool_add_owner, add chunk (size = 4k + 32   ?) to
pool:                              [a]
    (
     nbits = size >> pool->min_alloc_order = (4k + 32) >> 6 = 4k >> 6;
     for the nbits bitmap, we managed the 4k size.
    )
    chunk->avail = 4k + 32;

3. after some alloc actions, the chunk state is:
    chunk->avail = 32;
    (the remain 32 Byte is out of managed)

4. In function gen_pool_alloc_algo_owner, we want to alloc size = 32
Byte, we may do the follow step:
    list_for_each_entry_rcu(.......)
        if (32 > chunk->avail)      ===> if (false)
            continue;
        .........
        start_bit = algo(chunk->bits, end_bit ....)      ===>
gen_pool_first_fit(chunk->bits, end_bit ...)           [b]

for [b], In step 4, the real avail size chunk managed is 0, but we
still call the algo to alloc... which is meaningless.
                           the final result is ok, just there are some
redundant steps.

for [a], the root reason is the memory size  %  pool->min_alloc_order
!= 0. (though I also doubt if this exists).
> > --- a/lib/genalloc.c
> > +++ b/lib/genalloc.c
> > @@ -193,6 +193,7 @@ int gen_pool_add_owner(struct gen_pool *pool, unsigned long virt, phys_addr_t ph
> >       if (unlikely(chunk == NULL))
> >               return -ENOMEM;
> >
> > +     size = nbits << pool->min_alloc_order;
>
> If we're going to do this then gen_pool_add_owner() no longer needs its
> `size' argument.
>
>
Sorry, What point did I misunderstand？
The chunk owns a bitmap itself,  and nbits is calculate from size in
gen_pool_add_owner:

unsigned long nbits = size >> pool->min_alloc_order;
......
size = nbits << pool->min_alloc_order;