Date:	Thu, 28 Feb 2013 09:34:58 -0700
From:	David Ahern <dsahern@...il.com>
To:	chenggang <chenggang.qin@...il.com>
CC:	linux-kernel@...r.kernel.org, chenggang <chenggang.qcg@...bao.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Paul Mackerras <paulus@...ba.org>,
	Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	Namhyung Kim <namhyung@...il.com>,
	Yanmin Zhang <yanmin.zhang@...el.com>,
	Wu Fengguang <fengguang.wu@...el.com>,
	Mike Galbraith <efault@....de>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists