linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] perf: drop unneeded bitmap_zero() in util/header.c
@ 2018-06-23  7:35 Yury Norov
  2018-06-23  7:35 ` [PATCH 2/2] bitmap: sync tools with new bitmap allocation API Yury Norov
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Yury Norov @ 2018-06-23  7:35 UTC (permalink / raw)
  To: Alexander Shishkin, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Kate Stewart,
	Matthew Wilcox, Philippe Ombredanne, David Ahern,
	David Carrillo-Cisneros, Andi Kleen, Jin Yao, linux-kernel
  Cc: Yury Norov, Andy Shevchenko, Dmitry Torokhov, Andrew Morton,
	Mike Snitzer

On top of next-20180622.

bitmap_zero() is called after bitmap_alloc() in perf code. But
bitmap_alloc() internally uses calloc() which guarantees that allocated
area is zeroed. So following bitmap_zero is unneeded. Drop it.

This happened because of confusing name for bitmap allocator. It
should has name bitmap_zalloc instead of bitmap_alloc. This series:
https://lkml.org/lkml/2018/6/18/841
introduces new API for bitmap allocations in kernel, and functions
there are named correctly. Following patch propogates the API to tools,
and fixes naming issue.

Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
---
 tools/perf/tests/bitmap.c   | 2 --
 tools/perf/tests/mem2node.c | 5 +----
 tools/perf/util/header.c    | 3 ---
 3 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/tools/perf/tests/bitmap.c b/tools/perf/tests/bitmap.c
index 47bedf25ba69..96e7fc1ad3f9 100644
--- a/tools/perf/tests/bitmap.c
+++ b/tools/perf/tests/bitmap.c
@@ -16,8 +16,6 @@ static unsigned long *get_bitmap(const char *str, int nbits)
 	bm = bitmap_alloc(nbits);
 
 	if (map && bm) {
-		bitmap_zero(bm, nbits);
-
 		for (i = 0; i < map->nr; i++)
 			set_bit(map->map[i], bm);
 	}
diff --git a/tools/perf/tests/mem2node.c b/tools/perf/tests/mem2node.c
index 0c3c87f86e03..d8e3d49d3638 100644
--- a/tools/perf/tests/mem2node.c
+++ b/tools/perf/tests/mem2node.c
@@ -24,11 +24,8 @@ static unsigned long *get_bitmap(const char *str, int nbits)
 	bm = bitmap_alloc(nbits);
 
 	if (map && bm) {
-		bitmap_zero(bm, nbits);
-
-		for (i = 0; i < map->nr; i++) {
+		for (i = 0; i < map->nr; i++)
 			set_bit(map->map[i], bm);
-		}
 	}
 
 	if (map)
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 540cd2dcd3e7..3a6bec22baa3 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -279,8 +279,6 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
 	if (!set)
 		return -ENOMEM;
 
-	bitmap_zero(set, size);
-
 	p = (u64 *) set;
 
 	for (i = 0; (u64) i < BITS_TO_U64(size); i++) {
@@ -1285,7 +1283,6 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
 		return -ENOMEM;
 	}
 
-	bitmap_zero(n->set, size);
 	n->node = idx;
 	n->size = size;
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/2] bitmap: sync tools with new bitmap allocation API
  2018-06-23  7:35 [PATCH 1/2] perf: drop unneeded bitmap_zero() in util/header.c Yury Norov
@ 2018-06-23  7:35 ` Yury Norov
  2018-06-24 21:31   ` Dmitry Torokhov
                     ` (2 more replies)
  2018-07-24 20:26 ` [PATCH 1/2] perf: drop unneeded bitmap_zero() in util/header.c Yury Norov
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 13+ messages in thread
From: Yury Norov @ 2018-06-23  7:35 UTC (permalink / raw)
  To: Alexander Shishkin, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Kate Stewart,
	Matthew Wilcox, Philippe Ombredanne, David Ahern,
	David Carrillo-Cisneros, Andi Kleen, Jin Yao, linux-kernel
  Cc: Yury Norov, Andy Shevchenko, Dmitry Torokhov, Andrew Morton,
	Mike Snitzer

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.
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)
 {
-	return calloc(1, BITS_TO_LONGS(nbits) * sizeof(unsigned long));
+	(void) flags;
+	return malloc(BITS_TO_LONGS(nbits) * sizeof(unsigned long));
+}
+
+static inline unsigned long *bitmap_zalloc(unsigned int nbits, gfp_t flags)
+{
+	(void) flags;
+	return calloc(BITS_TO_LONGS(nbits), sizeof(unsigned long));
+}
+
+static inline void bitmap_free(const unsigned long *bitmap)
+{
+	free((unsigned long *)bitmap);
 }
 
 /*
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 6a8738f7ead3..d747e98bc37d 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -127,11 +127,11 @@ static void *c2c_he_zalloc(size_t size)
 	if (!c2c_he)
 		return NULL;
 
-	c2c_he->cpuset = bitmap_alloc(c2c.cpus_cnt);
+	c2c_he->cpuset = bitmap_zalloc(c2c.cpus_cnt, 0);
 	if (!c2c_he->cpuset)
 		return NULL;
 
-	c2c_he->nodeset = bitmap_alloc(c2c.nodes_cnt);
+	c2c_he->nodeset = bitmap_zalloc(c2c.nodes_cnt, 0);
 	if (!c2c_he->nodeset)
 		return NULL;
 
@@ -156,8 +156,8 @@ static void c2c_he_free(void *he)
 		free(c2c_he->hists);
 	}
 
-	free(c2c_he->cpuset);
-	free(c2c_he->nodeset);
+	bitmap_free(c2c_he->cpuset);
+	bitmap_free(c2c_he->nodeset);
 	free(c2c_he->nodestr);
 	free(c2c_he->node_stats);
 	free(c2c_he);
@@ -2051,7 +2051,7 @@ static int setup_nodes(struct perf_session *session)
 		struct cpu_map *map = n[node].map;
 		unsigned long *set;
 
-		set = bitmap_alloc(c2c.cpus_cnt);
+		set = bitmap_zalloc(c2c.cpus_cnt, 0);
 		if (!set)
 			return -ENOMEM;
 
diff --git a/tools/perf/tests/bitmap.c b/tools/perf/tests/bitmap.c
index 96e7fc1ad3f9..a35d44ad54bc 100644
--- a/tools/perf/tests/bitmap.c
+++ b/tools/perf/tests/bitmap.c
@@ -13,7 +13,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
 	unsigned long *bm = NULL;
 	int i;
 
-	bm = bitmap_alloc(nbits);
+	bm = bitmap_zalloc(nbits, 0);
 
 	if (map && bm) {
 		for (i = 0; i < map->nr; i++)
@@ -35,7 +35,7 @@ static int test_bitmap(const char *str)
 	pr_debug("bitmap: %s\n", buf);
 
 	ret = !strcmp(buf, str);
-	free(bm);
+	bitmap_free(bm);
 	return ret;
 }
 
diff --git a/tools/perf/tests/mem2node.c b/tools/perf/tests/mem2node.c
index d8e3d49d3638..81a9b05dc632 100644
--- a/tools/perf/tests/mem2node.c
+++ b/tools/perf/tests/mem2node.c
@@ -21,7 +21,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
 	unsigned long *bm = NULL;
 	int i;
 
-	bm = bitmap_alloc(nbits);
+	bm = bitmap_zalloc(nbits, 0);
 
 	if (map && bm) {
 		for (i = 0; i < map->nr; i++)
@@ -65,7 +65,7 @@ int test__mem2node(struct test *t __maybe_unused, int subtest __maybe_unused)
 	T("failed: mem2node__node", -1 == mem2node__node(&map, 0x1050));
 
 	for (i = 0; i < ARRAY_SIZE(nodes); i++)
-		free(nodes[i].set);
+		bitmap_free(nodes[i].set);
 
 	mem2node__exit(&map);
 	return 0;
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 3a6bec22baa3..201c91db95df 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -275,7 +275,7 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
 	if (ret)
 		return ret;
 
-	set = bitmap_alloc(size);
+	set = bitmap_zalloc(size, 0);
 	if (!set)
 		return -ENOMEM;
 
@@ -284,7 +284,7 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
 	for (i = 0; (u64) i < BITS_TO_U64(size); i++) {
 		ret = do_read_u64(ff, p + i);
 		if (ret < 0) {
-			free(set);
+			bitmap_free(set);
 			return ret;
 		}
 	}
@@ -1277,7 +1277,7 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
 
 	size++;
 
-	n->set = bitmap_alloc(size);
+	n->set = bitmap_zalloc(size, 0);
 	if (!n->set) {
 		closedir(dir);
 		return -ENOMEM;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] bitmap: sync tools with new bitmap allocation API
  2018-06-23  7:35 ` [PATCH 2/2] bitmap: sync tools with new bitmap allocation API Yury Norov
@ 2018-06-24 21:31   ` Dmitry Torokhov
  2018-06-24 22:45     ` Yury Norov
  2018-06-25 14:12     ` Arnaldo Carvalho de Melo
  2018-07-04 17:30   ` Jiri Olsa
  2018-07-04 22:15   ` [PATCH v2 " Yury Norov
  2 siblings, 2 replies; 13+ messages in thread
From: Dmitry Torokhov @ 2018-06-24 21:31 UTC (permalink / raw)
  To: Yury Norov
  Cc: Alexander Shishkin, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Kate Stewart,
	Matthew Wilcox, Philippe Ombredanne, David Ahern,
	David Carrillo-Cisneros, Andi Kleen, Jin Yao, linux-kernel,
	Andy Shevchenko, Andrew Morton, Mike Snitzer

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@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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] bitmap: sync tools with new bitmap allocation API
  2018-06-24 21:31   ` Dmitry Torokhov
@ 2018-06-24 22:45     ` Yury Norov
  2018-07-04 15:36       ` Dmitry Torokhov
  2018-06-25 14:12     ` Arnaldo Carvalho de Melo
  1 sibling, 1 reply; 13+ messages in thread
From: Yury Norov @ 2018-06-24 22:45 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Alexander Shishkin, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Kate Stewart,
	Matthew Wilcox, Philippe Ombredanne, David Ahern,
	David Carrillo-Cisneros, Andi Kleen, Jin Yao, linux-kernel,
	Andy Shevchenko, Andrew Morton, Mike Snitzer

On Sun, Jun 24, 2018 at 02:31:03PM -0700, Dmitry Torokhov wrote:
> External Email
> 
> 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@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.

Identical API makes easier porting the code from kernel to tools.
Refer for example declaration of kmalloc in:
tools/testing/radix-tree/linux.c
tools/testing/scatterlist/linux/mm.h
tools/virtio/linux/kernel.h
tools/virtio/ringtest/ptr_ring.c

Yury

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] bitmap: sync tools with new bitmap allocation API
  2018-06-24 21:31   ` Dmitry Torokhov
  2018-06-24 22:45     ` Yury Norov
@ 2018-06-25 14:12     ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-06-25 14:12 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Yury Norov, Alexander Shishkin, Peter Zijlstra, Ingo Molnar,
	Jiri Olsa, Namhyung Kim, Kate Stewart, Matthew Wilcox,
	Philippe Ombredanne, David Ahern, David Carrillo-Cisneros,
	Andi Kleen, Jin Yao, linux-kernel, Andy Shevchenko,
	Andrew Morton, Mike Snitzer

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] bitmap: sync tools with new bitmap allocation API
  2018-06-24 22:45     ` Yury Norov
@ 2018-07-04 15:36       ` Dmitry Torokhov
  2018-07-04 17:24         ` Jiri Olsa
  0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2018-07-04 15:36 UTC (permalink / raw)
  To: Yury Norov
  Cc: Alexander Shishkin, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Kate Stewart,
	Matthew Wilcox, Philippe Ombredanne, David Ahern,
	David Carrillo-Cisneros, Andi Kleen, Jin Yao, linux-kernel,
	Andy Shevchenko, Andrew Morton, Mike Snitzer

On Mon, Jun 25, 2018 at 01:45:22AM +0300, Yury Norov wrote:
> On Sun, Jun 24, 2018 at 02:31:03PM -0700, Dmitry Torokhov wrote:
> > External Email
> > 
> > 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@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.
> 
> Identical API makes easier porting the code from kernel to tools.
> Refer for example declaration of kmalloc in:
> tools/testing/radix-tree/linux.c
> tools/testing/scatterlist/linux/mm.h
> tools/virtio/linux/kernel.h
> tools/virtio/ringtest/ptr_ring.c

These are unittests for the APIs in question, of course they would have
to match exactly.

perf tool however is not a unittest, so there is no need to match kernel
API.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] bitmap: sync tools with new bitmap allocation API
  2018-07-04 15:36       ` Dmitry Torokhov
@ 2018-07-04 17:24         ` Jiri Olsa
  0 siblings, 0 replies; 13+ messages in thread
From: Jiri Olsa @ 2018-07-04 17:24 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Yury Norov, Alexander Shishkin, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Namhyung Kim, Kate Stewart,
	Matthew Wilcox, Philippe Ombredanne, David Ahern,
	David Carrillo-Cisneros, Andi Kleen, Jin Yao, linux-kernel,
	Andy Shevchenko, Andrew Morton, Mike Snitzer

On Wed, Jul 04, 2018 at 03:36:17PM +0000, Dmitry Torokhov wrote:

SNIP

> > > > 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.
> > 
> > Identical API makes easier porting the code from kernel to tools.
> > Refer for example declaration of kmalloc in:
> > tools/testing/radix-tree/linux.c
> > tools/testing/scatterlist/linux/mm.h
> > tools/virtio/linux/kernel.h
> > tools/virtio/ringtest/ptr_ring.c
> 
> These are unittests for the APIs in question, of course they would have
> to match exactly.
> 
> perf tool however is not a unittest, so there is no need to match kernel
> API.

+1, please remove that flags argument

thanks,
jirka

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] bitmap: sync tools with new bitmap allocation API
  2018-06-23  7:35 ` [PATCH 2/2] bitmap: sync tools with new bitmap allocation API Yury Norov
  2018-06-24 21:31   ` Dmitry Torokhov
@ 2018-07-04 17:30   ` Jiri Olsa
  2018-07-04 22:15   ` [PATCH v2 " Yury Norov
  2 siblings, 0 replies; 13+ messages in thread
From: Jiri Olsa @ 2018-07-04 17:30 UTC (permalink / raw)
  To: Yury Norov
  Cc: Alexander Shishkin, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Namhyung Kim, Kate Stewart,
	Matthew Wilcox, Philippe Ombredanne, David Ahern,
	David Carrillo-Cisneros, Andi Kleen, Jin Yao, linux-kernel,
	Andy Shevchenko, Dmitry Torokhov, Andrew Morton, Mike Snitzer

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.
> 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)
>  {
> -	return calloc(1, BITS_TO_LONGS(nbits) * sizeof(unsigned long));
> +	(void) flags;
> +	return malloc(BITS_TO_LONGS(nbits) * sizeof(unsigned long));
> +}

hum I don't see any tools/ user fo bitmap_alloc now,
but I guess we don't mind ;-)

