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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > ---
> > > 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
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
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
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 <[email protected]>
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
Commit-ID: 3c8b81864080b710bdce446fd8401923f26fc4d4
Gitweb: https://git.kernel.org/tip/3c8b81864080b710bdce446fd8401923f26fc4d4
Author: Yury Norov <[email protected]>
AuthorDate: Sat, 23 Jun 2018 10:35:01 +0300
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andriy Shevchenko <[email protected]>
Cc: David Ahern <[email protected]>
Cc: David Carrillo-Cisneros <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Cc: Jin Yao <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Kate Stewart <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Mike Snitzer <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Philippe Ombredanne <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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;