Messages in this thread | | | From | Alexander Shishkin <> | Subject | Re: [PATCH]: perf/core: addressing 4x slowdown during per-process profiling of STREAM benchmark on Intel Xeon Phi | Date | Mon, 29 May 2017 15:03:35 +0300 |
| |
Alexey Budankov <alexey.budankov@linux.intel.com> writes:
Here (above the function) you could include a comment describing what happens when this is called, locking considerations, etc.
> +static int > +perf_cpu_tree_insert(struct rb_root *tree, struct perf_event *event) > +{ > + struct rb_node **node; > + struct rb_node *parent; > + > + if (!tree || !event) > + return 0;
I don't think this should be happening, should it? And either way you probably don't want to return 0 here, unless you're using !0 for success.
> + > + node = &tree->rb_node; > + parent = *node; > + > + while (*node) { > + struct perf_event *node_event = container_of(*node, > + struct perf_event, group_node); > + > + parent = *node; > + > + if (event->cpu < node_event->cpu) { > + node = &((*node)->rb_left);
this would be the same as node = &parent->rb_left, right?
> + } else if (event->cpu > node_event->cpu) { > + node = &((*node)->rb_right); > + } else { > + list_add_tail(&event->group_list_entry, > + &node_event->group_list);
So why is this better than simply having per-cpu event lists plus one for per-thread events?
Also,
> + return 2;
2?
> + } > + } > + > + list_add_tail(&event->group_list_entry, &event->group_list); > + > + rb_link_node(&event->group_node, parent, node); > + rb_insert_color(&event->group_node, tree); > + > + return 1;
Oh, you are using !0 for success. I guess it's a good thing you're not actually checking its return code at the call site.
Regards, -- Alex
| |