Re: [PATCH 1/1] perf, core: Use sample period avg as child event's initial period

[Peter Zijlstra]

On Fri, Dec 12, 2014 at 10:10:35AM -0500, kan.liang@xxxxxxxxx wrote:

> That's because, in the inherit_event, the period
> for child event is inherit from parent's parent's event, which is
> usually the default sample_period 1. Each child event has to recaculate
> the period from 1 everytime. That brings high overhead.
> 
> This patch keeps the sample period average in original parent event.

That's going to make an even worse nightmare of the refcounting :/

> Each new child event can use it as its initial sample period.
> Adding a ori_parent in struct perf_event to help child event access the
> original parent. For each new child event, the parent event refcount++.
> Parent will not go away until all children go away. So the stored
> pointer is safe to be accessed.

So going by the above you only use this once, during fork, which already
guarantees the existence of the parent.

Now looking at the code you actually use it again in
perf_adjust_period() and push period adjustments into the parent.

This doesn't seem to make any kind of sense, and its weirdly
implemented.

So why would you push anything to the original parent? Your description
states that the parent event usually has 1, and then you argue about
fixing that by using the orig parent, but then you need to update the
orig parent. Did you go in circles and confuse yourself? Why not push
things into the regular parent event if you're going to push things up.

Also, since you can have multiple child events, on many CPUs local64_t
is the wrong data type, furthermore its going to be a scalability issue
on big hardware.

So please, try again. And try and explain what you're actually doing.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/