On 29.05.2017 15:03, Alexander Shishkin wrote:
Alexey Budankov <alexey.budan...@linux.intel.com> writes:
Here (above the function) you could include a comment describing what
happens when this is called, locking considerations, etc.
I can put the short description from the initial thread message here.
Would it be sufficient?
+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.
As you might notice already, currently return codes of the tree API are
not checked all other the implementation. I suggest replacing that int
error code by void and simplify the stuff.
+
+ 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?
Please ask more. node is the leaf node and parent is the parent of the
node at the end of cycle. We need the both to insert a new node into a
tree.
+ } 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?
Good question. Choice of data structure and layout depends on the
operations applied to the data so keeping groups as a tree simplifies
and improves the implementation in terms of scalability and performance.
Please ask more if any.
Also,
+ return 2;
2?
Answered above.
+ }
+ }
+
+ 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.
Answered above.
Regards,
--
Alex
