lkml.org 
[lkml]   [2013]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
From
SubjectRe: [PATCH v2 3/4] Transform mmap and other related structures to list with new xyarray
On 2/26/13 2:41 AM, chenggang wrote:
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 774c907..13112c6 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -31,6 +31,8 @@
> #include <sched.h>
> #include <sys/mman.h>
>
> +#define MMAP(e, y) (*(struct perf_mmap *)xyarray__entry(e->mmap, 0, y))
> +

That is ugly to have in perf commands. It would be better to hide such
details within evlist.c. e.g.

struct perf_mmap *perf_evlist__get_mmap(struct perf_evlist *evlist, int i)

> #ifndef HAVE_ON_EXIT
> #ifndef ATEXIT_MAX
> #define ATEXIT_MAX 32
> @@ -367,8 +369,8 @@ static int perf_record__mmap_read_all(struct perf_record *rec)
> int rc = 0;
>
> for (i = 0; i < rec->evlist->nr_mmaps; i++) {
> - if (rec->evlist->mmap[i].base) {
> - if (perf_record__mmap_read(rec, &rec->evlist->mmap[i]) != 0) {
> + if (MMAP(rec->evlist, i).base) {
> + if (perf_record__mmap_read(rec, &MMAP(rec->evlist, i)) != 0) {
> rc = -1;
> goto out;
> }

and then here get the mmap, if base is set call the read function.

However, changing the mmaps from an indexed array to a linked list is
going to have a cost with loops like this one - especially as the number
of events goes up (e.g., perf record -e kvm:*). Might be better to walk
the mmap list and call mmap_read for each.

---8<---

> +/*
> + * If threads->nr > 1, the cpu_map__nr() must be 1.
> + * If the cpu_map__nr() > 1, we should not append pollfd.
> + */
> +static int perf_evlist__append_pollfd_thread(struct perf_evlist *evlist)
> +{
> + int new_nfds;
> +
> + if (cpu_map__all(evlist->cpus)) {
> + struct pollfd *pfd;
> +
> + new_nfds = evlist->threads->nr * evlist->nr_entries;
> + pfd = zalloc(sizeof(struct pollfd) * new_nfds);
> +
> + if (!pfd)
> + return -1;
> +
> + memcpy(pfd, evlist->pollfd, (evlist->threads->nr - 1) * evlist->nr_entries);
> +
> + evlist->pollfd = pfd;
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> static int perf_evlist__alloc_pollfd(struct perf_evlist *evlist)
> {
> int nfds = cpu_map__nr(evlist->cpus) * evlist->threads->nr * evlist->nr_entries;
> @@ -288,7 +316,7 @@ void perf_evlist__id_add(struct perf_evlist *evlist, struct perf_evsel *evsel,
> int cpu, int thread, u64 id)
> {
> perf_evlist__id_hash(evlist, evsel, cpu, thread, id);
> - evsel->id[evsel->ids++] = id;
> + ID(evsel, evsel->ids++) = id;
> }

The pollfd changes should be a separate patch (should be possible; seems
independent of the mmap change).

>
> static int perf_evlist__id_add_fd(struct perf_evlist *evlist,
> @@ -336,7 +364,7 @@ struct perf_evsel *perf_evlist__id2evsel(struct perf_evlist *evlist, u64 id)
>
> union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
> {
> - struct perf_mmap *md = &evlist->mmap[idx];
> + struct perf_mmap *md = &MMAP(evlist, idx);
> unsigned int head = perf_mmap__read_head(md);
> unsigned int old = md->prev;
> unsigned char *data = md->base + page_size;
> @@ -404,9 +432,9 @@ void perf_evlist__munmap(struct perf_evlist *evlist)
> int i;
>
> for (i = 0; i < evlist->nr_mmaps; i++) {
> - if (evlist->mmap[i].base != NULL) {
> - munmap(evlist->mmap[i].base, evlist->mmap_len);
> - evlist->mmap[i].base = NULL;
> + if (MMAP(evlist, i).base != NULL) {
> + munmap(MMAP(evlist, i).base, evlist->mmap_len);
> + MMAP(evlist, i).base = NULL;
> }
> }

same comment here -- walk the mmap list rather than index looping.
Changing evlist as threads come and go will have implications on
multi-threaded users.


---8<---

> +int perf_evlist__mmap_thread(struct perf_evlist *evlist, bool overwrite, int tidx)
> +{
> + struct perf_evsel *evsel;
> + int prot = PROT_READ | (overwrite ? 0 : PROT_WRITE);
> + int mask = evlist->mmap_len - page_size - 1;
> + int output = -1;
> + struct pollfd *old_pollfd = evlist->pollfd;
> +
> + if (!cpu_map__all(evlist->cpus))
> + return 1;
> +
> + if (perf_evlist__append_mmap_thread(evlist) < 0)
> + return -ENOMEM;
> +
> + if (perf_evlist__append_pollfd_thread(evlist) < 0)
> + goto free_append_mmap;
> +
> + list_for_each_entry(evsel, &evlist->entries, node)
> + if ((evsel->attr.read_format & PERF_FORMAT_ID) &&
> + evsel->sample_id == NULL)
> + if (perf_evsel__append_id_thread(evsel, tidx) < 0)
> + goto free_append_pollfd;

braces


---8<---
> +void perf_evsel__close_thread(struct perf_evsel *evsel, int cpu_nr, int tidx)
> +{
> + int cpu;
> +
> + for (cpu = 0; cpu < cpu_nr; cpu++)
> + if (FD(evsel, cpu, tidx) >= 0)
> + close(FD(evsel, cpu, tidx));

braces

---8<---
\
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index f4bfd79..51a52d4 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -25,6 +25,8 @@
> #include "strbuf.h"
> #include "build-id.h"
>
> +#define ID(e, y) (*(u64 *)xyarray__entry(e->id, 0, y))

again here, really do not want that outside of the evlist.c/evsel.c files

> +
> static bool no_buildid_cache = false;
>
> static int trace_event_count;
> @@ -1260,7 +1262,6 @@ static struct perf_evsel *
> read_event_desc(struct perf_header *ph, int fd)
> {
> struct perf_evsel *evsel, *events = NULL;
> - u64 *id;
> void *buf = NULL;
> u32 nre, sz, nr, i, j;
> ssize_t ret;
> @@ -1325,19 +1326,17 @@ read_event_desc(struct perf_header *ph, int fd)
> if (!nr)
> continue;
>
> - id = calloc(nr, sizeof(*id));
> - if (!id)
> - goto error;
> evsel->ids = nr;
> - evsel->id = id;
> + evsel->id = xyarray__new(1, nr, sizeof(u64));
> + if (!evsel->id)
> + goto error;

perf_evsel__id_new()?

>
> for (j = 0 ; j < nr; j++) {
> - ret = readn(fd, id, sizeof(*id));
> - if (ret != (ssize_t)sizeof(*id))
> + ret = readn(fd, &ID(evsel, j), sizeof(u64));

perf_evsel__get_id()?

Also, think about how to break up the patches into smaller change sets.

David


\
 
 \ /
  Last update: 2013-02-28 18:21    [W:0.089 / U:0.336 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site