jirka

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/2] bitmap: sync tools with new bitmap allocation API
  2018-06-23  7:35 ` [PATCH 2/2] bitmap: sync tools with new bitmap allocation API Yury Norov
  2018-06-24 21:31   ` Dmitry Torokhov
  2018-07-04 17:30   ` Jiri Olsa
@ 2018-07-04 22:15   ` Yury Norov
  2018-07-25 11:22     ` Arnaldo Carvalho de Melo
  2 siblings, 1 reply; 13+ messages in thread
From: Yury Norov @ 2018-07-04 22:15 UTC (permalink / raw)
  To: Alexander Shishkin, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Kate Stewart,
	Matthew Wilcox, Philippe Ombredanne, David Ahern,
	David Carrillo-Cisneros, Andi Kleen, Jin Yao, linux-kernel
  Cc: Andy Shevchenko, Dmitry Torokhov, Andrew Morton, Mike Snitzer,
	Yury Norov

On top of next-20180622 and Andy Shevchenko series:
https://lkml.org/lkml/2018/6/18/841

The series https://lkml.org/lkml/2018/6/18/841 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 patch tools is switched to equivalent API. Some bitmap_zalloc()s
are not paired with corresponding bitmap_free()s, so marked with FIXME tag.

Since v1:
 - removed flags parameter;
 - removed RFC tag: this is real bug where free() is not called;
 - FIXME notes added where needed.

Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
---
 tools/include/linux/bitmap.h | 17 +++++++++++++----
 tools/perf/builtin-c2c.c     | 11 ++++++-----
 tools/perf/tests/bitmap.c    |  4 ++--
 tools/perf/tests/mem2node.c  |  4 ++--
 tools/perf/util/header.c     |  8 +++++---
 5 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/tools/include/linux/bitmap.h b/tools/include/linux/bitmap.h
index 48c208437bbd..64a87921bac8 100644
--- a/tools/include/linux/bitmap.h
+++ b/tools/include/linux/bitmap.h
@@ -98,12 +98,21 @@ 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)
 {
-	return calloc(1, BITS_TO_LONGS(nbits) * sizeof(unsigned long));
+	return malloc(BITS_TO_LONGS(nbits) * sizeof(unsigned long));
+}
+
+static inline unsigned long *bitmap_zalloc(unsigned int nbits)
+{
+	return calloc(BITS_TO_LONGS(nbits), sizeof(unsigned long));
+}
+
+static inline void bitmap_free(const unsigned long *bitmap)
+{
+	free((unsigned long *)bitmap);
 }
 
 /*
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 6a8738f7ead3..584abe437154 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -127,11 +127,11 @@ static void *c2c_he_zalloc(size_t size)
 	if (!c2c_he)
 		return NULL;
 
-	c2c_he->cpuset = bitmap_alloc(c2c.cpus_cnt);
+	c2c_he->cpuset = bitmap_zalloc(c2c.cpus_cnt);
 	if (!c2c_he->cpuset)
 		return NULL;
 
-	c2c_he->nodeset = bitmap_alloc(c2c.nodes_cnt);
+	c2c_he->nodeset = bitmap_zalloc(c2c.nodes_cnt);
 	if (!c2c_he->nodeset)
 		return NULL;
 
@@ -156,8 +156,8 @@ static void c2c_he_free(void *he)
 		free(c2c_he->hists);
 	}
 
-	free(c2c_he->cpuset);
-	free(c2c_he->nodeset);
+	bitmap_free(c2c_he->cpuset);
+	bitmap_free(c2c_he->nodeset);
 	free(c2c_he->nodestr);
 	free(c2c_he->node_stats);
 	free(c2c_he);
@@ -2051,7 +2051,8 @@ static int setup_nodes(struct perf_session *session)
 		struct cpu_map *map = n[node].map;
 		unsigned long *set;
 
-		set = bitmap_alloc(c2c.cpus_cnt);
+		/* FIXME: No counterpart free() */
+		set = bitmap_zalloc(c2c.cpus_cnt);
 		if (!set)
 			return -ENOMEM;
 
diff --git a/tools/perf/tests/bitmap.c b/tools/perf/tests/bitmap.c
index 96e7fc1ad3f9..0f7491a4e0f2 100644
--- a/tools/perf/tests/bitmap.c
+++ b/tools/perf/tests/bitmap.c
@@ -13,7 +13,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
 	unsigned long *bm = NULL;
 	int i;
 
-	bm = bitmap_alloc(nbits);
+	bm = bitmap_zalloc(nbits);
 
 	if (map && bm) {
 		for (i = 0; i < map->nr; i++)
@@ -35,7 +35,7 @@ static int test_bitmap(const char *str)
 	pr_debug("bitmap: %s\n", buf);
 
 	ret = !strcmp(buf, str);
-	free(bm);
+	bitmap_free(bm);
 	return ret;
 }
 
diff --git a/tools/perf/tests/mem2node.c b/tools/perf/tests/mem2node.c
index d8e3d49d3638..27fd83bab453 100644
--- a/tools/perf/tests/mem2node.c
+++ b/tools/perf/tests/mem2node.c
@@ -21,7 +21,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
 	unsigned long *bm = NULL;
 	int i;
 
-	bm = bitmap_alloc(nbits);
+	bm = bitmap_zalloc(nbits);
 
 	if (map && bm) {
 		for (i = 0; i < map->nr; i++)
@@ -65,7 +65,7 @@ int test__mem2node(struct test *t __maybe_unused, int subtest __maybe_unused)
 	T("failed: mem2node__node", -1 == mem2node__node(&map, 0x1050));
 
 	for (i = 0; i < ARRAY_SIZE(nodes); i++)
-		free(nodes[i].set);
+		bitmap_free(nodes[i].set);
 
 	mem2node__exit(&map);
 	return 0;
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 3a6bec22baa3..8736c70ffb51 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -275,7 +275,8 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
 	if (ret)
 		return ret;
 
-	set = bitmap_alloc(size);
+	/* FIXME: No counterpart free() */
+	set = bitmap_zalloc(size);
 	if (!set)
 		return -ENOMEM;
 
@@ -284,7 +285,7 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
 	for (i = 0; (u64) i < BITS_TO_U64(size); i++) {
 		ret = do_read_u64(ff, p + i);
 		if (ret < 0) {
-			free(set);
+			bitmap_free(set);
 			return ret;
 		}
 	}
@@ -1277,7 +1278,8 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
 
 	size++;
 
-	n->set = bitmap_alloc(size);
+	/* FIXME: No counterpart free() */
+	n->set = bitmap_zalloc(size);
 	if (!n->set) {
 		closedir(dir);
 		return -ENOMEM;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] perf: drop unneeded bitmap_zero() in util/header.c
  2018-06-23  7:35 [PATCH 1/2] perf: drop unneeded bitmap_zero() in util/header.c Yury Norov
  2018-06-23  7:35 ` [PATCH 2/2] bitmap: sync tools with new bitmap allocation API Yury Norov
