Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1714568 > unrolled thread

[PATCH v7 0/2] perf/core: addressing 4x slowdown during per-process profiling of STREAM benchmark on Intel Xeon Phi

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v7 0/2] perf/core: addressing 4x slowdown during per-process  profiling of STREAM benchmark on Intel Xeon Phi Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-08-18 07:20 +0200
    [PATCH v7 1/2] perf/core: use rb trees for pinned/flexible groups Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-08-18 07:30 +0200
      Re: [PATCH v7 1/2] perf/core: use rb trees for pinned/flexible groups Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-08-23 13:30 +0200
        Re: [PATCH v7 1/2] perf/core: use rb trees for pinned/flexible groups Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-08-23 19:30 +0200
    [PATCH v7 2/2] perf/core: add mux switch to skip to the current CPU's  events list on mux interrupt Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-08-18 07:30 +0200
      Re: [PATCH v7 2/2] perf/core: add mux switch to skip to the current CPU's events list on mux interrupt Alexander Shishkin <alexander.shishkin@linux.intel.com> - 2017-08-23 14:10 +0200
        Re: [PATCH v7 2/2] perf/core: add mux switch to skip to the current  CPU's events list on mux interrupt Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-08-23 20:20 +0200
    Re: [PATCH v7 0/2] perf/core: addressing 4x slowdown during  per-process profiling of STREAM benchmark on Intel Xeon Phi Peter Zijlstra <peterz@infradead.org> - 2017-08-22 22:30 +0200
      Re: [PATCH v7 0/2] perf/core: addressing 4x slowdown during  per-process profiling of STREAM benchmark on Intel Xeon Phi Alexey Budankov <alexey.budankov@linux.intel.com> - 2017-08-23 11:00 +0200

#1714568 — [PATCH v7 0/2] perf/core: addressing 4x slowdown during per-process profiling of STREAM benchmark on Intel Xeon Phi

Hi,

This patch set v7 moves event groups into rb trees and implements 
skipping to the current CPU's list on hrtimer interrupt.

Events allocated for the same CPU are still kept in a linked list
of the event directly attached to the tree because it is unclear 
how to implement fast iteration thru events allocated for 
the same CPU when they are all attached to a tree employing 
additional 64bit index as a secondary treee key.

The patch set addresses feeback captured previously. Specifically
API with a callback in signature is replaced by a macro what reduced
the size of adapting changes.

Patches in the set are expected to be applied one after another in 
the mentioned order and they are logically split into two parts 
to simplify the review process.

For more background details and feedback of the patch set please 
refer to v6 and older.

Thanks,
Alexey

---
 Alexey Budankov (2):
	perf/core: use rb trees for pinned/flexible groups
	perf/core: add mux switch to skip to the current CPU's events list on mux interrupt

 include/linux/perf_event.h |  19 +-
 kernel/events/core.c       | 463 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 364 insertions(+), 118 deletions(-)

[toc] | [next] | [standalone]

#1714572 — [PATCH v7 1/2] perf/core: use rb trees for pinned/flexible groups

This patch moves event groups into rb tree sorted by CPU, so that 
multiplexing hrtimer interrupt handler would be able skipping to the current 
CPU's list and ignore groups allocated for the other CPUs.

