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

From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Yury Norov <ynorov@caviumnetworks.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, Jiri Olsa <jolsa@redhat.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Kate Stewart <kstewart@linuxfoundation.org>,
	Matthew Wilcox <willy@infradead.org>,
	Philippe Ombredanne <pombredanne@nexb.com>,
	David Ahern <dsahern@gmail.com>,
	David Carrillo-Cisneros <davidcc@google.com>,
	Andi Kleen <ak@linux.intel.com>,
	Jin Yao <yao.jin@linux.intel.com>,
	linux-kernel@vger.kernel.org,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Mike Snitzer <snitzer@redhat.com>
Subject: Re: [PATCH 2/2] bitmap: sync tools with new bitmap allocation API
Date: Mon, 25 Jun 2018 11:12:17 -0300	[thread overview]
Message-ID: <20180625141217.GW20477@kernel.org> (raw)
In-Reply-To: <20180624213103.GA166241@dtor-ws>

Em Sun, Jun 24, 2018 at 02:31:03PM -0700, Dmitry Torokhov escreveu:
> 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.

But neverthless these should be fixed, we can't rule out having some
long lived 'perf top' like tool, etc.

I.e. when you find missing the delete/free counterpart calls to
new/alloc operations, please send fixes.

- Arnaldo

> Thanks.
> 
> > memory_node__read();
> > 
> > Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> > ---
> >  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