Re: [PATCH 2/2] bitmap: sync tools with new bitmap allocation API

[Dmitry Torokhov]

On Sat, Jun 23, 2018 at 10:35:02AM +0300, Yury Norov wrote:
> On top of next-20180622 and Andy Shevchenko series:
> https://lkml.org/lkml/2018/6/18/841
> 
> The series mentioned above introduces helpers for bitmap allocation.
> tools/ has its own bitmap_alloc() which differs from bitmap_alloc()
> proposed in new kernel API, and is equivalent to bitmap_zalloc().
> In this series tools is switched to new API.
> 
> This is RFC because I didn't find counterpart free() call to some
> bitmap_zalloc()'s. So I didn't convert them to bitmap_free(). Could
> someone point me out? The functions are:
> setup_nodes();
> do_read_bitmap(); // Free is called, but only in fail path.

Yes, because if we succeed we effectively return allocated bitmap to the
caller. You'd need to trace upwards and see how it all gets cleaned up.
But given that this is userspace and is not expected to be long-lived,
maybe nobody bothered freeing memory and we instead rely on the kernel
to clean it all up when process terminates.

Thanks.

> memory_node__read();
> 
> Signed-off-by: Yury Norov <ynorov@xxxxxxxxxxxxxxxxxx>
> ---
>  tools/include/linux/bitmap.h | 19 +++++++++++++++----
>  tools/perf/builtin-c2c.c     | 10 +++++-----
>  tools/perf/tests/bitmap.c    |  4 ++--
>  tools/perf/tests/mem2node.c  |  4 ++--
>  tools/perf/util/header.c     |  6 +++---
>  5 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/include/linux/bitmap.h b/tools/include/linux/bitmap.h
> index 48c208437bbd..b9b85b94c937 100644
> --- a/tools/include/linux/bitmap.h
> +++ b/tools/include/linux/bitmap.h
> @@ -98,12 +98,23 @@ static inline int test_and_set_bit(int nr, unsigned long *addr)
>  }
>  
>  /**
> - * bitmap_alloc - Allocate bitmap
> - * @nbits: Number of bits
> + * Allocation and deallocation of bitmap.
>   */
> -static inline unsigned long *bitmap_alloc(int nbits)
> +static inline unsigned long *bitmap_alloc(unsigned int nbits, gfp_t flags)

This makes absolutely no sense for userspace API. What gfp_t even means
here?

If you want to introduce bitmap_zalloc and bitmap_free it is fine but
adding dummy parameters to match kernel API exactly is a folly.

Thanks.

-- 
Dmitry