Re: [PATCH v13 2/8] perf evlist: Introduce aux evlist
From: Wangnan (F)
Date: Mon Jul 11 2016 - 06:21:10 EST
On 2016/7/8 22:46, Jiri Olsa wrote:
On Wed, Jul 06, 2016 at 08:16:52PM +0800, Wangnan (F) wrote:
On 2016/7/6 19:36, Jiri Olsa wrote:
On Mon, Jul 04, 2016 at 06:20:03AM +0000, Wang Nan wrote:
SNIP
+struct perf_evlist *perf_evlist__new_aux(struct perf_evlist *parent)
+{
+ struct perf_evlist *evlist;
+
+ if (perf_evlist__is_aux(parent)) {
+ pr_err("Internal error: create aux evlist from another aux evlist\n");
+ return NULL;
+ }
+
+ evlist = zalloc(sizeof(*evlist));
+ if (!evlist)
+ return NULL;
+
+ perf_evlist__init(evlist, parent->cpus, parent->threads);
+ evlist->parent = parent;
+ INIT_LIST_HEAD(&evlist->list);
+ list_add(&evlist->list, &parent->children);
I understand there's some reason for separating maps with and
without overwrite set, but I'm missing it.. why is that?
You are asking overwrite, not write_backward?
Overwrite mapping needs to be mapped without PROT_WRITE, so its
control page is also read only, so perf_evlist__mmap_consume() is
not able to use, and there's no way to tell kernel to where we have
read. Kernel overwrite old records when its full. Compare with normal
mapping: perf uses perf_evlist__mmap_consume() to tell kernel the
last byte it has read, so kernel stop writing data to it when it full,
and issues LOST event. This is the reason we need to separate maps
with and without overwrite set.
For write backward: kernel write data in different direction, so
requires map separation.
I dont like the idea of duplicating whole perf_evlist
in order just to map some events with overwrite/backward
perf_evlist carries all the other info about events,
not just memory maping..
I think it'd be better to do it some other way, like:
- we have mmaps for events/evsels, so you're able to map
it differently with or without PROT_WRITE even in current
design.. there's struct perf_mmap that can carry that info
then it's the matter of reading/processing those maps
that needs to change.. new perf_evlist interface
- we could keep separate struct perf_mmap arrays for forward
and backward/overwrite maps
- ...
I understand both mapping need different treatment,
but I think that should be encapsulated within the
struct perf_evlist interface
I don't like it either, but aux_evlist is the easiest way to
do this work. Other potential solutions require heavy API changes.
Thank you.