@ 2018-07-24 20:26 ` Yury Norov
  2018-07-25 11:20 ` Arnaldo Carvalho de Melo
  2018-08-18 11:24 ` [tip:perf/urgent] perf tools: Drop unneeded bitmap_zero() calls tip-bot for Yury Norov
  3 siblings, 0 replies; 13+ messages in thread
From: Yury Norov @ 2018-07-24 20:26 UTC (permalink / raw)
  To: Alexander Shishkin, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Kate Stewart,
	Matthew Wilcox, Philippe Ombredanne, David Ahern,
	David Carrillo-Cisneros, Andi Kleen, Jin Yao, linux-kernel
  Cc: Andy Shevchenko, Dmitry Torokhov, Andrew Morton, Mike Snitzer

On Sat, Jun 23, 2018 at 10:35:01AM +0300, Yury Norov wrote:
> On top of next-20180622.
> 
> bitmap_zero() is called after bitmap_alloc() in perf code. But
> bitmap_alloc() internally uses calloc() which guarantees that allocated
> area is zeroed. So following bitmap_zero is unneeded. Drop it.
> 
> This happened because of confusing name for bitmap allocator. It
> should has name bitmap_zalloc instead of bitmap_alloc. This series:
> https://lkml.org/lkml/2018/6/18/841
> introduces new API for bitmap allocations in kernel, and functions
> there are named correctly. Following patch propogates the API to tools,
> and fixes naming issue.
> 
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>

Ping?

> ---
>  tools/perf/tests/bitmap.c   | 2 --
>  tools/perf/tests/mem2node.c | 5 +----
>  tools/perf/util/header.c    | 3 ---
>  3 files changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/tools/perf/tests/bitmap.c b/tools/perf/tests/bitmap.c
> index 47bedf25ba69..96e7fc1ad3f9 100644
> --- a/tools/perf/tests/bitmap.c
> +++ b/tools/perf/tests/bitmap.c
> @@ -16,8 +16,6 @@ static unsigned long *get_bitmap(const char *str, int nbits)
>  	bm = bitmap_alloc(nbits);
>  
>  	if (map && bm) {
> -		bitmap_zero(bm, nbits);
> -
>  		for (i = 0; i < map->nr; i++)
>  			set_bit(map->map[i], bm);
>  	}
> diff --git a/tools/perf/tests/mem2node.c b/tools/perf/tests/mem2node.c
> index 0c3c87f86e03..d8e3d49d3638 100644
> --- a/tools/perf/tests/mem2node.c
> +++ b/tools/perf/tests/mem2node.c
> @@ -24,11 +24,8 @@ static unsigned long *get_bitmap(const char *str, int nbits)
>  	bm = bitmap_alloc(nbits);
>  
>  	if (map && bm) {
> -		bitmap_zero(bm, nbits);
> -
> -		for (i = 0; i < map->nr; i++) {
> +		for (i = 0; i < map->nr; i++)
>  			set_bit(map->map[i], bm);
> -		}
>  	}
>  
>  	if (map)
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 540cd2dcd3e7..3a6bec22baa3 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -279,8 +279,6 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
>  	if (!set)
>  		return -ENOMEM;
>  
> -	bitmap_zero(set, size);
> -
>  	p = (u64 *) set;
>  
>  	for (i = 0; (u64) i < BITS_TO_U64(size); i++) {
> @@ -1285,7 +1283,6 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
>  		return -ENOMEM;
>  	}
>  
> -	bitmap_zero(n->set, size);
>  	n->node = idx;
>  	n->size = size;
>  
> -- 
> 2.17.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] perf: drop unneeded bitmap_zero() in util/header.c
  2018-06-23  7:35 [PATCH 1/2] perf: drop unneeded bitmap_zero() in util/header.c Yury Norov
  2018-06-23  7:35 ` [PATCH 2/2] bitmap: sync tools with new bitmap allocation API Yury Norov
  2018-07-24 20:26 ` [PATCH 1/2] perf: drop unneeded bitmap_zero() in util/header.c Yury Norov
@ 2018-07-25 11:20 ` Arnaldo Carvalho de Melo
  2018-08-18 11:24 ` [tip:perf/urgent] perf tools: Drop unneeded bitmap_zero() calls tip-bot for Yury Norov
  3 siblings, 0 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-07-25 11:20 UTC (permalink / raw)
  To: Yury Norov
  Cc: Alexander Shishkin, Peter Zijlstra, Ingo Molnar, Jiri Olsa,
	Namhyung Kim, Kate Stewart, Matthew Wilcox, Philippe Ombredanne,
	David Ahern, David Carrillo-Cisneros, Andi Kleen, Jin Yao,
	linux-kernel, Andy Shevchenko, Dmitry Torokhov, Andrew Morton,
	Mike Snitzer

Em Sat, Jun 23, 2018 at 10:35:01AM +0300, Yury Norov escreveu:
> On top of next-20180622.
> 
> bitmap_zero() is called after bitmap_alloc() in perf code. But
> bitmap_alloc() internally uses calloc() which guarantees that allocated
> area is zeroed. So following bitmap_zero is unneeded. Drop it.
> 
> This happened because of confusing name for bitmap allocator. It
> should has name bitmap_zalloc instead of bitmap_alloc. This series:
> https://lkml.org/lkml/2018/6/18/841
> introduces new API for bitmap allocations in kernel, and functions
> there are named correctly. Following patch propogates the API to tools,
> and fixes naming issue.
> 
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> ---
>  tools/perf/tests/bitmap.c   | 2 --
>  tools/perf/tests/mem2node.c | 5 +----
>  tools/perf/util/header.c    | 3 ---
>  3 files changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/tools/perf/tests/bitmap.c b/tools/perf/tests/bitmap.c
> index 47bedf25ba69..96e7fc1ad3f9 100644
> --- a/tools/perf/tests/bitmap.c
> +++ b/tools/perf/tests/bitmap.c
> @@ -16,8 +16,6 @@ static unsigned long *get_bitmap(const char *str, int nbits)
>  	bm = bitmap_alloc(nbits);
>  
>  	if (map && bm) {
> -		bitmap_zero(bm, nbits);
> -
>  		for (i = 0; i < map->nr; i++)
>  			set_bit(map->map[i], bm);
>  	}
> diff --git a/tools/perf/tests/mem2node.c b/tools/perf/tests/mem2node.c
> index 0c3c87f86e03..d8e3d49d3638 100644
> --- a/tools/perf/tests/mem2node.c
> +++ b/tools/perf/tests/mem2node.c
> @@ -24,11 +24,8 @@ static unsigned long *get_bitmap(const char *str, int nbits)
>  	bm = bitmap_alloc(nbits);
>  
>  	if (map && bm) {
> -		bitmap_zero(bm, nbits);
> -
> -		for (i = 0; i < map->nr; i++) {
> +		for (i = 0; i < map->nr; i++)
>  			set_bit(map->map[i], bm);
> -		}
>  	}

Please refrain from doing unrelated changes to the purpose of the patch,
that just gets in the way of reviewing, i.e. what for removing those
braces? The patch should be just:

@@ -24,6 +24,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
 	bm = bitmap_alloc(nbits);
 
 	if (map && bm) {
-		bitmap_zero(bm, nbits);
 
 		for (i = 0; i < map->nr; i++) {

Because the point of this patch is just to remove this unneeded
bitmap_zero().

>  
>  	if (map)
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 540cd2dcd3e7..3a6bec22baa3 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -279,8 +279,6 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
>  	if (!set)
>  		return -ENOMEM;
>  
> -	bitmap_zero(set, size);
> -
>  	p = (u64 *) set;
>  
>  	for (i = 0; (u64) i < BITS_TO_U64(size); i++) {
> @@ -1285,7 +1283,6 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
>  		return -ENOMEM;
>  	}
>  
> -	bitmap_zero(n->set, size);
>  	n->node = idx;
>  	n->size = size;
>  
> -- 
> 2.17.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/2] bitmap: sync tools with new bitmap allocation API
  2018-07-04 22:15   ` [PATCH v2 " Yury Norov
@ 2018-07-25 11:22     ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 13+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-07-25 11:22 UTC (permalink / raw)
  To: Yury Norov
  Cc: Alexander Shishkin, Peter Zijlstra, Ingo Molnar, Jiri Olsa,
	Namhyung Kim, Kate Stewart, Matthew Wilcox, Philippe Ombredanne,
	David Ahern, David Carrillo-Cisneros, Andi Kleen, Jin Yao,
	linux-kernel, Andy Shevchenko, Dmitry Torokhov, Andrew Morton,
	Mike Snitzer

Em Thu, Jul 05, 2018 at 01:15:53AM +0300, Yury Norov escreveu:
> On top of next-20180622 and Andy Shevchenko series:
> https://lkml.org/lkml/2018/6/18/841
> 
> The series https://lkml.org/lkml/2018/6/18/841 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 patch tools is switched to equivalent API. Some bitmap_zalloc()s
> are not paired with corresponding bitmap_free()s, so marked with FIXME tag.
> 
> Since v1:
>  - removed flags parameter;
>  - removed RFC tag: this is real bug where free() is not called;
>  - FIXME notes added where needed.
> 
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> ---
>  tools/include/linux/bitmap.h | 17 +++++++++++++----
>  tools/perf/builtin-c2c.c     | 11 ++++++-----
>  tools/perf/tests/bitmap.c    |  4 ++--
>  tools/perf/tests/mem2node.c  |  4 ++--
>  tools/perf/util/header.c     |  8 +++++---
>  5 files changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/include/linux/bitmap.h b/tools/include/linux/bitmap.h
> index 48c208437bbd..64a87921bac8 100644
> --- a/tools/include/linux/bitmap.h
> +++ b/tools/include/linux/bitmap.h
> @@ -98,12 +98,21 @@ 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)
>  {
> -	return calloc(1, BITS_TO_LONGS(nbits) * sizeof(unsigned long));
> +	return malloc(BITS_TO_LONGS(nbits) * sizeof(unsigned long));
> +}
> +
> +static inline unsigned long *bitmap_zalloc(unsigned int nbits)
> +{
> +	return calloc(BITS_TO_LONGS(nbits), sizeof(unsigned long));
> +}
> +
> +static inline void bitmap_free(const unsigned long *bitmap)
> +{
> +	free((unsigned long *)bitmap);
>  }

So the patch should be split into at least two, one that introduces
bitmap_zalloc() and bitmap_free(),  and then another patch that converts
things to zalloc when needed, other patches adding bitmap_free() where
its missing, etc.

- Arnaldo

p.s. cleaning up inbox after vacation.

>  
>  /*
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index 6a8738f7ead3..584abe437154 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -127,11 +127,11 @@ static void *c2c_he_zalloc(size_t size)
>  	if (!c2c_he)
>  		return NULL;
>  
> -	c2c_he->cpuset = bitmap_alloc(c2c.cpus_cnt);
> +	c2c_he->cpuset = bitmap_zalloc(c2c.cpus_cnt);
>  	if (!c2c_he->cpuset)
>  		return NULL;
>  
> -	c2c_he->nodeset = bitmap_alloc(c2c.nodes_cnt);
> +	c2c_he->nodeset = bitmap_zalloc(c2c.nodes_cnt);
>  	if (!c2c_he->nodeset)
>  		return NULL;
>  
> @@ -156,8 +156,8 @@ static void c2c_he_free(void *he)
>  		free(c2c_he->hists);
>  	}
>  
> -	free(c2c_he->cpuset);
> -	free(c2c_he->nodeset);
> +	bitmap_free(c2c_he->cpuset);
> +	bitmap_free(c2c_he->nodeset);
>  	free(c2c_he->nodestr);
>  	free(c2c_he->node_stats);
>  	free(c2c_he);
> @@ -2051,7 +2051,8 @@ static int setup_nodes(struct perf_session *session)
>  		struct cpu_map *map = n[node].map;
>  		unsigned long *set;
>  
> -		set = bitmap_alloc(c2c.cpus_cnt);
> +		/* FIXME: No counterpart free() */
> +		set = bitmap_zalloc(c2c.cpus_cnt);
>  		if (!set)
>  			return -ENOMEM;
>  
> diff --git a/tools/perf/tests/bitmap.c b/tools/perf/tests/bitmap.c
> index 96e7fc1ad3f9..0f7491a4e0f2 100644
> --- a/tools/perf/tests/bitmap.c
> +++ b/tools/perf/tests/bitmap.c
> @@ -13,7 +13,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
>  	unsigned long *bm = NULL;
>  	int i;
>  
> -	bm = bitmap_alloc(nbits);
> +	bm = bitmap_zalloc(nbits);
>  
>  	if (map && bm) {
>  		for (i = 0; i < map->nr; i++)
> @@ -35,7 +35,7 @@ static int test_bitmap(const char *str)
>  	pr_debug("bitmap: %s\n", buf);
>  
>  	ret = !strcmp(buf, str);
> -	free(bm);
> +	bitmap_free(bm);
>  	return ret;
>  }
>  
> diff --git a/tools/perf/tests/mem2node.c b/tools/perf/tests/mem2node.c
> index d8e3d49d3638..27fd83bab453 100644
> --- a/tools/perf/tests/mem2node.c
> +++ b/tools/perf/tests/mem2node.c
> @@ -21,7 +21,7 @@ static unsigned long *get_bitmap(const char *str, int nbits)
>  	unsigned long *bm = NULL;
>  	int i;
>  
> -	bm = bitmap_alloc(nbits);
> +	bm = bitmap_zalloc(nbits);
>  
>  	if (map && bm) {
>  		for (i = 0; i < map->nr; i++)
> @@ -65,7 +65,7 @@ int test__mem2node(struct test *t __maybe_unused, int subtest __maybe_unused)
>  	T("failed: mem2node__node", -1 == mem2node__node(&map, 0x1050));
>  
>  	for (i = 0; i < ARRAY_SIZE(nodes); i++)
> -		free(nodes[i].set);
> +		bitmap_free(nodes[i].set);
>  
>  	mem2node__exit(&map);
>  	return 0;
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 3a6bec22baa3..8736c70ffb51 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -275,7 +275,8 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
>  	if (ret)
>  		return ret;
>  
> -	set = bitmap_alloc(size);
> +	/* FIXME: No counterpart free() */
> +	set = bitmap_zalloc(size);
>  	if (!set)
>  		return -ENOMEM;
>  
> @@ -284,7 +285,7 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
>  	for (i = 0; (u64) i < BITS_TO_U64(size); i++) {
>  		ret = do_read_u64(ff, p + i);
>  		if (ret < 0) {
> -			free(set);
> +			bitmap_free(set);
>  			return ret;
>  		}
>  	}
> @@ -1277,7 +1278,8 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
>  
>  	size++;
>  
> -	n->set = bitmap_alloc(size);
> +	/* FIXME: No counterpart free() */
> +	n->set = bitmap_zalloc(size);
>  	if (!n->set) {
>  		closedir(dir);
>  		return -ENOMEM;
> -- 
> 2.17.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [tip:perf/urgent] perf tools: Drop unneeded bitmap_zero() calls
  2018-06-23  7:35 [PATCH 1/2] perf: drop unneeded bitmap_zero() in util/header.c Yury Norov
                   ` (2 preceding siblings ...)
  2018-07-25 11:20 ` Arnaldo Carvalho de Melo
@ 2018-08-18 11:24 ` tip-bot for Yury Norov
  3 siblings, 0 replies; 13+ messages in thread
From: tip-bot for Yury Norov @ 2018-08-18 11:24 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, namhyung, akpm, tglx, mingo, acme,
	alexander.shishkin, peterz, willy, kstewart, pombredanne, ak,
	yao.jin, dsahern, andriy.shevchenko, davidcc, dmitry.torokhov,
	snitzer, hpa, ynorov, jolsa

Commit-ID:  3c8b81864080b710bdce446fd8401923f26fc4d4
Gitweb:     https://git.kernel.org/tip/3c8b81864080b710bdce446fd8401923f26fc4d4
Author:     Yury Norov <ynorov@caviumnetworks.com>
AuthorDate: Sat, 23 Jun 2018 10:35:01 +0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 8 Aug 2018 15:55:44 -0300

perf tools: Drop unneeded bitmap_zero() calls

bitmap_zero() is called after bitmap_alloc() in perf code. But
bitmap_alloc() internally uses calloc() which guarantees that allocated
area is zeroed. So following bitmap_zero is unneeded. Drop it.

This happened because of confusing name for bitmap allocator. It
should has name bitmap_zalloc instead of bitmap_alloc.

This series:

  https://lkml.org/lkml/2018/6/18/841

introduces a new API for bitmap allocations in kernel, and functions
there are named correctly. Following patch propogates the API to tools,
and fixes naming issue.

Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andriy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: David Carrillo-Cisneros <davidcc@google.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Philippe Ombredanne <pombredanne@nexb.com>
Link: http://lkml.kernel.org/r/20180623073502.16321-1-ynorov@caviumnetworks.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/tests/bitmap.c   | 2 --
 tools/perf/tests/mem2node.c | 2 --
 tools/perf/util/header.c    | 3 ---
 3 files changed, 7 deletions(-)

diff --git a/tools/perf/tests/bitmap.c b/tools/perf/tests/bitmap.c
index 47bedf25ba69..96e7fc1ad3f9 100644
--- a/tools/perf/tests/bitmap.c
+++ b/tools/perf/tests/bitmap.c
@@ -16,8 +16,6 @@ static unsigned long *get_bitmap(const char *str, int nbits)
 	bm = bitmap_alloc(nbits);
 
 	if (map && bm) {
-		bitmap_zero(bm, nbits);
-
 		for (i = 0; i < map->nr; i++)
 			set_bit(map->map[i], bm);
 	}
diff --git a/tools/perf/tests/mem2node.c b/tools/perf/tests/mem2node.c
index 0c3c87f86e03..9e9e4d37cc77 100644
--- a/tools/perf/tests/mem2node.c
+++ b/tools/perf/tests/mem2node.c
@@ -24,8 +24,6 @@ static unsigned long *get_bitmap(const char *str, int nbits)
 	bm = bitmap_alloc(nbits);
 
 	if (map && bm) {
-		bitmap_zero(bm, nbits);
-
 		for (i = 0; i < map->nr; i++) {
 			set_bit(map->map[i], bm);
 		}
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 5af58aac91ad..5f1af7b07b96 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -279,8 +279,6 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
 	if (!set)
 		return -ENOMEM;
 
-	bitmap_zero(set, size);
-
 	p = (u64 *) set;
 
 	for (i = 0; (u64) i < BITS_TO_U64(size); i++) {
@@ -1285,7 +1283,6 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
 		return -ENOMEM;
 	}
 
-	bitmap_zero(n->set, size);
 	n->node = idx;
 	n->size = size;
 

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-08-18 11:25 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-23  7:35 [PATCH 1/2] perf: drop unneeded bitmap_zero() in util/header.c Yury Norov
2018-06-23  7:35 ` [PATCH 2/2] bitmap: sync tools with new bitmap allocation API Yury Norov
2018-06-24 21:31   ` Dmitry Torokhov
2018-06-24 22:45     ` Yury Norov
2018-07-04 15:36       ` Dmitry Torokhov
2018-07-04 17:24         ` Jiri Olsa
2018-06-25 14:12     ` Arnaldo Carvalho de Melo
2018-07-04 17:30   ` Jiri Olsa
2018-07-04 22:15   ` [PATCH v2 " Yury Norov
2018-07-25 11:22     ` Arnaldo Carvalho de Melo
2018-07-24 20:26 ` [PATCH 1/2] perf: drop unneeded bitmap_zero() in util/header.c Yury Norov
2018-07-25 11:20 ` Arnaldo Carvalho de Melo
2018-08-18 11:24 ` [tip:perf/urgent] perf tools: Drop unneeded bitmap_zero() calls tip-bot for Yury Norov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).