2023-05-25 07:20:44

by Ian Rogers

Subject: [PATCH v1 01/16] perf header: Make nodes dynamic in write_mem_topology

Avoid a large static array, dynamically allocate the nodes avoiding a
hard coded limited as well.

Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/header.c | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 2dde3ca20de5..80593ed8c79b 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -24,6 +24,7 @@
#include <bpf/libbpf.h>
#endif
#include <perf/cpumap.h>
+#include <tools/libc_compat.h> // reallocarray

#include "dso.h"
#include "evlist.h"
@@ -1396,13 +1397,14 @@ static int memory_node__sort(const void *a, const void *b)
return na->node - nb->node;
}

-static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
+static int build_mem_topology(struct memory_node **nodesp, u64 *cntp)
{
char path[PATH_MAX];
struct dirent *ent;
DIR *dir;
- u64 cnt = 0;
int ret = 0;
+ size_t cnt = 0, size = 0;
+ struct memory_node *nodes = NULL;

scnprintf(path, PATH_MAX, "%s/devices/system/node/",
sysfs__mountpoint());
@@ -1426,16 +1428,24 @@ static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
if (r != 1)
continue;

- if (WARN_ONCE(cnt >= size,
- "failed to write MEM_TOPOLOGY, way too many nodes\n")) {
- closedir(dir);
- return -1;
- }
+ if (cnt >= size) {
+ struct memory_node *new_nodes =
+ reallocarray(nodes, cnt + 4, sizeof(*nodes));

+ if (!new_nodes) {
+ pr_err("Failed to write MEM_TOPOLOGY, size %zd nodes\n", size);
+ free(nodes);
+ closedir(dir);
+ return -ENOMEM;
+ }
+ nodes = new_nodes;
+ size += 4;
+ }
ret = memory_node__read(&nodes[cnt++], idx);
}

*cntp = cnt;
+ *nodesp = nodes;
closedir(dir);

if (!ret)
@@ -1444,8 +1454,6 @@ static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
return ret;
}

