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

Groups > linux.kernel > #1435842 > unrolled thread

Re: [PATCH] perf: fix pmu::filter_match for SW-led groups

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

  Re: [PATCH] perf: fix pmu::filter_match for SW-led groups Peter Zijlstra <peterz@infradead.org> - 2016-07-02 18:50 +0200
    Re: [PATCH] perf: fix pmu::filter_match for SW-led groups Mark Rutland <mark.rutland@arm.com> - 2016-07-04 20:10 +0200
      Re: [PATCH] perf: fix pmu::filter_match for SW-led groups Peter Zijlstra <peterz@infradead.org> - 2016-07-05 10:40 +0200
        Re: [PATCH] perf: fix pmu::filter_match for SW-led groups Mark Rutland <mark.rutland@arm.com> - 2016-07-05 11:50 +0200
          Re: [PATCH] perf: fix pmu::filter_match for SW-led groups Peter Zijlstra <peterz@infradead.org> - 2016-07-05 14:10 +0200
            Re: [PATCH] perf: fix pmu::filter_match for SW-led groups Mark Rutland <mark.rutland@arm.com> - 2016-07-05 15:00 +0200

#1435842 — Re: [PATCH] perf: fix pmu::filter_match for SW-led groups

On Tue, Jun 14, 2016 at 04:10:41PM +0100, Mark Rutland wrote:
> However, pmu::filter_match is only called for the leader of each event
> group. When the leader is a SW event, we do not filter the groups, and
> may fail at pmu::add time, and when this happens we'll give up on
> scheduling any event groups later in the list until they are rotated
> ahead of the failing group.

Ha! indeed.

> I've tried to find a better way of handling this (without needing to walk the
> siblings list), but so far I'm at a loss. At least it's "only" O(n) in the size
> of the sibling list we were going to walk anyway.
> 
> I suspect that at a more fundamental level, I need to stop sharing a
> perf_hw_context between HW PMUs (i.e. replace task_struct::perf_event_ctxp with
> something that can handle multiple HW PMUs). From previous attempts I'm not
> sure if that's going to be possible.
> 
> Any ideas appreciated!

So I think I have half-cooked ideas.

One of the problems I've been wanting to solve for a long time is that
the per-cpu flexible list has priority over the per-task flexible list.

I would like them to rotate together.

One of the ways I was looking at getting that done is a virtual runtime
scheduler (just like cfs). The tricky point is merging two virtual
runtime trees. But I think that should be doable if we sort the trees on
lag.

In any case, the relevance to your question is that once we have a tree,
we can play games with order; that is, if we first order on PMU-id and
only second on lag, we get whole subtree clusters specific for a PMU.


Lost of details missing in that picture, but I think something along
those lines might get us what we want.

[toc] | [next] | [standalone]

#1436502

On Sat, Jul 02, 2016 at 06:40:25PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 14, 2016 at 04:10:41PM +0100, Mark Rutland wrote:
> > However, pmu::filter_match is only called for the leader of each event
> > group. When the leader is a SW event, we do not filter the groups, and
> > may fail at pmu::add time, and when this happens we'll give up on
> > scheduling any event groups later in the list until they are rotated
> > ahead of the failing group.
> 
> Ha! indeed.
> 
> > I've tried to find a better way of handling this (without needing to walk the
> > siblings list), but so far I'm at a loss. At least it's "only" O(n) in the size
> > of the sibling list we were going to walk anyway.
> > 
> > I suspect that at a more fundamental level, I need to stop sharing a
> > perf_hw_context between HW PMUs (i.e. replace task_struct::perf_event_ctxp with
> > something that can handle multiple HW PMUs). From previous attempts I'm not
> > sure if that's going to be possible.
> > 
> > Any ideas appreciated!
> 
> So I think I have half-cooked ideas.
> 
> One of the problems I've been wanting to solve for a long time is that
> the per-cpu flexible list has priority over the per-task flexible list.
> 
> I would like them to rotate together.

