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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 15 Dec 2014 10:55:51 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	kan.liang@...el.com
Cc:	linux-kernel@...r.kernel.org, eranian@...gle.com,
	ak@...ux.intel.com
Subject: Re: [PATCH 1/1] perf, core: Use sample period avg as child event's
 initial period

On Fri, Dec 12, 2014 at 10:10:35AM -0500, kan.liang@...el.com 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/