New API for manipulating event groups in the trees is implemented as well 
as adoption on the API in the current implementation.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 include/linux/perf_event.h |  19 ++-
 kernel/events/core.c       | 314 +++++++++++++++++++++++++++++++++------------
 2 files changed, 249 insertions(+), 84 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index b14095b..cc07904 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -572,7 +572,20 @@ struct perf_event {
 	 */
 	struct list_head		group_entry;
 	struct list_head		sibling_list;
-
+	/*
+	 * Node on the pinned or flexible tree located at the event context;
+	 * the node may be empty in case its event is not directly attached
+	 * to the tree but to group_list list of the event directly
+	 * attached to the tree;
+	 */
+	struct rb_node			group_node;
+	/*
+	 * List keeps groups allocated for the same cpu;
+	 * the list may be empty in case its event is not directly
+	 * attached to the tree but to group_list list of the event directly
+	 * attached to the tree;
+	 */
+	struct list_head		group_list;
 	/*
 	 * We need storage to track the entries in perf_pmu_migrate_context; we
 	 * cannot use the event_entry because of RCU and we want to keep the
@@ -741,8 +754,8 @@ struct perf_event_context {
 	struct mutex			mutex;
 
 	struct list_head		active_ctx_list;
-	struct list_head		pinned_groups;
-	struct list_head		flexible_groups;
+	struct rb_root			pinned_groups;
+	struct rb_root			flexible_groups;
 	struct list_head		event_list;
 	int				nr_events;
 	int				nr_active;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index d704e23..08ccfb2 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1466,8 +1466,12 @@ static enum event_type_t get_event_type(struct perf_event *event)
 	return event_type;
 }
 
-static struct list_head *
-ctx_group_list(struct perf_event *event, struct perf_event_context *ctx)
+/*
+ * Extract pinned or flexible groups from the context
+ * based on event attrs bits;
+ */
+static struct rb_root *
+get_event_groups(struct perf_event *event, struct perf_event_context *ctx)
 {
 	if (event->attr.pinned)
 		return &ctx->pinned_groups;
@@ -1476,6 +1480,143 @@ ctx_group_list(struct perf_event *event, struct perf_event_context *ctx)
 }
 
 /*
+ * Insert a group into a tree using event->cpu as a key. If event->cpu node
+ * is already attached to the tree then the event is added to the attached
+ * group's group_list list.
+ */
+static void
+perf_event_groups_insert(struct rb_root *groups, struct perf_event *event)
+{
+	struct perf_event *node_event;
+	struct rb_node *parent;
+	struct rb_node **node;
+
+	node = &groups->rb_node;
+	parent = *node;
+
+	while (*node) {
+		parent = *node;
+		node_event = container_of(*node,
+				struct perf_event, group_node);
+
+		if (event->cpu < node_event->cpu) {
+			node = &parent->rb_left;
+		} else if (event->cpu > node_event->cpu) {
+			node = &parent->rb_right;
+		} else {
+			list_add_tail(&event->group_entry,
+					&node_event->group_list);
+			return;
+		}
+	}
+
+	list_add_tail(&event->group_entry, &event->group_list);
+
+	rb_link_node(&event->group_node, parent, node);
+	rb_insert_color(&event->group_node, groups);
+}
+
+/*
+ * Helper function to insert event into the pinned or
+ * flexible groups;
+ */
+static void
+add_event_to_groups(struct perf_event *event, struct perf_event_context *ctx)
+{
+	struct rb_root *groups;
+
+	groups = get_event_groups(event, ctx);
+	perf_event_groups_insert(groups, event);
+}
+
+/*
+ * Delete a group from a tree. If the group is directly attached to the tree
+ * it is replaced by the next group on the group's group_list.
+ */
+static void
+perf_event_groups_delete(struct rb_root *groups, struct perf_event *event)
+{
+	list_del_init(&event->group_entry);
+
+	if (!RB_EMPTY_NODE(&event->group_node)) {
+		if (!RB_EMPTY_ROOT(groups)) {
+			if (list_empty(&event->group_list)) {
+				rb_erase(&event->group_node, groups);
+			} else {
+				struct perf_event *next =
+					list_first_entry(&event->group_list,
+						struct perf_event, group_entry);
+				list_replace_init(&event->group_list,
+						&next->group_list);
+				rb_replace_node(&event->group_node,
+						&next->group_node, groups);
+			}
+		}
+		RB_CLEAR_NODE(&event->group_node);
+	}
+}
+
+/*
+ * Helper function to delete event from its groups;
+ */
+static void
+del_event_from_groups(struct perf_event *event, struct perf_event_context *ctx)
+{
+	struct rb_root *groups;
+
+	groups = get_event_groups(event, ctx);
+	perf_event_groups_delete(groups, event);
+}
+
+/*
+ * Find group_list list by a cpu key.
+ */
+static struct list_head *
+perf_event_groups_get_list(struct rb_root *groups, int cpu)
+{
+	struct perf_event *node_event;
+	struct rb_node *node;
+
+	node = groups->rb_node;
+
+	while (node) {
+		node_event = container_of(node,
+				struct perf_event, group_node);
+
+		if (cpu < node_event->cpu) {
+			node = node->rb_left;
+		} else if (cpu > node_event->cpu) {
+			node = node->rb_right;
+		} else {
+			return &node_event->group_list;
+		}
+	}
+
+	return NULL;
+}
+
+/*
+ * Find group list by a cpu key and rotate it.
+ */
+static void
+perf_event_groups_rotate(struct rb_root *groups, int cpu)
+{
+	struct list_head *group_list =
+			perf_event_groups_get_list(groups, cpu);
+
+	if (group_list)
+		list_rotate_left(group_list);
+}
+
+/*
+ * Iterate event groups thru the whole tree.
+ */
+#define perf_event_groups_for_each(event, iter, tree, node, list, link) \
+	   for (iter = rb_first(tree); iter; iter = rb_next(iter))	\
+		list_for_each_entry(event, &(rb_entry(iter,		\
+			typeof(*event), node)->list), link)
+
+/*
  * Add a event from the lists for its context.
  * Must be called with ctx->mutex and ctx->lock held.
  */
@@ -1493,12 +1634,8 @@ list_add_event(struct perf_event *event, struct perf_event_context *ctx)
 	 * perf_group_detach can, at all times, locate all siblings.
 	 */
 	if (event->group_leader == event) {
-		struct list_head *list;
-
 		event->group_caps = event->event_caps;
-
-		list = ctx_group_list(event, ctx);
-		list_add_tail(&event->group_entry, list);
+		add_event_to_groups(event, ctx);
 	}
 
 	list_update_cgroup_event(event, ctx, true);
@@ -1689,7 +1826,7 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
 	list_del_rcu(&event->event_entry);
 
 	if (event->group_leader == event)
-		list_del_init(&event->group_entry);
+		del_event_from_groups(event, ctx);
 
 	update_group_times(event);
 
@@ -1709,7 +1846,6 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
 static void perf_group_detach(struct perf_event *event)
 {
 	struct perf_event *sibling, *tmp;
-	struct list_head *list = NULL;
 
 	lockdep_assert_held(&event->ctx->lock);
 
@@ -1730,22 +1866,22 @@ static void perf_group_detach(struct perf_event *event)
 		goto out;
 	}
 
-	if (!list_empty(&event->group_entry))
-		list = &event->group_entry;
-
 	/*
 	 * If this was a group event with sibling events then
 	 * upgrade the siblings to singleton events by adding them
 	 * to whatever list we are on.
 	 */
 	list_for_each_entry_safe(sibling, tmp, &event->sibling_list, group_entry) {
-		if (list)
-			list_move_tail(&sibling->group_entry, list);
 		sibling->group_leader = sibling;
 
 		/* Inherit group flags from the previous leader */
 		sibling->group_caps = event->group_caps;
 
+		if (!list_empty(&event->group_entry)) {
+			list_del_init(&sibling->group_entry);
+			add_event_to_groups(sibling, event->ctx);
+		}
+
 		WARN_ON_ONCE(sibling->ctx != event->ctx);
 	}
 
@@ -2744,7 +2880,7 @@ static void ctx_sched_out(struct perf_event_context *ctx,
 {
 	int is_active = ctx->is_active;
 	struct perf_event *event;
-
+	struct rb_node *node;
 	lockdep_assert_held(&ctx->lock);
 
 	if (likely(!ctx->nr_events)) {
@@ -2789,15 +2925,19 @@ static void ctx_sched_out(struct perf_event_context *ctx,
 		return;
 
 	perf_pmu_disable(ctx->pmu);
-	if (is_active & EVENT_PINNED) {
-		list_for_each_entry(event, &ctx->pinned_groups, group_entry)
+
+	if (is_active & EVENT_PINNED)
+		perf_event_groups_for_each(event, node,
+				&ctx->pinned_groups, group_node,
+				group_list, group_entry)
 			group_sched_out(event, cpuctx, ctx);
-	}
 
-	if (is_active & EVENT_FLEXIBLE) {
-		list_for_each_entry(event, &ctx->flexible_groups, group_entry)
+	if (is_active & EVENT_FLEXIBLE)
+		perf_event_groups_for_each(event, node,
+				&ctx->flexible_groups, group_node,
+				group_list, group_entry)
 			group_sched_out(event, cpuctx, ctx);
-	}
+
 	perf_pmu_enable(ctx->pmu);
 }
 
@@ -3091,61 +3231,55 @@ static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx,
 }
 
 static void
-ctx_pinned_sched_in(struct perf_event_context *ctx,
-		    struct perf_cpu_context *cpuctx)
+ctx_pinned_sched_in(struct perf_event *event,
+		   struct perf_cpu_context *cpuctx,
+		   struct perf_event_context *ctx)
 {
-	struct perf_event *event;
-
-	list_for_each_entry(event, &ctx->pinned_groups, group_entry) {
-		if (event->state <= PERF_EVENT_STATE_OFF)
-			continue;
-		if (!event_filter_match(event))
-			continue;
+	if (event->state <= PERF_EVENT_STATE_OFF)
+		return;
+	if (!event_filter_match(event))
+		return;
 
-		/* may need to reset tstamp_enabled */
-		if (is_cgroup_event(event))
-			perf_cgroup_mark_enabled(event, ctx);
+	/* may need to reset tstamp_enabled */
+	if (is_cgroup_event(event))
+		perf_cgroup_mark_enabled(event, ctx);
 
-		if (group_can_go_on(event, cpuctx, 1))
-			group_sched_in(event, cpuctx, ctx);
+	if (group_can_go_on(event, cpuctx, 1))
+		group_sched_in(event, cpuctx, ctx);
 
-		/*
-		 * If this pinned group hasn't been scheduled,
-		 * put it in error state.
-		 */
-		if (event->state == PERF_EVENT_STATE_INACTIVE) {
-			update_group_times(event);
-			event->state = PERF_EVENT_STATE_ERROR;
-		}
+	/*
+	 * If this pinned group hasn't been scheduled,
+	 * put it in error state.
+	 */
+	if (event->state == PERF_EVENT_STATE_INACTIVE) {
+		update_group_times(event);
+		event->state = PERF_EVENT_STATE_ERROR;
 	}
 }
 
 static void
-ctx_flexible_sched_in(struct perf_event_context *ctx,
-		      struct perf_cpu_context *cpuctx)
+ctx_flexible_sched_in(struct perf_event *event,
+		     struct perf_cpu_context *cpuctx,
+		     struct perf_event_context *ctx,
+		     int *can_add_hw)
 {
-	struct perf_event *event;
-	int can_add_hw = 1;
-
-	list_for_each_entry(event, &ctx->flexible_groups, group_entry) {
-		/* Ignore events in OFF or ERROR state */
-		if (event->state <= PERF_EVENT_STATE_OFF)
-			continue;
-		/*
-		 * Listen to the 'cpu' scheduling filter constraint
-		 * of events:
-		 */
-		if (!event_filter_match(event))
-			continue;
+	/* Ignore events in OFF or ERROR state */
+	if (event->state <= PERF_EVENT_STATE_OFF)
+		return;
+	/*
+	 * Listen to the 'cpu' scheduling filter constraint
+	 * of events:
+	 */
+	if (!event_filter_match(event))
+		return;
 
-		/* may need to reset tstamp_enabled */
-		if (is_cgroup_event(event))
-			perf_cgroup_mark_enabled(event, ctx);
+	/* may need to reset tstamp_enabled */
+	if (is_cgroup_event(event))
+		perf_cgroup_mark_enabled(event, ctx);
 
-		if (group_can_go_on(event, cpuctx, can_add_hw)) {
-			if (group_sched_in(event, cpuctx, ctx))
-				can_add_hw = 0;
-		}
+	if (group_can_go_on(event, cpuctx, *can_add_hw)) {
+		if (group_sched_in(event, cpuctx, ctx))
+			*can_add_hw = 0;
 	}
 }
 
@@ -3156,7 +3290,8 @@ ctx_sched_in(struct perf_event_context *ctx,
 	     struct task_struct *task)
 {
 	int is_active = ctx->is_active;
-	u64 now;
+	struct perf_event *event;
+	struct rb_node *node;
 
 	lockdep_assert_held(&ctx->lock);
 
@@ -3175,7 +3310,7 @@ ctx_sched_in(struct perf_event_context *ctx,
 
 	if (is_active & EVENT_TIME) {
 		/* start ctx time */
-		now = perf_clock();
+		u64 now = perf_clock();
 		ctx->timestamp = now;
 		perf_cgroup_set_timestamp(task, ctx);
 	}
@@ -3185,11 +3320,19 @@ ctx_sched_in(struct perf_event_context *ctx,
 	 * in order to give them the best chance of going on.
 	 */
 	if (is_active & EVENT_PINNED)
-		ctx_pinned_sched_in(ctx, cpuctx);
+		perf_event_groups_for_each(event, node,
+				&ctx->pinned_groups, group_node,
+				group_list, group_entry)
+			ctx_pinned_sched_in(event, cpuctx, ctx);
 
 	/* Then walk through the lower prio flexible groups */
-	if (is_active & EVENT_FLEXIBLE)
-		ctx_flexible_sched_in(ctx, cpuctx);
+	if (is_active & EVENT_FLEXIBLE) {
+		int can_add_hw = 1;
+		perf_event_groups_for_each(event, node,
+				&ctx->flexible_groups, group_node,
+				group_list, group_entry)
+			ctx_flexible_sched_in(event, cpuctx, ctx, &can_add_hw);
+	}
 }
 
 static void cpu_ctx_sched_in(struct perf_cpu_context *cpuctx,
@@ -3227,7 +3370,7 @@ static void perf_event_context_sched_in(struct perf_event_context *ctx,
 	 * However, if task's ctx is not carrying any pinned
 	 * events, no need to flip the cpuctx's events around.
 	 */
-	if (!list_empty(&ctx->pinned_groups))
+	if (!RB_EMPTY_ROOT(&ctx->pinned_groups))
 		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
 	perf_event_sched_in(cpuctx, ctx, task);
 	perf_pmu_enable(ctx->pmu);
@@ -3464,8 +3607,12 @@ static void rotate_ctx(struct perf_event_context *ctx)
 	 * Rotate the first entry last of non-pinned groups. Rotation might be
 	 * disabled by the inheritance code.
 	 */
-	if (!ctx->rotate_disable)
-		list_rotate_left(&ctx->flexible_groups);
+	if (!ctx->rotate_disable) {
+		int sw = -1, cpu = smp_processor_id();
+
+		perf_event_groups_rotate(&ctx->flexible_groups, sw);
+		perf_event_groups_rotate(&ctx->flexible_groups, cpu);
+	}
 }
 
 static int perf_rotate_context(struct perf_cpu_context *cpuctx)
@@ -3804,8 +3951,8 @@ static void __perf_event_init_context(struct perf_event_context *ctx)
 	raw_spin_lock_init(&ctx->lock);
 	mutex_init(&ctx->mutex);
 	INIT_LIST_HEAD(&ctx->active_ctx_list);
-	INIT_LIST_HEAD(&ctx->pinned_groups);
-	INIT_LIST_HEAD(&ctx->flexible_groups);
+	ctx->pinned_groups = RB_ROOT;
+	ctx->flexible_groups = RB_ROOT;
 	INIT_LIST_HEAD(&ctx->event_list);
 	atomic_set(&ctx->refcount, 1);
 }
@@ -9412,6 +9559,8 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 	INIT_LIST_HEAD(&event->group_entry);
 	INIT_LIST_HEAD(&event->event_entry);
 	INIT_LIST_HEAD(&event->sibling_list);
+	RB_CLEAR_NODE(&event->group_node);
+	INIT_LIST_HEAD(&event->group_list);
 	INIT_LIST_HEAD(&event->rb_entry);
 	INIT_LIST_HEAD(&event->active_entry);
 	INIT_LIST_HEAD(&event->addr_filters.list);
@@ -10839,9 +10988,9 @@ static int inherit_group(struct perf_event *parent_event,
  */
 static int
 inherit_task_group(struct perf_event *event, struct task_struct *parent,
-		   struct perf_event_context *parent_ctx,
-		   struct task_struct *child, int ctxn,
-		   int *inherited_all)
+		  struct perf_event_context *parent_ctx,
+		  struct task_struct *child, int ctxn,
+		  int *inherited_all)
 {
 	int ret;
 	struct perf_event_context *child_ctx;
@@ -10859,7 +11008,7 @@ inherit_task_group(struct perf_event *event, struct task_struct *parent,
 		 * First allocate and initialize a context for the
 		 * child.
 		 */
-		child_ctx = alloc_perf_context(parent_ctx->pmu, child);
+		child_ctx = alloc_perf_context(parent_ctx->pmu,	child);
 		if (!child_ctx)
 			return -ENOMEM;
 
@@ -10883,6 +11032,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 	struct perf_event_context *child_ctx, *parent_ctx;
 	struct perf_event_context *cloned_ctx;
 	struct perf_event *event;
+	struct rb_node *node;
 	struct task_struct *parent = current;
 	int inherited_all = 1;
 	unsigned long flags;
@@ -10916,7 +11066,8 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 	 * We dont have to disable NMIs - we are only looking at
 	 * the list, not manipulating it:
 	 */
-	list_for_each_entry(event, &parent_ctx->pinned_groups, group_entry) {
+	perf_event_groups_for_each(event, node,	&parent_ctx->pinned_groups,
+			group_node, group_list, group_entry) {
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)
@@ -10932,7 +11083,8 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
 	parent_ctx->rotate_disable = 1;
 	raw_spin_unlock_irqrestore(&parent_ctx->lock, flags);
 
-	list_for_each_entry(event, &parent_ctx->flexible_groups, group_entry) {
+	perf_event_groups_for_each(event, node,	&parent_ctx->flexible_groups,
+			group_node, group_list, group_entry) {
 		ret = inherit_task_group(event, parent, parent_ctx,
 					 child, ctxn, &inherited_all);
 		if (ret)

[toc] | [prev] | [next] | [standalone]

#1718262 — Re: [PATCH v7 1/2] perf/core: use rb trees for pinned/flexible groups

Alexey Budankov <alexey.budankov@linux.intel.com> writes:

> @@ -3091,61 +3231,55 @@ static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx,
>  }
>  
>  static void
> -ctx_pinned_sched_in(struct perf_event_context *ctx,
> -		    struct perf_cpu_context *cpuctx)
> +ctx_pinned_sched_in(struct perf_event *event,
> +		   struct perf_cpu_context *cpuctx,
> +		   struct perf_event_context *ctx)

If you're doing this, you also need to rename the function, because it
now schedules in one event and not a context. But better just keep it as
is.

>  {
> -	struct perf_event *event;
> -
> -	list_for_each_entry(event, &ctx->pinned_groups, group_entry) {

Why not put your new iterator here in place of the old one, instead of
moving things around? Because what follows is hard to read and is also
completely unnecessary:

> -		if (event->state <= PERF_EVENT_STATE_OFF)
> -			continue;
> -		if (!event_filter_match(event))
> -			continue;
> +	if (event->state <= PERF_EVENT_STATE_OFF)
> +		return;
> +	if (!event_filter_match(event))
> +		return;

like this,

>  
> -		/* may need to reset tstamp_enabled */
> -		if (is_cgroup_event(event))
> -			perf_cgroup_mark_enabled(event, ctx);
> +	/* may need to reset tstamp_enabled */
> +	if (is_cgroup_event(event))
> +		perf_cgroup_mark_enabled(event, ctx);

or this

>  
> -		if (group_can_go_on(event, cpuctx, 1))
> -			group_sched_in(event, cpuctx, ctx);
> +	if (group_can_go_on(event, cpuctx, 1))
> +		group_sched_in(event, cpuctx, ctx);

etc, etc.

> @@ -3156,7 +3290,8 @@ ctx_sched_in(struct perf_event_context *ctx,
>  	     struct task_struct *task)
>  {
>  	int is_active = ctx->is_active;
> -	u64 now;

Why?

> +	struct perf_event *event;
> +	struct rb_node *node;
>  
>  	lockdep_assert_held(&ctx->lock);
>  
> @@ -3175,7 +3310,7 @@ ctx_sched_in(struct perf_event_context *ctx,
>  
>  	if (is_active & EVENT_TIME) {
>  		/* start ctx time */
> -		now = perf_clock();
> +		u64 now = perf_clock();

Why?

>  		ctx->timestamp = now;
>  		perf_cgroup_set_timestamp(task, ctx);
>  	}
> @@ -3185,11 +3320,19 @@ ctx_sched_in(struct perf_event_context *ctx,
>  	 * in order to give them the best chance of going on.
>  	 */
>  	if (is_active & EVENT_PINNED)
> -		ctx_pinned_sched_in(ctx, cpuctx);
> +		perf_event_groups_for_each(event, node,
> +				&ctx->pinned_groups, group_node,
> +				group_list, group_entry)
> +			ctx_pinned_sched_in(event, cpuctx, ctx);

So this perf_event_groups_for_each() can just move into
ctx_*_sched_in(), can't it?

Regards,
--
Alex

[toc] | [prev] | [next] | [standalone]

#1718540 — Re: [PATCH v7 1/2] perf/core: use rb trees for pinned/flexible groups

On 23.08.2017 14:17, Alexander Shishkin wrote:
> Alexey Budankov <alexey.budankov@linux.intel.com> writes:
> 
>> @@ -3091,61 +3231,55 @@ static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx,
>>  }
>>  
>>  static void
>> -ctx_pinned_sched_in(struct perf_event_context *ctx,
>> -		    struct perf_cpu_context *cpuctx)
>> +ctx_pinned_sched_in(struct perf_event *event,
>> +		   struct perf_cpu_context *cpuctx,
>> +		   struct perf_event_context *ctx)
> 
> If you're doing this, you also need to rename the function, because it
> now schedules in one event and not a context. But better just keep it as
> is.
> 
>>  {
>> -	struct perf_event *event;
>> -
>> -	list_for_each_entry(event, &ctx->pinned_groups, group_entry) {
> 
> Why not put your new iterator here in place of the old one, instead of
> moving things around? Because what follows is hard to read and is also
> completely unnecessary:
> 
>> -		if (event->state <= PERF_EVENT_STATE_OFF)
>> -			continue;
>> -		if (!event_filter_match(event))
>> -			continue;
>> +	if (event->state <= PERF_EVENT_STATE_OFF)
>> +		return;
>> +	if (!event_filter_match(event))
>> +		return;
> 
> like this,
> 
>>  
>> -		/* may need to reset tstamp_enabled */
>> -		if (is_cgroup_event(event))
>> -			perf_cgroup_mark_enabled(event, ctx);
>> +	/* may need to reset tstamp_enabled */
>> +	if (is_cgroup_event(event))
>> +		perf_cgroup_mark_enabled(event, ctx);
> 
> or this
> 
>>  
>> -		if (group_can_go_on(event, cpuctx, 1))
>> -			group_sched_in(event, cpuctx, ctx);
>> +	if (group_can_go_on(event, cpuctx, 1))
>> +		group_sched_in(event, cpuctx, ctx);
> 
> etc, etc.
> 
>> @@ -3156,7 +3290,8 @@ ctx_sched_in(struct perf_event_context *ctx,
>>  	     struct task_struct *task)
>>  {
>>  	int is_active = ctx->is_active;
>> -	u64 now;
> 
> Why?

Shortened the scope/span/life time of this variable.
Declared, defined and initialized closer to the place of employment.

> 
>> +	struct perf_event *event;
>> +	struct rb_node *node;
>>  
>>  	lockdep_assert_held(&ctx->lock);
>>  
>> @@ -3175,7 +3310,7 @@ ctx_sched_in(struct perf_event_context *ctx,
>>  
>>  	if (is_active & EVENT_TIME) {
>>  		/* start ctx time */
>> -		now = perf_clock();
>> +		u64 now = perf_clock();
> 
> Why?> 
>>  		ctx->timestamp = now;
>>  		perf_cgroup_set_timestamp(task, ctx);
>>  	}
>> @@ -3185,11 +3320,19 @@ ctx_sched_in(struct perf_event_context *ctx,
>>  	 * in order to give them the best chance of going on.
>>  	 */
>>  	if (is_active & EVENT_PINNED)
>> -		ctx_pinned_sched_in(ctx, cpuctx);
>> +		perf_event_groups_for_each(event, node,
>> +				&ctx->pinned_groups, group_node,
>> +				group_list, group_entry)
>> +			ctx_pinned_sched_in(event, cpuctx, ctx);
> 
> So this perf_event_groups_for_each() can just move into
> ctx_*_sched_in(), can't it?

Yes. Makes sense. Addressed it in v8. Thanks!

> 
> Regards,
> --
> Alex
> 

[toc] | [prev] | [next] | [standalone]

#1714576 — [PATCH v7 2/2] perf/core: add mux switch to skip to the current CPU's events list on mux interrupt

This patch implements mux switch that triggers skipping to the 
current CPU's events list at mulitplexing hrtimer interrupt 
handler as well as adoption of the switch in the existing 
implementation.

perf_event_groups_iterate_cpu() API is introduced to implement 
iteration thru the certain CPU groups list skipping groups 
allocated for the other CPUs.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 kernel/events/core.c | 193 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 137 insertions(+), 56 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 08ccfb2..aeb0f81 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -556,11 +556,11 @@ void perf_sample_event_took(u64 sample_len_ns)
 static atomic64_t perf_event_id;
 
 static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx,
-			      enum event_type_t event_type);
+			      enum event_type_t event_type, int mux);
 
 static void cpu_ctx_sched_in(struct perf_cpu_context *cpuctx,
 			     enum event_type_t event_type,
-			     struct task_struct *task);
+			     struct task_struct *task, int mux);
 
 static void update_context_time(struct perf_event_context *ctx);
 static u64 perf_event_time(struct perf_event *event);
@@ -702,6 +702,7 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
 	struct perf_cpu_context *cpuctx;
 	struct list_head *list;
 	unsigned long flags;
+	int mux = 0;
 
 	/*
 	 * Disable interrupts and preemption to avoid this CPU's
@@ -717,7 +718,7 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
 		perf_pmu_disable(cpuctx->ctx.pmu);
 
 		if (mode & PERF_CGROUP_SWOUT) {
-			cpu_ctx_sched_out(cpuctx, EVENT_ALL);
+			cpu_ctx_sched_out(cpuctx, EVENT_ALL, mux);
 			/*
 			 * must not be done before ctxswout due
 			 * to event_filter_match() in event_sched_out()
@@ -736,7 +737,7 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
 			 */
 			cpuctx->cgrp = perf_cgroup_from_task(task,
 							     &cpuctx->ctx);
-			cpu_ctx_sched_in(cpuctx, EVENT_ALL, task);
+			cpu_ctx_sched_in(cpuctx, EVENT_ALL, task, mux);
 		}
 		perf_pmu_enable(cpuctx->ctx.pmu);
 		perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
@@ -1613,8 +1614,16 @@ perf_event_groups_rotate(struct rb_root *groups, int cpu)
  */
 #define perf_event_groups_for_each(event, iter, tree, node, list, link) \
 	   for (iter = rb_first(tree); iter; iter = rb_next(iter))	\
-		list_for_each_entry(event, &(rb_entry(iter,		\
-			typeof(*event), node)->list), link)
+		list_for_each_entry(event, &(rb_entry(iter, 		\
+				typeof(*event), node)->list), link)
+
+/*
+ * Iterate event groups related to specific cpu.
+ */
+#define perf_event_groups_for_each_cpu(event, cpu, tree, list, link)	\
+		list = perf_event_groups_get_list(tree, cpu);		\
+		if (list)						\
+			list_for_each_entry(event, list, link)
 
 /*
  * Add a event from the lists for its context.
@@ -2397,36 +2406,38 @@ static void add_event_to_ctx(struct perf_event *event,
 
 static void ctx_sched_out(struct perf_event_context *ctx,
 			  struct perf_cpu_context *cpuctx,
-			  enum event_type_t event_type);
+			  enum event_type_t event_type, int mux);
 static void
 ctx_sched_in(struct perf_event_context *ctx,
 	     struct perf_cpu_context *cpuctx,
 	     enum event_type_t event_type,
-	     struct task_struct *task);
+	     struct task_struct *task, int mux);
 
 static void task_ctx_sched_out(struct perf_cpu_context *cpuctx,
 			       struct perf_event_context *ctx,
 			       enum event_type_t event_type)
 {
+	int mux = 0;
+
 	if (!cpuctx->task_ctx)
 		return;
 
 	if (WARN_ON_ONCE(ctx != cpuctx->task_ctx))
 		return;
 
-	ctx_sched_out(ctx, cpuctx, event_type);
+	ctx_sched_out(ctx, cpuctx, event_type, mux);
 }
 
 static void perf_event_sched_in(struct perf_cpu_context *cpuctx,
 				struct perf_event_context *ctx,
-				struct task_struct *task)
+				struct task_struct *task, int mux)
 {
-	cpu_ctx_sched_in(cpuctx, EVENT_PINNED, task);
+	cpu_ctx_sched_in(cpuctx, EVENT_PINNED, task, mux);
 	if (ctx)
-		ctx_sched_in(ctx, cpuctx, EVENT_PINNED, task);
-	cpu_ctx_sched_in(cpuctx, EVENT_FLEXIBLE, task);
+		ctx_sched_in(ctx, cpuctx, EVENT_PINNED, task, mux);
+	cpu_ctx_sched_in(cpuctx, EVENT_FLEXIBLE, task, mux);
 	if (ctx)
-		ctx_sched_in(ctx, cpuctx, EVENT_FLEXIBLE, task);
+		ctx_sched_in(ctx, cpuctx, EVENT_FLEXIBLE, task, mux);
 }
 
 /*
@@ -2450,6 +2461,7 @@ static void ctx_resched(struct perf_cpu_context *cpuctx,
 {
 	enum event_type_t ctx_event_type = event_type & EVENT_ALL;
 	bool cpu_event = !!(event_type & EVENT_CPU);
+	int mux = 0;
 
 	/*
 	 * If pinned groups are involved, flexible groups also need to be
@@ -2470,11 +2482,11 @@ static void ctx_resched(struct perf_cpu_context *cpuctx,
 	 *  - otherwise, do nothing more.
 	 */
 	if (cpu_event)
-		cpu_ctx_sched_out(cpuctx, ctx_event_type);
+		cpu_ctx_sched_out(cpuctx, ctx_event_type, mux);
 	else if (ctx_event_type & EVENT_PINNED)
-		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
+		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE, mux);
 
-	perf_event_sched_in(cpuctx, task_ctx, current);
+	perf_event_sched_in(cpuctx, task_ctx, current, mux);
 	perf_pmu_enable(cpuctx->ctx.pmu);
 }
 
@@ -2491,7 +2503,7 @@ static int  __perf_install_in_context(void *info)
 	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
 	struct perf_event_context *task_ctx = cpuctx->task_ctx;
 	bool reprogram = true;
-	int ret = 0;
+	int ret = 0, mux =0;
 
 	raw_spin_lock(&cpuctx->ctx.lock);
 	if (ctx->task) {
@@ -2518,7 +2530,7 @@ static int  __perf_install_in_context(void *info)
 	}
 
 	if (reprogram) {
-		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
+		ctx_sched_out(ctx, cpuctx, EVENT_TIME, mux);
 		add_event_to_ctx(event, ctx);
 		ctx_resched(cpuctx, task_ctx, get_event_type(event));
 	} else {
@@ -2655,13 +2667,14 @@ static void __perf_event_enable(struct perf_event *event,
 {
 	struct perf_event *leader = event->group_leader;
 	struct perf_event_context *task_ctx;
+	int mux = 0;
 
 	if (event->state >= PERF_EVENT_STATE_INACTIVE ||
 	    event->state <= PERF_EVENT_STATE_ERROR)
 		return;
 
 	if (ctx->is_active)
-		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
+		ctx_sched_out(ctx, cpuctx, EVENT_TIME, mux);
 
 	__perf_event_mark_enabled(event);
 
@@ -2671,7 +2684,7 @@ static void __perf_event_enable(struct perf_event *event,
 	if (!event_filter_match(event)) {
 		if (is_cgroup_event(event))
 			perf_cgroup_defer_enabled(event);
-		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
+		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current, mux);
 		return;
 	}
 
@@ -2680,7 +2693,7 @@ static void __perf_event_enable(struct perf_event *event,
 	 * then don't put it on unless the group is on.
 	 */
 	if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE) {
-		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
+		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current, mux);
 		return;
 	}
 
@@ -2876,11 +2889,13 @@ EXPORT_SYMBOL_GPL(perf_event_refresh);
 
 static void ctx_sched_out(struct perf_event_context *ctx,
 			  struct perf_cpu_context *cpuctx,
-			  enum event_type_t event_type)
+			  enum event_type_t event_type, int mux)
 {
 	int is_active = ctx->is_active;
+	struct list_head *group_list;
 	struct perf_event *event;
 	struct rb_node *node;
+	int sw = -1, cpu = smp_processor_id();
 	lockdep_assert_held(&ctx->lock);
 
 	if (likely(!ctx->nr_events)) {
@@ -2926,17 +2941,47 @@ static void ctx_sched_out(struct perf_event_context *ctx,
 
 	perf_pmu_disable(ctx->pmu);
 
-	if (is_active & EVENT_PINNED)
-		perf_event_groups_for_each(event, node,
-				&ctx->pinned_groups, group_node,
-				group_list, group_entry)
-			group_sched_out(event, cpuctx, ctx);
+	if (is_active & EVENT_PINNED) {
+		if (mux) {
+			perf_event_groups_for_each_cpu(event, cpu,
+					&ctx->pinned_groups,
+					group_list, group_entry) {
+					group_sched_out(event, cpuctx, ctx);
+			}
+			perf_event_groups_for_each_cpu(event, sw,
+					&ctx->pinned_groups,
+					group_list, group_entry) {
+					group_sched_out(event, cpuctx, ctx);
+			}
+		} else {
+			perf_event_groups_for_each(event, node,
+					&ctx->pinned_groups, group_node,
+					group_list, group_entry) {
+					group_sched_out(event, cpuctx, ctx);
+			}
+		}
+	}
 
-	if (is_active & EVENT_FLEXIBLE)
-		perf_event_groups_for_each(event, node,
-				&ctx->flexible_groups, group_node,
-				group_list, group_entry)
-			group_sched_out(event, cpuctx, ctx);
+	if (is_active & EVENT_FLEXIBLE) {
+		if (mux) {
+			perf_event_groups_for_each_cpu(event, cpu,
+					&ctx->flexible_groups,
+					group_list, group_entry) {
+					group_sched_out(event, cpuctx, ctx);
+			}
+			perf_event_groups_for_each_cpu(event, sw,
+					&ctx->flexible_groups,
+					group_list, group_entry) {
+					group_sched_out(event, cpuctx, ctx);
+			}
+		} else {
+			perf_event_groups_for_each(event, node,
+					&ctx->flexible_groups, group_node,
+					group_list, group_entry) {
+					group_sched_out(event, cpuctx, ctx);
+			}
+		}
+	}
 
 	perf_pmu_enable(ctx->pmu);
 }
@@ -3225,9 +3270,9 @@ void __perf_event_task_sched_out(struct task_struct *task,
  * Called with IRQs disabled
  */
 static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx,
-			      enum event_type_t event_type)
+			      enum event_type_t event_type, int mux)
 {
-	ctx_sched_out(&cpuctx->ctx, cpuctx, event_type);
+	ctx_sched_out(&cpuctx->ctx, cpuctx, event_type, mux);
 }
 
 static void
@@ -3287,11 +3332,13 @@ static void
 ctx_sched_in(struct perf_event_context *ctx,
 	     struct perf_cpu_context *cpuctx,
 	     enum event_type_t event_type,
-	     struct task_struct *task)
+	     struct task_struct *task, int mux)
 {
 	int is_active = ctx->is_active;
+	struct list_head *group_list;
 	struct perf_event *event;
 	struct rb_node *node;
+	int sw = -1, cpu = smp_processor_id();
 
 	lockdep_assert_held(&ctx->lock);
 
@@ -3319,35 +3366,69 @@ ctx_sched_in(struct perf_event_context *ctx,
 	 * First go through the list and put on any pinned groups
 	 * in order to give them the best chance of going on.
 	 */
-	if (is_active & EVENT_PINNED)
-		perf_event_groups_for_each(event, node,
-				&ctx->pinned_groups, group_node,
-				group_list, group_entry)
-			ctx_pinned_sched_in(event, cpuctx, ctx);
+	if (is_active & EVENT_PINNED) {
+		if (mux) {
+			perf_event_groups_for_each_cpu(event, sw,
+					&ctx->pinned_groups,
+					group_list, group_entry) {
+					ctx_pinned_sched_in(event, cpuctx, ctx);
+			}
+			perf_event_groups_for_each_cpu(event, cpu,
+					&ctx->pinned_groups,
+					group_list, group_entry) {
+					ctx_pinned_sched_in(event, cpuctx, ctx);
+			}
+		} else {
+			perf_event_groups_for_each(event, node,
+					&ctx->pinned_groups, group_node,
+					group_list, group_entry) {
+					ctx_pinned_sched_in(event, cpuctx, ctx);
+			}
+		}
+	}
 
 	/* Then walk through the lower prio flexible groups */
 	if (is_active & EVENT_FLEXIBLE) {
 		int can_add_hw = 1;
-		perf_event_groups_for_each(event, node,
-				&ctx->flexible_groups, group_node,
-				group_list, group_entry)
-			ctx_flexible_sched_in(event, cpuctx, ctx, &can_add_hw);
+		if (mux) {
+			perf_event_groups_for_each_cpu(event, sw,
+					&ctx->flexible_groups,
+					group_list, group_entry) {
+					ctx_flexible_sched_in(event, cpuctx,
+							ctx, &can_add_hw);
+			}
+			can_add_hw = 1;
+			perf_event_groups_for_each_cpu(event, cpu,
+					&ctx->flexible_groups,
+					group_list, group_entry) {
+					ctx_flexible_sched_in(event, cpuctx,
+							ctx, &can_add_hw);
+			}
+		} else {
+			perf_event_groups_for_each(event, node,
+					&ctx->flexible_groups, group_node,
+					group_list, group_entry) {
+					ctx_flexible_sched_in(event, cpuctx,
+							ctx, &can_add_hw);
+			}
+		}
 	}
 }
 
 static void cpu_ctx_sched_in(struct perf_cpu_context *cpuctx,
 			     enum event_type_t event_type,
-			     struct task_struct *task)
+			     struct task_struct *task, int mux)
 {
 	struct perf_event_context *ctx = &cpuctx->ctx;
 
-	ctx_sched_in(ctx, cpuctx, event_type, task);
+	ctx_sched_in(ctx, cpuctx, event_type, task, mux);
 }
 
 static void perf_event_context_sched_in(struct perf_event_context *ctx,
 					struct task_struct *task)
 {
 	struct perf_cpu_context *cpuctx;
+	int mux = 0;
 
 	cpuctx = __get_cpu_context(ctx);
 	if (cpuctx->task_ctx == ctx)
@@ -3371,8 +3452,8 @@ static void perf_event_context_sched_in(struct perf_event_context *ctx,
 	 * events, no need to flip the cpuctx's events around.
 	 */
 	if (!RB_EMPTY_ROOT(&ctx->pinned_groups))
-		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
-	perf_event_sched_in(cpuctx, ctx, task);
+		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE, mux);
+	perf_event_sched_in(cpuctx, ctx, task, mux);
 	perf_pmu_enable(ctx->pmu);
 
 unlock:
@@ -3618,7 +3699,7 @@ static void rotate_ctx(struct perf_event_context *ctx)
 static int perf_rotate_context(struct perf_cpu_context *cpuctx)
 {
 	struct perf_event_context *ctx = NULL;
-	int rotate = 0;
+	int rotate = 0, mux = 1;
 
 	if (cpuctx->ctx.nr_events) {
 		if (cpuctx->ctx.nr_events != cpuctx->ctx.nr_active)
@@ -3637,15 +3718,15 @@ static int perf_rotate_context(struct perf_cpu_context *cpuctx)
 	perf_ctx_lock(cpuctx, cpuctx->task_ctx);
 	perf_pmu_disable(cpuctx->ctx.pmu);
 
-	cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
+	cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE, mux);
 	if (ctx)
-		ctx_sched_out(ctx, cpuctx, EVENT_FLEXIBLE);
+		ctx_sched_out(ctx, cpuctx, EVENT_FLEXIBLE, mux);
 
 	rotate_ctx(&cpuctx->ctx);
 	if (ctx)
 		rotate_ctx(ctx);
 
-	perf_event_sched_in(cpuctx, ctx, current);
+	perf_event_sched_in(cpuctx, ctx, current, mux);
 
 	perf_pmu_enable(cpuctx->ctx.pmu);
 	perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
@@ -3696,7 +3777,7 @@ static void perf_event_enable_on_exec(int ctxn)
 	struct perf_cpu_context *cpuctx;
 	struct perf_event *event;
 	unsigned long flags;
-	int enabled = 0;
+	int enabled = 0, mux = 0;
 
 	local_irq_save(flags);
 	ctx = current->perf_event_ctxp[ctxn];
@@ -3705,7 +3786,7 @@ static void perf_event_enable_on_exec(int ctxn)
 
 	cpuctx = __get_cpu_context(ctx);
 	perf_ctx_lock(cpuctx, ctx);
-	ctx_sched_out(ctx, cpuctx, EVENT_TIME);
+	ctx_sched_out(ctx, cpuctx, EVENT_TIME, mux);
 	list_for_each_entry(event, &ctx->event_list, event_entry) {
 		enabled |= event_enable_on_exec(event, ctx);
 		event_type |= get_event_type(event);
@@ -3718,7 +3799,7 @@ static void perf_event_enable_on_exec(int ctxn)
 		clone_ctx = unclone_ctx(ctx);
 		ctx_resched(cpuctx, ctx, event_type);
 	} else {
-		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
+		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current, mux);
 	}
 	perf_ctx_unlock(cpuctx, ctx);
 

[toc] | [prev] | [next] | [standalone]

#1718281 — Re: [PATCH v7 2/2] perf/core: add mux switch to skip to the current CPU's events list on mux interrupt

Alexey Budankov <alexey.budankov@linux.intel.com> writes:

> This patch implements mux switch that triggers skipping to the 
> current CPU's events list at mulitplexing hrtimer interrupt 
> handler as well as adoption of the switch in the existing 
> implementation.
>
> perf_event_groups_iterate_cpu() API is introduced to implement 
> iteration thru the certain CPU groups list skipping groups 

"through"

> allocated for the other CPUs.
>
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> ---
>  kernel/events/core.c | 193 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 137 insertions(+), 56 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 08ccfb2..aeb0f81 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -556,11 +556,11 @@ void perf_sample_event_took(u64 sample_len_ns)
>  static atomic64_t perf_event_id;
>  
>  static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx,
> -			      enum event_type_t event_type);
> +			      enum event_type_t event_type, int mux);
>  
>  static void cpu_ctx_sched_in(struct perf_cpu_context *cpuctx,
>  			     enum event_type_t event_type,
> -			     struct task_struct *task);
> +			     struct task_struct *task, int mux);
>  
>  static void update_context_time(struct perf_event_context *ctx);
>  static u64 perf_event_time(struct perf_event *event);
> @@ -702,6 +702,7 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
>  	struct perf_cpu_context *cpuctx;
>  	struct list_head *list;
>  	unsigned long flags;
> +	int mux = 0;
>  
>  	/*
>  	 * Disable interrupts and preemption to avoid this CPU's
> @@ -717,7 +718,7 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
>  		perf_pmu_disable(cpuctx->ctx.pmu);
>  
>  		if (mode & PERF_CGROUP_SWOUT) {
> -			cpu_ctx_sched_out(cpuctx, EVENT_ALL);
> +			cpu_ctx_sched_out(cpuctx, EVENT_ALL, mux);
>  			/*
>  			 * must not be done before ctxswout due
>  			 * to event_filter_match() in event_sched_out()
> @@ -736,7 +737,7 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
>  			 */
>  			cpuctx->cgrp = perf_cgroup_from_task(task,
>  							     &cpuctx->ctx);
> -			cpu_ctx_sched_in(cpuctx, EVENT_ALL, task);
> +			cpu_ctx_sched_in(cpuctx, EVENT_ALL, task, mux);

'mux' is always zero in this function, isn't it?

>  		}
>  		perf_pmu_enable(cpuctx->ctx.pmu);
>  		perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
> @@ -1613,8 +1614,16 @@ perf_event_groups_rotate(struct rb_root *groups, int cpu)
>   */
>  #define perf_event_groups_for_each(event, iter, tree, node, list, link) \
>  	   for (iter = rb_first(tree); iter; iter = rb_next(iter))	\
> -		list_for_each_entry(event, &(rb_entry(iter,		\
> -			typeof(*event), node)->list), link)
> +		list_for_each_entry(event, &(rb_entry(iter, 		\
> +				typeof(*event), node)->list), link)

Is this an indentation change? What is it doing here?

> +
> +/*
> + * Iterate event groups related to specific cpu.
> + */
> +#define perf_event_groups_for_each_cpu(event, cpu, tree, list, link)	\
> +		list = perf_event_groups_get_list(tree, cpu);		\
> +		if (list)						\
> +			list_for_each_entry(event, list, link)

..or not, if there's no list.

>  
>  /*
>   * Add a event from the lists for its context.
> @@ -2397,36 +2406,38 @@ static void add_event_to_ctx(struct perf_event *event,
>  
>  static void ctx_sched_out(struct perf_event_context *ctx,
>  			  struct perf_cpu_context *cpuctx,
> -			  enum event_type_t event_type);
> +			  enum event_type_t event_type, int mux);
>  static void
>  ctx_sched_in(struct perf_event_context *ctx,
>  	     struct perf_cpu_context *cpuctx,
>  	     enum event_type_t event_type,
> -	     struct task_struct *task);
> +	     struct task_struct *task, int mux);
>  
>  static void task_ctx_sched_out(struct perf_cpu_context *cpuctx,
>  			       struct perf_event_context *ctx,
>  			       enum event_type_t event_type)
>  {
> +	int mux = 0;
> +
>  	if (!cpuctx->task_ctx)
>  		return;
>  
>  	if (WARN_ON_ONCE(ctx != cpuctx->task_ctx))
>  		return;
>  
> -	ctx_sched_out(ctx, cpuctx, event_type);
> +	ctx_sched_out(ctx, cpuctx, event_type, mux);

Just use 0.

>  }
>  
>  static void perf_event_sched_in(struct perf_cpu_context *cpuctx,
>  				struct perf_event_context *ctx,
> -				struct task_struct *task)
> +				struct task_struct *task, int mux)
>  {
> -	cpu_ctx_sched_in(cpuctx, EVENT_PINNED, task);
> +	cpu_ctx_sched_in(cpuctx, EVENT_PINNED, task, mux);
>  	if (ctx)
> -		ctx_sched_in(ctx, cpuctx, EVENT_PINNED, task);
> -	cpu_ctx_sched_in(cpuctx, EVENT_FLEXIBLE, task);
> +		ctx_sched_in(ctx, cpuctx, EVENT_PINNED, task, mux);
> +	cpu_ctx_sched_in(cpuctx, EVENT_FLEXIBLE, task, mux);
>  	if (ctx)
> -		ctx_sched_in(ctx, cpuctx, EVENT_FLEXIBLE, task);
> +		ctx_sched_in(ctx, cpuctx, EVENT_FLEXIBLE, task, mux);
>  }
>  
>  /*
> @@ -2450,6 +2461,7 @@ static void ctx_resched(struct perf_cpu_context *cpuctx,
>  {
>  	enum event_type_t ctx_event_type = event_type & EVENT_ALL;
>  	bool cpu_event = !!(event_type & EVENT_CPU);
> +	int mux = 0;
>  
>  	/*
>  	 * If pinned groups are involved, flexible groups also need to be
> @@ -2470,11 +2482,11 @@ static void ctx_resched(struct perf_cpu_context *cpuctx,
>  	 *  - otherwise, do nothing more.
>  	 */
>  	if (cpu_event)
> -		cpu_ctx_sched_out(cpuctx, ctx_event_type);
> +		cpu_ctx_sched_out(cpuctx, ctx_event_type, mux);
>  	else if (ctx_event_type & EVENT_PINNED)
> -		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
> +		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE, mux);
>  
> -	perf_event_sched_in(cpuctx, task_ctx, current);
> +	perf_event_sched_in(cpuctx, task_ctx, current, mux);

Also mux==0 in all cases in this function.

>  	perf_pmu_enable(cpuctx->ctx.pmu);
>  }
>  
> @@ -2491,7 +2503,7 @@ static int  __perf_install_in_context(void *info)
>  	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
>  	struct perf_event_context *task_ctx = cpuctx->task_ctx;
>  	bool reprogram = true;
> -	int ret = 0;
> +	int ret = 0, mux =0;
>  
>  	raw_spin_lock(&cpuctx->ctx.lock);
>  	if (ctx->task) {
> @@ -2518,7 +2530,7 @@ static int  __perf_install_in_context(void *info)
>  	}
>  
>  	if (reprogram) {
> -		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
> +		ctx_sched_out(ctx, cpuctx, EVENT_TIME, mux);
>  		add_event_to_ctx(event, ctx);
>  		ctx_resched(cpuctx, task_ctx, get_event_type(event));
>  	} else {
> @@ -2655,13 +2667,14 @@ static void __perf_event_enable(struct perf_event *event,
>  {
>  	struct perf_event *leader = event->group_leader;
>  	struct perf_event_context *task_ctx;
> +	int mux = 0;
>  
>  	if (event->state >= PERF_EVENT_STATE_INACTIVE ||
>  	    event->state <= PERF_EVENT_STATE_ERROR)
>  		return;
>  
>  	if (ctx->is_active)
> -		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
> +		ctx_sched_out(ctx, cpuctx, EVENT_TIME, mux);
>  
>  	__perf_event_mark_enabled(event);
>  
> @@ -2671,7 +2684,7 @@ static void __perf_event_enable(struct perf_event *event,
>  	if (!event_filter_match(event)) {
>  		if (is_cgroup_event(event))
>  			perf_cgroup_defer_enabled(event);
> -		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
> +		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current, mux);
>  		return;
>  	}
>  
> @@ -2680,7 +2693,7 @@ static void __perf_event_enable(struct perf_event *event,
>  	 * then don't put it on unless the group is on.
>  	 */
>  	if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE) {
> -		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
> +		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current, mux);

And here.

>  		return;
>  	}
>  
> @@ -2876,11 +2889,13 @@ EXPORT_SYMBOL_GPL(perf_event_refresh);
>  
>  static void ctx_sched_out(struct perf_event_context *ctx,
>  			  struct perf_cpu_context *cpuctx,
> -			  enum event_type_t event_type)
> +			  enum event_type_t event_type, int mux)
>  {
>  	int is_active = ctx->is_active;
> +	struct list_head *group_list;
>  	struct perf_event *event;
>  	struct rb_node *node;
> +	int sw = -1, cpu = smp_processor_id();

Same thing seems to be happening with 'sw'.

>  	lockdep_assert_held(&ctx->lock);
>  
>  	if (likely(!ctx->nr_events)) {
> @@ -2926,17 +2941,47 @@ static void ctx_sched_out(struct perf_event_context *ctx,
>  
>  	perf_pmu_disable(ctx->pmu);
>  
> -	if (is_active & EVENT_PINNED)
> -		perf_event_groups_for_each(event, node,
> -				&ctx->pinned_groups, group_node,
> -				group_list, group_entry)
> -			group_sched_out(event, cpuctx, ctx);
> +	if (is_active & EVENT_PINNED) {
> +		if (mux) {

So it's 'rotate', really.

> +			perf_event_groups_for_each_cpu(event, cpu,
> +					&ctx->pinned_groups,
> +					group_list, group_entry) {
> +					group_sched_out(event, cpuctx, ctx);
> +			}
> +			perf_event_groups_for_each_cpu(event, sw,
> +					&ctx->pinned_groups,
> +					group_list, group_entry) {
> +					group_sched_out(event, cpuctx, ctx);
> +			}
> +		} else {
> +			perf_event_groups_for_each(event, node,
> +					&ctx->pinned_groups, group_node,
> +					group_list, group_entry) {
> +					group_sched_out(event, cpuctx, ctx);
> +			}
> +		}
> +	}
>  
> -	if (is_active & EVENT_FLEXIBLE)
> -		perf_event_groups_for_each(event, node,
> -				&ctx->flexible_groups, group_node,
> -				group_list, group_entry)
> -			group_sched_out(event, cpuctx, ctx);
> +	if (is_active & EVENT_FLEXIBLE) {
> +		if (mux) {
> +			perf_event_groups_for_each_cpu(event, cpu,
> +					&ctx->flexible_groups,
> +					group_list, group_entry) {
> +					group_sched_out(event, cpuctx, ctx);
> +			}
> +			perf_event_groups_for_each_cpu(event, sw,
> +					&ctx->flexible_groups,
> +					group_list, group_entry) {
> +					group_sched_out(event, cpuctx, ctx);
> +			}
> +		} else {
> +			perf_event_groups_for_each(event, node,
> +					&ctx->flexible_groups, group_node,
> +					group_list, group_entry) {
> +					group_sched_out(event, cpuctx, ctx);
> +			}
> +		}
> +	}
>  
>  	perf_pmu_enable(ctx->pmu);
>  }
> @@ -3225,9 +3270,9 @@ void __perf_event_task_sched_out(struct task_struct *task,
>   * Called with IRQs disabled
>   */
>  static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx,
> -			      enum event_type_t event_type)
> +			      enum event_type_t event_type, int mux)
>  {
> -	ctx_sched_out(&cpuctx->ctx, cpuctx, event_type);
> +	ctx_sched_out(&cpuctx->ctx, cpuctx, event_type, mux);
>  }
>  
>  static void
> @@ -3287,11 +3332,13 @@ static void
>  ctx_sched_in(struct perf_event_context *ctx,
>  	     struct perf_cpu_context *cpuctx,
>  	     enum event_type_t event_type,
> -	     struct task_struct *task)
> +	     struct task_struct *task, int mux)
>  {
>  	int is_active = ctx->is_active;
> +	struct list_head *group_list;
>  	struct perf_event *event;
>  	struct rb_node *node;
> +	int sw = -1, cpu = smp_processor_id();
>  
>  	lockdep_assert_held(&ctx->lock);
>  
> @@ -3319,35 +3366,69 @@ ctx_sched_in(struct perf_event_context *ctx,
>  	 * First go through the list and put on any pinned groups
>  	 * in order to give them the best chance of going on.
>  	 */
> -	if (is_active & EVENT_PINNED)
> -		perf_event_groups_for_each(event, node,
> -				&ctx->pinned_groups, group_node,
> -				group_list, group_entry)
> -			ctx_pinned_sched_in(event, cpuctx, ctx);
> +	if (is_active & EVENT_PINNED) {
> +		if (mux) {
> +			perf_event_groups_for_each_cpu(event, sw,
> +					&ctx->pinned_groups,
> +					group_list, group_entry) {
> +					ctx_pinned_sched_in(event, cpuctx, ctx);
> +			}
> +			perf_event_groups_for_each_cpu(event, cpu,
> +					&ctx->pinned_groups,
> +					group_list, group_entry) {
> +					ctx_pinned_sched_in(event, cpuctx, ctx);
> +			}
> +		} else {
> +			perf_event_groups_for_each(event, node,
> +					&ctx->pinned_groups, group_node,
> +					group_list, group_entry) {
> +					ctx_pinned_sched_in(event, cpuctx, ctx);
> +			}
> +		}
> +	}
>  
>  	/* Then walk through the lower prio flexible groups */
>  	if (is_active & EVENT_FLEXIBLE) {
>  		int can_add_hw = 1;
> -		perf_event_groups_for_each(event, node,
> -				&ctx->flexible_groups, group_node,
> -				group_list, group_entry)
> -			ctx_flexible_sched_in(event, cpuctx, ctx, &can_add_hw);
> +		if (mux) {
> +			perf_event_groups_for_each_cpu(event, sw,
> +					&ctx->flexible_groups,
> +					group_list, group_entry) {
> +					ctx_flexible_sched_in(event, cpuctx,
> +							ctx, &can_add_hw);
> +			}
> +			can_add_hw = 1;
> +			perf_event_groups_for_each_cpu(event, cpu,
> +					&ctx->flexible_groups,
> +					group_list, group_entry) {
> +					ctx_flexible_sched_in(event, cpuctx,
> +							ctx, &can_add_hw);
> +			}
> +		} else {
> +			perf_event_groups_for_each(event, node,
> +					&ctx->flexible_groups, group_node,
> +					group_list, group_entry) {
> +					ctx_flexible_sched_in(event, cpuctx,
> +							ctx, &can_add_hw);
> +			}
> +		}
>  	}
>  }
>  
>  static void cpu_ctx_sched_in(struct perf_cpu_context *cpuctx,
>  			     enum event_type_t event_type,
> -			     struct task_struct *task)
> +			     struct task_struct *task, int mux)
>  {
>  	struct perf_event_context *ctx = &cpuctx->ctx;
>  
> -	ctx_sched_in(ctx, cpuctx, event_type, task);
> +	ctx_sched_in(ctx, cpuctx, event_type, task, mux);
>  }
>  
>  static void perf_event_context_sched_in(struct perf_event_context *ctx,
>  					struct task_struct *task)
>  {
>  	struct perf_cpu_context *cpuctx;
> +	int mux = 0;
>  
>  	cpuctx = __get_cpu_context(ctx);
>  	if (cpuctx->task_ctx == ctx)
> @@ -3371,8 +3452,8 @@ static void perf_event_context_sched_in(struct perf_event_context *ctx,
>  	 * events, no need to flip the cpuctx's events around.
>  	 */
>  	if (!RB_EMPTY_ROOT(&ctx->pinned_groups))
> -		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
> -	perf_event_sched_in(cpuctx, ctx, task);
> +		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE, mux);
> +	perf_event_sched_in(cpuctx, ctx, task, mux);
>  	perf_pmu_enable(ctx->pmu);
>  
>  unlock:
> @@ -3618,7 +3699,7 @@ static void rotate_ctx(struct perf_event_context *ctx)
>  static int perf_rotate_context(struct perf_cpu_context *cpuctx)
>  {
>  	struct perf_event_context *ctx = NULL;
> -	int rotate = 0;
> +	int rotate = 0, mux = 1;
>  
>  	if (cpuctx->ctx.nr_events) {
>  		if (cpuctx->ctx.nr_events != cpuctx->ctx.nr_active)
> @@ -3637,15 +3718,15 @@ static int perf_rotate_context(struct perf_cpu_context *cpuctx)
>  	perf_ctx_lock(cpuctx, cpuctx->task_ctx);
>  	perf_pmu_disable(cpuctx->ctx.pmu);
>  
> -	cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
> +	cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE, mux);

It's '1'.

[toc] | [prev] | [next] | [standalone]

#1718567 — Re: [PATCH v7 2/2] perf/core: add mux switch to skip to the current CPU's events list on mux interrupt

On 23.08.2017 14:54, Alexander Shishkin wrote:
> Alexey Budankov <alexey.budankov@linux.intel.com> writes:
> 
>> This patch implements mux switch that triggers skipping to the 
>> current CPU's events list at mulitplexing hrtimer interrupt 
>> handler as well as adoption of the switch in the existing 
>> implementation.
>>
>> perf_event_groups_iterate_cpu() API is introduced to implement 
>> iteration thru the certain CPU groups list skipping groups 
> 
> "through"
> 
>> allocated for the other CPUs.
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>> ---
>>  kernel/events/core.c | 193 ++++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 137 insertions(+), 56 deletions(-)
>>
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 08ccfb2..aeb0f81 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -556,11 +556,11 @@ void perf_sample_event_took(u64 sample_len_ns)
>>  static atomic64_t perf_event_id;
>>  
>>  static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx,
>> -			      enum event_type_t event_type);
>> +			      enum event_type_t event_type, int mux);
>>  
>>  static void cpu_ctx_sched_in(struct perf_cpu_context *cpuctx,
>>  			     enum event_type_t event_type,
>> -			     struct task_struct *task);
>> +			     struct task_struct *task, int mux);
>>  
>>  static void update_context_time(struct perf_event_context *ctx);
>>  static u64 perf_event_time(struct perf_event *event);
>> @@ -702,6 +702,7 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
>>  	struct perf_cpu_context *cpuctx;
>>  	struct list_head *list;
>>  	unsigned long flags;
>> +	int mux = 0;
>>  
>>  	/*
>>  	 * Disable interrupts and preemption to avoid this CPU's
>> @@ -717,7 +718,7 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
>>  		perf_pmu_disable(cpuctx->ctx.pmu);
>>  
>>  		if (mode & PERF_CGROUP_SWOUT) {
>> -			cpu_ctx_sched_out(cpuctx, EVENT_ALL);
>> +			cpu_ctx_sched_out(cpuctx, EVENT_ALL, mux);
>>  			/*
>>  			 * must not be done before ctxswout due
>>  			 * to event_filter_match() in event_sched_out()
>> @@ -736,7 +737,7 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
>>  			 */
>>  			cpuctx->cgrp = perf_cgroup_from_task(task,
>>  							     &cpuctx->ctx);
>> -			cpu_ctx_sched_in(cpuctx, EVENT_ALL, task);
>> +			cpu_ctx_sched_in(cpuctx, EVENT_ALL, task, mux);
> 
> 'mux' is always zero in this function, isn't it?
> 
>>  		}
>>  		perf_pmu_enable(cpuctx->ctx.pmu);
>>  		perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
>> @@ -1613,8 +1614,16 @@ perf_event_groups_rotate(struct rb_root *groups, int cpu)
>>   */
>>  #define perf_event_groups_for_each(event, iter, tree, node, list, link) \
>>  	   for (iter = rb_first(tree); iter; iter = rb_next(iter))	\
>> -		list_for_each_entry(event, &(rb_entry(iter,		\
>> -			typeof(*event), node)->list), link)
>> +		list_for_each_entry(event, &(rb_entry(iter, 		\
>> +				typeof(*event), node)->list), link)
> 
> Is this an indentation change? What is it doing here?
> 
>> +
>> +/*
>> + * Iterate event groups related to specific cpu.
>> + */
>> +#define perf_event_groups_for_each_cpu(event, cpu, tree, list, link)	\
>> +		list = perf_event_groups_get_list(tree, cpu);		\
>> +		if (list)						\
>> +			list_for_each_entry(event, list, link)
> 
> ..or not, if there's no list.
> 
>>  
>>  /*
>>   * Add a event from the lists for its context.
>> @@ -2397,36 +2406,38 @@ static void add_event_to_ctx(struct perf_event *event,
>>  
>>  static void ctx_sched_out(struct perf_event_context *ctx,
>>  			  struct perf_cpu_context *cpuctx,
>> -			  enum event_type_t event_type);
>> +			  enum event_type_t event_type, int mux);
>>  static void
>>  ctx_sched_in(struct perf_event_context *ctx,
>>  	     struct perf_cpu_context *cpuctx,
>>  	     enum event_type_t event_type,
>> -	     struct task_struct *task);
>> +	     struct task_struct *task, int mux);
>>  
>>  static void task_ctx_sched_out(struct perf_cpu_context *cpuctx,
>>  			       struct perf_event_context *ctx,
>>  			       enum event_type_t event_type)
>>  {
>> +	int mux = 0;
>> +
>>  	if (!cpuctx->task_ctx)
>>  		return;
>>  
>>  	if (WARN_ON_ONCE(ctx != cpuctx->task_ctx))
>>  		return;
>>  
>> -	ctx_sched_out(ctx, cpuctx, event_type);
>> +	ctx_sched_out(ctx, cpuctx, event_type, mux);
> 
> Just use 0.

Well, I intentionally made this ugly switch named over local variable 
to add clarity into the code so I simply wonder - why do you prefer 
the unnamed variant?

> 
>>  }
>>  
>>  static void perf_event_sched_in(struct perf_cpu_context *cpuctx,
>>  				struct perf_event_context *ctx,
>> -				struct task_struct *task)
>> +				struct task_struct *task, int mux)
>>  {
>> -	cpu_ctx_sched_in(cpuctx, EVENT_PINNED, task);
>> +	cpu_ctx_sched_in(cpuctx, EVENT_PINNED, task, mux);
>>  	if (ctx)
>> -		ctx_sched_in(ctx, cpuctx, EVENT_PINNED, task);
>> -	cpu_ctx_sched_in(cpuctx, EVENT_FLEXIBLE, task);
>> +		ctx_sched_in(ctx, cpuctx, EVENT_PINNED, task, mux);
>> +	cpu_ctx_sched_in(cpuctx, EVENT_FLEXIBLE, task, mux);
>>  	if (ctx)
>> -		ctx_sched_in(ctx, cpuctx, EVENT_FLEXIBLE, task);
>> +		ctx_sched_in(ctx, cpuctx, EVENT_FLEXIBLE, task, mux);
>>  }
>>  
>>  /*
>> @@ -2450,6 +2461,7 @@ static void ctx_resched(struct perf_cpu_context *cpuctx,
>>  {
>>  	enum event_type_t ctx_event_type = event_type & EVENT_ALL;
>>  	bool cpu_event = !!(event_type & EVENT_CPU);
>> +	int mux = 0;
>>  
>>  	/*
>>  	 * If pinned groups are involved, flexible groups also need to be
>> @@ -2470,11 +2482,11 @@ static void ctx_resched(struct perf_cpu_context *cpuctx,
>>  	 *  - otherwise, do nothing more.
>>  	 */
>>  	if (cpu_event)
>> -		cpu_ctx_sched_out(cpuctx, ctx_event_type);
>> +		cpu_ctx_sched_out(cpuctx, ctx_event_type, mux);
>>  	else if (ctx_event_type & EVENT_PINNED)
>> -		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
>> +		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE, mux);
>>  
>> -	perf_event_sched_in(cpuctx, task_ctx, current);
>> +	perf_event_sched_in(cpuctx, task_ctx, current, mux);
> 
> Also mux==0 in all cases in this function.
> 
>>  	perf_pmu_enable(cpuctx->ctx.pmu);
>>  }
>>  
>> @@ -2491,7 +2503,7 @@ static int  __perf_install_in_context(void *info)
>>  	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
>>  	struct perf_event_context *task_ctx = cpuctx->task_ctx;
>>  	bool reprogram = true;
>> -	int ret = 0;
>> +	int ret = 0, mux =0;
>>  
>>  	raw_spin_lock(&cpuctx->ctx.lock);
>>  	if (ctx->task) {
>> @@ -2518,7 +2530,7 @@ static int  __perf_install_in_context(void *info)
>>  	}
>>  
>>  	if (reprogram) {
>> -		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
>> +		ctx_sched_out(ctx, cpuctx, EVENT_TIME, mux);
>>  		add_event_to_ctx(event, ctx);
>>  		ctx_resched(cpuctx, task_ctx, get_event_type(event));
>>  	} else {
>> @@ -2655,13 +2667,14 @@ static void __perf_event_enable(struct perf_event *event,
>>  {
>>  	struct perf_event *leader = event->group_leader;
>>  	struct perf_event_context *task_ctx;
>> +	int mux = 0;
>>  
>>  	if (event->state >= PERF_EVENT_STATE_INACTIVE ||
>>  	    event->state <= PERF_EVENT_STATE_ERROR)
>>  		return;
>>  
>>  	if (ctx->is_active)
>> -		ctx_sched_out(ctx, cpuctx, EVENT_TIME);
>> +		ctx_sched_out(ctx, cpuctx, EVENT_TIME, mux);
>>  
>>  	__perf_event_mark_enabled(event);
>>  
>> @@ -2671,7 +2684,7 @@ static void __perf_event_enable(struct perf_event *event,
>>  	if (!event_filter_match(event)) {
>>  		if (is_cgroup_event(event))
>>  			perf_cgroup_defer_enabled(event);
>> -		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
>> +		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current, mux);
>>  		return;
>>  	}
>>  
>> @@ -2680,7 +2693,7 @@ static void __perf_event_enable(struct perf_event *event,
>>  	 * then don't put it on unless the group is on.
>>  	 */
>>  	if (leader != event && leader->state != PERF_EVENT_STATE_ACTIVE) {
>> -		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current);
>> +		ctx_sched_in(ctx, cpuctx, EVENT_TIME, current, mux);
> 
> And here.
> 
>>  		return;
>>  	}
>>  
>> @@ -2876,11 +2889,13 @@ EXPORT_SYMBOL_GPL(perf_event_refresh);
>>  
>>  static void ctx_sched_out(struct perf_event_context *ctx,
>>  			  struct perf_cpu_context *cpuctx,
>> -			  enum event_type_t event_type)
>> +			  enum event_type_t event_type, int mux)
>>  {
>>  	int is_active = ctx->is_active;
>> +	struct list_head *group_list;
>>  	struct perf_event *event;
>>  	struct rb_node *node;
>> +	int sw = -1, cpu = smp_processor_id();
> 
> Same thing seems to be happening with 'sw'.
> 
>>  	lockdep_assert_held(&ctx->lock);
>>  
>>  	if (likely(!ctx->nr_events)) {
>> @@ -2926,17 +2941,47 @@ static void ctx_sched_out(struct perf_event_context *ctx,
>>  
>>  	perf_pmu_disable(ctx->pmu);
>>  
>> -	if (is_active & EVENT_PINNED)
>> -		perf_event_groups_for_each(event, node,
>> -				&ctx->pinned_groups, group_node,
>> -				group_list, group_entry)
>> -			group_sched_out(event, cpuctx, ctx);
>> +	if (is_active & EVENT_PINNED) {
>> +		if (mux) {
> 
> So it's 'rotate', really.

Which 'rotate' do you mean? If the local variable from 
perf_rotate_context() then yes - it may be passed to this function 
call from there as mux value, but logically they are still different.

> 
>> +			perf_event_groups_for_each_cpu(event, cpu,
>> +					&ctx->pinned_groups,
>> +					group_list, group_entry) {
>> +					group_sched_out(event, cpuctx, ctx);
>> +			}
>> +			perf_event_groups_for_each_cpu(event, sw,
>> +					&ctx->pinned_groups,
>> +					group_list, group_entry) {
>> +					group_sched_out(event, cpuctx, ctx);
>> +			}
>> +		} else {
>> +			perf_event_groups_for_each(event, node,
>> +					&ctx->pinned_groups, group_node,
>> +					group_list, group_entry) {
>> +					group_sched_out(event, cpuctx, ctx);
>> +			}
>> +		}
>> +	}
>>  
>> -	if (is_active & EVENT_FLEXIBLE)
>> -		perf_event_groups_for_each(event, node,
>> -				&ctx->flexible_groups, group_node,
>> -				group_list, group_entry)
>> -			group_sched_out(event, cpuctx, ctx);
>> +	if (is_active & EVENT_FLEXIBLE) {
>> +		if (mux) {
>> +			perf_event_groups_for_each_cpu(event, cpu,
>> +					&ctx->flexible_groups,
>> +					group_list, group_entry) {
>> +					group_sched_out(event, cpuctx, ctx);
>> +			}
>> +			perf_event_groups_for_each_cpu(event, sw,
>> +					&ctx->flexible_groups,
>> +					group_list, group_entry) {
>> +					group_sched_out(event, cpuctx, ctx);
>> +			}
>> +		} else {
>> +			perf_event_groups_for_each(event, node,
>> +					&ctx->flexible_groups, group_node,
>> +					group_list, group_entry) {
>> +					group_sched_out(event, cpuctx, ctx);
>> +			}
>> +		}
>> +	}
>>  
>>  	perf_pmu_enable(ctx->pmu);
>>  }
>> @@ -3225,9 +3270,9 @@ void __perf_event_task_sched_out(struct task_struct *task,
>>   * Called with IRQs disabled
>>   */
>>  static void cpu_ctx_sched_out(struct perf_cpu_context *cpuctx,
>> -			      enum event_type_t event_type)
>> +			      enum event_type_t event_type, int mux)
>>  {
>> -	ctx_sched_out(&cpuctx->ctx, cpuctx, event_type);
>> +	ctx_sched_out(&cpuctx->ctx, cpuctx, event_type, mux);
>>  }
>>  
>>  static void
>> @@ -3287,11 +3332,13 @@ static void
>>  ctx_sched_in(struct perf_event_context *ctx,
>>  	     struct perf_cpu_context *cpuctx,
>>  	     enum event_type_t event_type,
>> -	     struct task_struct *task)
>> +	     struct task_struct *task, int mux)
>>  {
>>  	int is_active = ctx->is_active;
>> +	struct list_head *group_list;
>>  	struct perf_event *event;
>>  	struct rb_node *node;
>> +	int sw = -1, cpu = smp_processor_id();
>>  
>>  	lockdep_assert_held(&ctx->lock);
>>  
>> @@ -3319,35 +3366,69 @@ ctx_sched_in(struct perf_event_context *ctx,
>>  	 * First go through the list and put on any pinned groups
>>  	 * in order to give them the best chance of going on.
>>  	 */
>> -	if (is_active & EVENT_PINNED)
>> -		perf_event_groups_for_each(event, node,
>> -				&ctx->pinned_groups, group_node,
>> -				group_list, group_entry)
>> -			ctx_pinned_sched_in(event, cpuctx, ctx);
>> +	if (is_active & EVENT_PINNED) {
>> +		if (mux) {
>> +			perf_event_groups_for_each_cpu(event, sw,
>> +					&ctx->pinned_groups,
>> +					group_list, group_entry) {
>> +					ctx_pinned_sched_in(event, cpuctx, ctx);
>> +			}
>> +			perf_event_groups_for_each_cpu(event, cpu,
>> +					&ctx->pinned_groups,
>> +					group_list, group_entry) {
>> +					ctx_pinned_sched_in(event, cpuctx, ctx);
>> +			}
>> +		} else {
>> +			perf_event_groups_for_each(event, node,
>> +					&ctx->pinned_groups, group_node,
>> +					group_list, group_entry) {
>> +					ctx_pinned_sched_in(event, cpuctx, ctx);
>> +			}
>> +		}
>> +	}
>>  
>>  	/* Then walk through the lower prio flexible groups */
>>  	if (is_active & EVENT_FLEXIBLE) {
>>  		int can_add_hw = 1;
>> -		perf_event_groups_for_each(event, node,
>> -				&ctx->flexible_groups, group_node,
>> -				group_list, group_entry)
>> -			ctx_flexible_sched_in(event, cpuctx, ctx, &can_add_hw);
>> +		if (mux) {
>> +			perf_event_groups_for_each_cpu(event, sw,
>> +					&ctx->flexible_groups,
>> +					group_list, group_entry) {
>> +					ctx_flexible_sched_in(event, cpuctx,
>> +							ctx, &can_add_hw);
>> +			}
>> +			can_add_hw = 1;
>> +			perf_event_groups_for_each_cpu(event, cpu,
>> +					&ctx->flexible_groups,
>> +					group_list, group_entry) {
>> +					ctx_flexible_sched_in(event, cpuctx,
>> +							ctx, &can_add_hw);
>> +			}
>> +		} else {
>> +			perf_event_groups_for_each(event, node,
>> +					&ctx->flexible_groups, group_node,
>> +					group_list, group_entry) {
>> +					ctx_flexible_sched_in(event, cpuctx,
>> +							ctx, &can_add_hw);
>> +			}
>> +		}
>>  	}
>>  }
>>  
>>  static void cpu_ctx_sched_in(struct perf_cpu_context *cpuctx,
>>  			     enum event_type_t event_type,
>> -			     struct task_struct *task)
>> +			     struct task_struct *task, int mux)
>>  {
>>  	struct perf_event_context *ctx = &cpuctx->ctx;
>>  
>> -	ctx_sched_in(ctx, cpuctx, event_type, task);
>> +	ctx_sched_in(ctx, cpuctx, event_type, task, mux);
>>  }
>>  
>>  static void perf_event_context_sched_in(struct perf_event_context *ctx,
>>  					struct task_struct *task)
>>  {
>>  	struct perf_cpu_context *cpuctx;
>> +	int mux = 0;
>>  
>>  	cpuctx = __get_cpu_context(ctx);
>>  	if (cpuctx->task_ctx == ctx)
>> @@ -3371,8 +3452,8 @@ static void perf_event_context_sched_in(struct perf_event_context *ctx,
>>  	 * events, no need to flip the cpuctx's events around.
>>  	 */
>>  	if (!RB_EMPTY_ROOT(&ctx->pinned_groups))
>> -		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
>> -	perf_event_sched_in(cpuctx, ctx, task);
>> +		cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE, mux);
>> +	perf_event_sched_in(cpuctx, ctx, task, mux);
>>  	perf_pmu_enable(ctx->pmu);
>>  
>>  unlock:
>> @@ -3618,7 +3699,7 @@ static void rotate_ctx(struct perf_event_context *ctx)
>>  static int perf_rotate_context(struct perf_cpu_context *cpuctx)
>>  {
>>  	struct perf_event_context *ctx = NULL;
>> -	int rotate = 0;
>> +	int rotate = 0, mux = 1;
>>  
>>  	if (cpuctx->ctx.nr_events) {
>>  		if (cpuctx->ctx.nr_events != cpuctx->ctx.nr_active)
>> @@ -3637,15 +3718,15 @@ static int perf_rotate_context(struct perf_cpu_context *cpuctx)
>>  	perf_ctx_lock(cpuctx, cpuctx->task_ctx);
>>  	perf_pmu_disable(cpuctx->ctx.pmu);
>>  
>> -	cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
>> +	cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE, mux);
> 
> It's '1'.
> 
> 

[toc] | [prev] | [next] | [standalone]

#1717810 — Re: [PATCH v7 0/2] perf/core: addressing 4x slowdown during per-process profiling of STREAM benchmark on Intel Xeon Phi

On Fri, Aug 18, 2017 at 08:17:15AM +0300, Alexey Budankov wrote:
> Hi,

Please don't post new versions in reply to old versions, that gets them
lost in thread sorted views.

> This patch set v7 moves event groups into rb trees and implements 
> skipping to the current CPU's list on hrtimer interrupt.

Does this depend on your timekeeping rework posted in that v6 thread?
If so, I would have expected to see that as part of these patches, if
not, I'm confused, because part of the problem was that we currently
need to update times for events we don't want to schedule etc..

> Events allocated for the same CPU are still kept in a linked list
> of the event directly attached to the tree because it is unclear 
> how to implement fast iteration thru events allocated for 
> the same CPU when they are all attached to a tree employing 
> additional 64bit index as a secondary treee key.

Finding the CPU subtree and rb_next() wasn't good?

[toc] | [prev] | [next] | [standalone]

#1718173 — Re: [PATCH v7 0/2] perf/core: addressing 4x slowdown during per-process profiling of STREAM benchmark on Intel Xeon Phi

On 22.08.2017 23:21, Peter Zijlstra wrote:
> On Fri, Aug 18, 2017 at 08:17:15AM +0300, Alexey Budankov wrote:
>> Hi,
> 
> Please don't post new versions in reply to old versions, that gets them
> lost in thread sorted views.

Accepted. Actually I followed these recommendations:

	https://git-scm.com/docs/git-send-email

and they say it needs to be like this:

[PATCH 0/2] Here is what I did...
  [PATCH 1/2] Clean up and tests
  [PATCH 2/2] Implementation
  [PATCH v2 0/3] Here is a reroll
    [PATCH v2 1/3] Clean up
    [PATCH v2 2/3] New tests
    [PATCH v2 3/3] Implementation

So I made v7 a descendant of v6.

Do you prefer new version be at the same level as the previous 
versions like this?

[PATCH 0/2] Here is what I did...
  [PATCH 1/2] Clean up and tests
  [PATCH 2/2] Implementation
[PATCH v2 0/3] Here is a reroll
  [PATCH v2 1/3] Clean up
  [PATCH v2 2/3] New tests
  [PATCH v2 3/3] Implementation

> 
>> This patch set v7 moves event groups into rb trees and implements 
>> skipping to the current CPU's list on hrtimer interrupt.
> 
> Does this depend on your timekeeping rework posted in that v6 thread?

This v7 includes timekeeping rework so it is complete patch set 
addressing your earlier concerns. The bunch of changes became 
smaller so we are going right way.

I tested v7 thru several nights on Xeon Phi under fuzzer like this:
for ((i=0;i<1000;i=i+1)) do ./perf_fuzzer; done
and there were no crashes or hangs in perf code the machine is still alive.

> If so, I would have expected to see that as part of these patches, if
> not, I'm confused, because part of the problem was that we currently
> need to update times for events we don't want to schedule etc..

Yes, you are right. We need to update times for that events and 
we still do, but on-demand - read() syscall. Doing so we may skip 
slow iterating of the whole bunch of events and get performance boost.

We may skip updating the times every timer interrupt and do it only
on read() call and on thread context switch out when 
update_event_times() is actually called.

> 
>> Events allocated for the same CPU are still kept in a linked list
>> of the event directly attached to the tree because it is unclear 
>> how to implement fast iteration thru events allocated for 
>> the same CPU when they are all attached to a tree employing 
>> additional 64bit index as a secondary treee key.
> 
> Finding the CPU subtree and rb_next() wasn't good?

I implemented the approach you had suggested (as I understood it),
tested it and got results that are drastically different from what 
I am getting for the tree of lists. Specifically I did:

1. keeping all groups in the same single tree by employing a 64-bit index
   additionally to CPU key;
   
2. implementing special _less() function and rotation by re-inserting
   group with incremented index;

3. replacing API with a callback in the signature by a macro
   perf_event_groups_for_each();

Employing all that shrunk the total patch size, however I am still 
struggling with the correctness issues.

Now I figured that not all indexed events are always located under 
the root with the same cpu, and it depends on the order of insertion
e.g. with insertion order 01,02,03,14,15,16 we get this:

     02
    /  \
   01  14
      /  \
     03  15
           \
           16

and it is unclear how to iterate cpu==0 part of tree in this case.

Iterating cpu specific subtree like this:

#define for_each_group_event(event, group, cpu, pmu, field)	 \
	for (event = rb_entry_safe(group_first(group, cpu, pmu), \
				   typeof(*event), field);	 \
	     event && event->cpu == cpu && event->pmu == pmu;	 \
	     event = rb_entry_safe(rb_next(&event->field),	 \
				   typeof(*event), field))

misses event==03 for the case above and I guess this is where I loose 
samples in my testing.

> 
> 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web