-#define MAX_MEMORY_NODES 2000
-
/*
* The MEM_TOPOLOGY holds physical memory map for every
* node in system. The format of data is as follows:
@@ -1464,8 +1472,8 @@ static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
static int write_mem_topology(struct feat_fd *ff __maybe_unused,
struct evlist *evlist __maybe_unused)
{
- static struct memory_node nodes[MAX_MEMORY_NODES];
- u64 bsize, version = 1, i, nr;
+ struct memory_node *nodes = NULL;
+ u64 bsize, version = 1, i, nr = 0;
int ret;

ret = sysfs__read_xll("devices/system/memory/block_size_bytes",
@@ -1473,7 +1481,7 @@ static int write_mem_topology(struct feat_fd *ff __maybe_unused,
if (ret)
return ret;

- ret = build_mem_topology(&nodes[0], MAX_MEMORY_NODES, &nr);
+ ret = build_mem_topology(&nodes, &nr);
if (ret)
return ret;

@@ -1508,6 +1516,7 @@ static int write_mem_topology(struct feat_fd *ff __maybe_unused,
}

out:
+ free(nodes);
return ret;
}

--
2.40.1.698.g37aff9b760-goog

2023-05-25 20:09:04

by Namhyung Kim

Subject: Re: [PATCH v1 01/16] perf header: Make nodes dynamic in write_mem_topology

On Thu, May 25, 2023 at 12:12 AM Ian Rogers <[email protected]> wrote:
>
> Avoid a large static array, dynamically allocate the nodes avoiding a
> hard coded limited as well.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/header.c | 33 +++++++++++++++++++++------------
> 1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 2dde3ca20de5..80593ed8c79b 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -24,6 +24,7 @@
> #include <bpf/libbpf.h>
> #endif
> #include <perf/cpumap.h>
> +#include <tools/libc_compat.h> // reallocarray
>
> #include "dso.h"
> #include "evlist.h"
> @@ -1396,13 +1397,14 @@ static int memory_node__sort(const void *a, const void *b)
> return na->node - nb->node;
> }
>
> -static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
> +static int build_mem_topology(struct memory_node **nodesp, u64 *cntp)
> {
> char path[PATH_MAX];
> struct dirent *ent;
> DIR *dir;
> - u64 cnt = 0;
> int ret = 0;
> + size_t cnt = 0, size = 0;
> + struct memory_node *nodes = NULL;
>
> scnprintf(path, PATH_MAX, "%s/devices/system/node/",
> sysfs__mountpoint());
> @@ -1426,16 +1428,24 @@ static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
> if (r != 1)
> continue;
>
> - if (WARN_ONCE(cnt >= size,
> - "failed to write MEM_TOPOLOGY, way too many nodes\n")) {
> - closedir(dir);
> - return -1;
> - }
> + if (cnt >= size) {
> + struct memory_node *new_nodes =
> + reallocarray(nodes, cnt + 4, sizeof(*nodes));
>
> + if (!new_nodes) {
> + pr_err("Failed to write MEM_TOPOLOGY, size %zd nodes\n", size);
> + free(nodes);
> + closedir(dir);
> + return -ENOMEM;
> + }
> + nodes = new_nodes;
> + size += 4;
> + }
> ret = memory_node__read(&nodes[cnt++], idx);

I think you need to handle error cases here.

Thanks,
Namhyung


> }
>
> *cntp = cnt;
> + *nodesp = nodes;
> closedir(dir);
>
> if (!ret)
> @@ -1444,8 +1454,6 @@ static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
> return ret;
> }
>
> -#define MAX_MEMORY_NODES 2000
> -
> /*
> * The MEM_TOPOLOGY holds physical memory map for every
> * node in system. The format of data is as follows:
> @@ -1464,8 +1472,8 @@ static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
> static int write_mem_topology(struct feat_fd *ff __maybe_unused,
> struct evlist *evlist __maybe_unused)
> {
> - static struct memory_node nodes[MAX_MEMORY_NODES];
> - u64 bsize, version = 1, i, nr;
> + struct memory_node *nodes = NULL;
> + u64 bsize, version = 1, i, nr = 0;
> int ret;
>
> ret = sysfs__read_xll("devices/system/memory/block_size_bytes",
> @@ -1473,7 +1481,7 @@ static int write_mem_topology(struct feat_fd *ff __maybe_unused,
> if (ret)
> return ret;
>
> - ret = build_mem_topology(&nodes[0], MAX_MEMORY_NODES, &nr);
> + ret = build_mem_topology(&nodes, &nr);
> if (ret)
> return ret;
>
> @@ -1508,6 +1516,7 @@ static int write_mem_topology(struct feat_fd *ff __maybe_unused,
> }
>
> out:
> + free(nodes);
> return ret;
> }
>
> --
> 2.40.1.698.g37aff9b760-goog
>

2023-05-26 03:06:51

by Ian Rogers

Subject: Re: [PATCH v1 01/16] perf header: Make nodes dynamic in write_mem_topology

On Thu, May 25, 2023 at 12:15 PM Namhyung Kim <[email protected]> wrote:
>
> On Thu, May 25, 2023 at 12:12 AM Ian Rogers <[email protected]> wrote:
> >
> > Avoid a large static array, dynamically allocate the nodes avoiding a
> > hard coded limited as well.
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/util/header.c | 33 +++++++++++++++++++++------------
> > 1 file changed, 21 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index 2dde3ca20de5..80593ed8c79b 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -24,6 +24,7 @@
> > #include <bpf/libbpf.h>
> > #endif
> > #include <perf/cpumap.h>
> > +#include <tools/libc_compat.h> // reallocarray
> >
> > #include "dso.h"
> > #include "evlist.h"
> > @@ -1396,13 +1397,14 @@ static int memory_node__sort(const void *a, const void *b)
> > return na->node - nb->node;
> > }
> >
> > -static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
> > +static int build_mem_topology(struct memory_node **nodesp, u64 *cntp)
> > {
> > char path[PATH_MAX];
> > struct dirent *ent;
> > DIR *dir;
> > - u64 cnt = 0;
> > int ret = 0;
> > + size_t cnt = 0, size = 0;
> > + struct memory_node *nodes = NULL;
> >
> > scnprintf(path, PATH_MAX, "%s/devices/system/node/",
> > sysfs__mountpoint());
> > @@ -1426,16 +1428,24 @@ static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
> > if (r != 1)
> > continue;
> >
> > - if (WARN_ONCE(cnt >= size,
> > - "failed to write MEM_TOPOLOGY, way too many nodes\n")) {
> > - closedir(dir);
> > - return -1;
> > - }
> > + if (cnt >= size) {
> > + struct memory_node *new_nodes =
> > + reallocarray(nodes, cnt + 4, sizeof(*nodes));
> >
> > + if (!new_nodes) {
> > + pr_err("Failed to write MEM_TOPOLOGY, size %zd nodes\n", size);
> > + free(nodes);
> > + closedir(dir);
> > + return -ENOMEM;
> > + }
> > + nodes = new_nodes;
> > + size += 4;
> > + }
> > ret = memory_node__read(&nodes[cnt++], idx);
>
> I think you need to handle error cases here.
>
> Thanks,
> Namhyung

Not sure I follow. The reallocarray tests for failure, frees nodes and
returns -ENOMEM on error.

Thanks,
Ian

>
> > }
> >
> > *cntp = cnt;
> > + *nodesp = nodes;
> > closedir(dir);
> >
> > if (!ret)
> > @@ -1444,8 +1454,6 @@ static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
> > return ret;
> > }
> >
> > -#define MAX_MEMORY_NODES 2000
> > -
> > /*
> > * The MEM_TOPOLOGY holds physical memory map for every
> > * node in system. The format of data is as follows:
> > @@ -1464,8 +1472,8 @@ static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
> > static int write_mem_topology(struct feat_fd *ff __maybe_unused,
> > struct evlist *evlist __maybe_unused)
> > {
> > - static struct memory_node nodes[MAX_MEMORY_NODES];
> > - u64 bsize, version = 1, i, nr;
> > + struct memory_node *nodes = NULL;
> > + u64 bsize, version = 1, i, nr = 0;
> > int ret;
> >
> > ret = sysfs__read_xll("devices/system/memory/block_size_bytes",
> > @@ -1473,7 +1481,7 @@ static int write_mem_topology(struct feat_fd *ff __maybe_unused,
> > if (ret)
> > return ret;
> >
> > - ret = build_mem_topology(&nodes[0], MAX_MEMORY_NODES, &nr);
> > + ret = build_mem_topology(&nodes, &nr);
> > if (ret)
> > return ret;
> >
> > @@ -1508,6 +1516,7 @@ static int write_mem_topology(struct feat_fd *ff __maybe_unused,
> > }
> >
> > out:
> > + free(nodes);
> > return ret;
> > }
> >
> > --
> > 2.40.1.698.g37aff9b760-goog
> >

2023-05-26 06:07:50

by Namhyung Kim

Subject: Re: [PATCH v1 01/16] perf header: Make nodes dynamic in write_mem_topology

On Thu, May 25, 2023 at 7:39 PM Ian Rogers <[email protected]> wrote:
>
> On Thu, May 25, 2023 at 12:15 PM Namhyung Kim <[email protected]> wrote:
> >
> > On Thu, May 25, 2023 at 12:12 AM Ian Rogers <[email protected]> wrote:
> > >
> > > Avoid a large static array, dynamically allocate the nodes avoiding a
> > > hard coded limited as well.
> > >
> > > Signed-off-by: Ian Rogers <[email protected]>
> > > ---
> > > tools/perf/util/header.c | 33 +++++++++++++++++++++------------
> > > 1 file changed, 21 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > > index 2dde3ca20de5..80593ed8c79b 100644
> > > --- a/tools/perf/util/header.c
> > > +++ b/tools/perf/util/header.c
> > > @@ -24,6 +24,7 @@
> > > #include <bpf/libbpf.h>
> > > #endif
> > > #include <perf/cpumap.h>
> > > +#include <tools/libc_compat.h> // reallocarray
> > >
> > > #include "dso.h"
> > > #include "evlist.h"
> > > @@ -1396,13 +1397,14 @@ static int memory_node__sort(const void *a, const void *b)
> > > return na->node - nb->node;
> > > }
> > >
> > > -static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
> > > +static int build_mem_topology(struct memory_node **nodesp, u64 *cntp)
> > > {
> > > char path[PATH_MAX];
> > > struct dirent *ent;
> > > DIR *dir;
> > > - u64 cnt = 0;
> > > int ret = 0;
> > > + size_t cnt = 0, size = 0;
> > > + struct memory_node *nodes = NULL;
> > >
> > > scnprintf(path, PATH_MAX, "%s/devices/system/node/",
> > > sysfs__mountpoint());
> > > @@ -1426,16 +1428,24 @@ static int build_mem_topology(struct memory_node *nodes, u64 size, u64 *cntp)
> > > if (r != 1)
> > > continue;
> > >
> > > - if (WARN_ONCE(cnt >= size,
> > > - "failed to write MEM_TOPOLOGY, way too many nodes\n")) {
> > > - closedir(dir);
> > > - return -1;
> > > - }
> > > + if (cnt >= size) {
> > > + struct memory_node *new_nodes =
> > > + reallocarray(nodes, cnt + 4, sizeof(*nodes));
> > >
> > > + if (!new_nodes) {
> > > + pr_err("Failed to write MEM_TOPOLOGY, size %zd nodes\n", size);
> > > + free(nodes);
> > > + closedir(dir);
> > > + return -ENOMEM;
> > > + }
> > > + nodes = new_nodes;
> > > + size += 4;
> > > + }
> > > ret = memory_node__read(&nodes[cnt++], idx);
> >
> > I think you need to handle error cases here.
> >
> > Thanks,
> > Namhyung
>
> Not sure I follow. The reallocarray tests for failure, frees nodes and
> returns -ENOMEM on error.

Right, but I think it would leak the nodes when the
memory_node__read() fails.

Thanks,
Namhyung