Makes sense.

> One of the ways I was looking at getting that done is a virtual runtime
> scheduler (just like cfs). The tricky point is merging two virtual
> runtime trees. But I think that should be doable if we sort the trees on
> lag.
> 
> In any case, the relevance to your question is that once we have a tree,
> we can play games with order; that is, if we first order on PMU-id and
> only second on lag, we get whole subtree clusters specific for a PMU.

Hmm... I'm not sure how that helps in this case. Wouldn't we still need
to walk the sibling list to get the HW PMU-id in the case of a SW group
leader?

For the heterogeenous case we'd need a different sort order per-cpu
(well, per microarchitecture), which sounds like we're going to have to
fully sort the events every time they move between CPUs. :/

> Lost of details missing in that picture, but I think something along
> those lines might get us what we want.

Perhaps! Hopefully I'm just missing those detail above. :)

I also had another though about solving the SW-led group case: if the
leader had a reference to the group's HW PMU (of which there should only
be one), we can filter on that alone, and can also use that in
group_sched_in rather than the ctx->pmu, avoiding the issue that
ctx->pmu is not the same as the group's HW PMU.

I'll have a play with that approach in the mean time.

Thanks,
Mark.

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

#1436844

On Mon, Jul 04, 2016 at 07:05:35PM +0100, Mark Rutland wrote:
> On Sat, Jul 02, 2016 at 06:40:25PM +0200, Peter Zijlstra wrote:
> > One of the ways I was looking at getting that done is a virtual runtime
> > scheduler (just like cfs). The tricky point is merging two virtual
> > runtime trees. But I think that should be doable if we sort the trees on
> > lag.
> > 
> > In any case, the relevance to your question is that once we have a tree,
> > we can play games with order; that is, if we first order on PMU-id and
> > only second on lag, we get whole subtree clusters specific for a PMU.
> 
> Hmm... I'm not sure how that helps in this case. Wouldn't we still need
> to walk the sibling list to get the HW PMU-id in the case of a SW group
> leader?

Since there is a hardware even in the group, it must be part of the
hardware pmu list/tree and would thus end up classified (and sorted) by
that (hardware) PMU-id.

> For the heterogeenous case we'd need a different sort order per-cpu
> (well, per microarchitecture), which sounds like we're going to have to
> fully sort the events every time they move between CPUs. :/

Confused, I thought that for the HG case you had multiple events, one
for each PMU. If we classify these events differently we'd simply use a
different subtree depending on which CPU the task lands.

Currently we've munged the two PMUs together, because, well, that's the
only way.

> I also had another though about solving the SW-led group case: if the
> leader had a reference to the group's HW PMU (of which there should only
> be one), we can filter on that alone, and can also use that in
> group_sched_in rather than the ctx->pmu, avoiding the issue that
> ctx->pmu is not the same as the group's HW PMU.
> 
> I'll have a play with that approach in the mean time.

Right, adds one more pointer to the struct event, but that thing is
massive already.

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

#1436872

On Tue, Jul 05, 2016 at 10:35:26AM +0200, Peter Zijlstra wrote:
> On Mon, Jul 04, 2016 at 07:05:35PM +0100, Mark Rutland wrote:
> > On Sat, Jul 02, 2016 at 06:40:25PM +0200, Peter Zijlstra wrote:
> > > One of the ways I was looking at getting that done is a virtual runtime
> > > scheduler (just like cfs). The tricky point is merging two virtual
> > > runtime trees. But I think that should be doable if we sort the trees on
> > > lag.
> > > 
> > > In any case, the relevance to your question is that once we have a tree,
> > > we can play games with order; that is, if we first order on PMU-id and
> > > only second on lag, we get whole subtree clusters specific for a PMU.
> > 
> > Hmm... I'm not sure how that helps in this case. Wouldn't we still need
> > to walk the sibling list to get the HW PMU-id in the case of a SW group
> > leader?
> 
> Since there is a hardware even in the group, it must be part of the
> hardware pmu list/tree and would thus end up classified (and sorted) by
> that (hardware) PMU-id.
> 
> > For the heterogeenous case we'd need a different sort order per-cpu
> > (well, per microarchitecture), which sounds like we're going to have to
> > fully sort the events every time they move between CPUs. :/
> 
> Confused, I thought that for the HG case you had multiple events, one
> for each PMU. If we classify these events differently we'd simply use a
> different subtree depending on which CPU the task lands.

My bad; I assumed that for both PMUs we'd start at the root, and thus
would need to re-sort in order to get the current CPU's PMU ordered
first, much like currently with rotation.

I guess I'm having difficulty figuring out the structure of that tree.
If we can easily/cheaply find the relevant sub-tree then the above isn't
an issue.

> Currently we've munged the two PMUs together, because, well, that's the
> only way.

Yeah. Splitting them by any means would be great. In the past I'd looked
at changing task_struct::perf_event_ctxp into something that could
handle an arbitrary number of contexts, such that we could avoid
sharing, but ran away after considering the locking/rcu implications.

Thanks,
Mark.

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

#1436954

On Tue, Jul 05, 2016 at 10:44:48AM +0100, Mark Rutland wrote:
> My bad; I assumed that for both PMUs we'd start at the root, and thus
> would need to re-sort in order to get the current CPU's PMU ordered
> first, much like currently with rotation.
> 
> I guess I'm having difficulty figuring out the structure of that tree.
> If we can easily/cheaply find the relevant sub-tree then the above isn't
> an issue.

struct event {
	struct rb_node node;
	int pmu_id;
	s64 lag;
	...
};

bool event_less(struct rb_node *a, struct rb_node *b)
{
	struct event *left = rb_entry(a, struct event, node);
	struct event *right = rb_entry(b, struct event, node);

	if (a->pmu_id < b->pmu_id)
		return true;

	if (b->pmu_id > a->pmu_id)
		return false;

	/* a->pmu_id == b->pmu_id */
	if (a->lag < b->lag)
		return true;

	return false;
}

Will give you a tree with primary order @pmu_id and secondary order
@lag.

Which you'd iterate like:

	for (event = event_find(pmu_id); event->pmu_id == pmu_id; event = event_next(event)) {
	}

And get only the events matching @pmu_id in @lag order.

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

#1436975

On Tue, Jul 05, 2016 at 02:04:26PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 05, 2016 at 10:44:48AM +0100, Mark Rutland wrote:
> > My bad; I assumed that for both PMUs we'd start at the root, and thus
> > would need to re-sort in order to get the current CPU's PMU ordered
> > first, much like currently with rotation.
> > 
> > I guess I'm having difficulty figuring out the structure of that tree.
> > If we can easily/cheaply find the relevant sub-tree then the above isn't
> > an issue.
> 
> struct event {
> 	struct rb_node node;
> 	int pmu_id;
> 	s64 lag;
> 	...
> };
> 
> bool event_less(struct rb_node *a, struct rb_node *b)
> {
> 	struct event *left = rb_entry(a, struct event, node);
> 	struct event *right = rb_entry(b, struct event, node);
> 
> 	if (a->pmu_id < b->pmu_id)
> 		return true;
> 
> 	if (b->pmu_id > a->pmu_id)
> 		return false;
> 
> 	/* a->pmu_id == b->pmu_id */
> 	if (a->lag < b->lag)
> 		return true;
> 
> 	return false;
> }
> 
> Will give you a tree with primary order @pmu_id and secondary order
> @lag.
> 
> Which you'd iterate like:
> 
> 	for (event = event_find(pmu_id); event->pmu_id == pmu_id; event = event_next(event)) {
> 	}
> 
> And get only the events matching @pmu_id in @lag order.

Cheers! Sorry for being thick; I think I understand now.

I'll have a tinker with the idea.

Thanks,
Mark.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web