| Message ID | 1436929315-28520-6-git-send-email-sukadev@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Jul 14, 2015 at 08:01:52PM -0700, Sukadev Bhattiprolu wrote: > Move the part of perf_event_read_value() that computes the event > counts and event times into a new function, perf_event_compute(). > > This would allow us to call perf_event_compute() independently. > > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> > > Changelog[v3] > Rather than move perf_event_read() into callers and then > rename, just move the computations into a separate function > (redesign to address comment from Peter Zijlstra). > --- Changelog[] bits go here, below the '---' where they get discarded. > kernel/events/core.c | 37 ++++++++++++++++++++++++------------- > 1 file changed, 24 insertions(+), 13 deletions(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 44fb89d..b1e9a42 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -3704,6 +3704,29 @@ static int perf_release(struct inode *inode, struct file *file) > return 0; > } > > +static u64 perf_event_compute(struct perf_event *event, u64 *enabled, > + u64 *running) This is a horrible name, 'compute' what? > +{ > + struct perf_event *child; > + u64 total; > + > + total = perf_event_count(event); > + > + *enabled += event->total_time_enabled + > + atomic64_read(&event->child_total_time_enabled); > + *running += event->total_time_running + > + atomic64_read(&event->child_total_time_running); > + lockdep_assert_held(&event->child_mutex); > + list_for_each_entry(child, &event->child_list, child_list) { > + perf_event_read(child); > + total += perf_event_count(child); > + *enabled += child->total_time_enabled; > + *running += child->total_time_running; > + } > + > + return total; > +} > + > /* > * Remove all orphanes events from the context. > */
Peter Zijlstra [peterz@infradead.org] wrote:
| On Tue, Jul 14, 2015 at 08:01:52PM -0700, Sukadev Bhattiprolu wrote:
| > Move the part of perf_event_read_value() that computes the event
| > counts and event times into a new function, perf_event_compute().
| >
| > This would allow us to call perf_event_compute() independently.
| >
| > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
| >
| > Changelog[v3]
| > Rather than move perf_event_read() into callers and then
| > rename, just move the computations into a separate function
| > (redesign to address comment from Peter Zijlstra).
| > ---
|
| Changelog[] bits go here, below the '---' where they get discarded.
Sorry. Will fix it.
|
| > kernel/events/core.c | 37 ++++++++++++++++++++++++-------------
| > 1 file changed, 24 insertions(+), 13 deletions(-)
| >
| > diff --git a/kernel/events/core.c b/kernel/events/core.c
| > index 44fb89d..b1e9a42 100644
| > --- a/kernel/events/core.c
| > +++ b/kernel/events/core.c
| > @@ -3704,6 +3704,29 @@ static int perf_release(struct inode *inode, struct file *file)
| > return 0;
| > }
| >
| > +static u64 perf_event_compute(struct perf_event *event, u64 *enabled,
| > + u64 *running)
|
| This is a horrible name, 'compute' what?
We are aggregating event counts and time for children.
Would perf_event_aggregate() or perf_event_aggregate_children()
be better?
|
| > +{
| > + struct perf_event *child;
| > + u64 total;
| > +
| > + total = perf_event_count(event);
| > +
| > + *enabled += event->total_time_enabled +
| > + atomic64_read(&event->child_total_time_enabled);
| > + *running += event->total_time_running +
| > + atomic64_read(&event->child_total_time_running);
| > +
|
| lockdep_assert_held(&event->child_mutex);
OK. Thanks for the comments.
Sukadev
On Tue, Jul 14, 2015 at 08:01:52PM -0700, Sukadev Bhattiprolu wrote: > Move the part of perf_event_read_value() that computes the event > counts and event times into a new function, perf_event_compute(). > > This would allow us to call perf_event_compute() independently. > > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> > > Changelog[v3] > Rather than move perf_event_read() into callers and then > rename, just move the computations into a separate function > (redesign to address comment from Peter Zijlstra). > --- > kernel/events/core.c | 37 ++++++++++++++++++++++++------------- > 1 file changed, 24 insertions(+), 13 deletions(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 44fb89d..b1e9a42 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -3704,6 +3704,29 @@ static int perf_release(struct inode *inode, struct file *file) > return 0; > } > > +static u64 perf_event_compute(struct perf_event *event, u64 *enabled, > + u64 *running) > +{ > + struct perf_event *child; > + u64 total; > + > + total = perf_event_count(event); > + > + *enabled += event->total_time_enabled + > + atomic64_read(&event->child_total_time_enabled); > + *running += event->total_time_running + > + atomic64_read(&event->child_total_time_running); > + > + list_for_each_entry(child, &event->child_list, child_list) { > + perf_event_read(child); Sure we don't want that.. > + total += perf_event_count(child); > + *enabled += child->total_time_enabled; > + *running += child->total_time_running; > + } > + > + return total; > +}
Peter Zijlstra [peterz@infradead.org] wrote:
| On Tue, Jul 14, 2015 at 08:01:52PM -0700, Sukadev Bhattiprolu wrote:
| > Move the part of perf_event_read_value() that computes the event
| > counts and event times into a new function, perf_event_compute().
| >
| > This would allow us to call perf_event_compute() independently.
| >
| > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
| >
| > Changelog[v3]
| > Rather than move perf_event_read() into callers and then
| > rename, just move the computations into a separate function
| > (redesign to address comment from Peter Zijlstra).
| > ---
| > kernel/events/core.c | 37 ++++++++++++++++++++++++-------------
| > 1 file changed, 24 insertions(+), 13 deletions(-)
| >
| > diff --git a/kernel/events/core.c b/kernel/events/core.c
| > index 44fb89d..b1e9a42 100644
| > --- a/kernel/events/core.c
| > +++ b/kernel/events/core.c
| > @@ -3704,6 +3704,29 @@ static int perf_release(struct inode *inode, struct file *file)
| > return 0;
| > }
| >
| > +static u64 perf_event_compute(struct perf_event *event, u64 *enabled,
| > + u64 *running)
| > +{
| > + struct perf_event *child;
| > + u64 total;
| > +
| > + total = perf_event_count(event);
| > +
| > + *enabled += event->total_time_enabled +
| > + atomic64_read(&event->child_total_time_enabled);
| > + *running += event->total_time_running +
| > + atomic64_read(&event->child_total_time_running);
| > +
| > + list_for_each_entry(child, &event->child_list, child_list) {
| > + perf_event_read(child);
|
| Sure we don't want that..
So if say x86 calls perf_event_read_value() the current upstream code
makes the perf_event_read(child).
If we remove this, then it would be a change in behavior?
I have commented it out and have TODO in the latest patchset. Pls
review and let me know if we should drop this read (and the TODO)
of the child event.
Sukadev
diff --git a/kernel/events/core.c b/kernel/events/core.c index 44fb89d..b1e9a42 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -3704,6 +3704,29 @@ static int perf_release(struct inode *inode, struct file *file) return 0; } +static u64 perf_event_compute(struct perf_event *event, u64 *enabled, + u64 *running) +{ + struct perf_event *child; + u64 total; + + total = perf_event_count(event); + + *enabled += event->total_time_enabled + + atomic64_read(&event->child_total_time_enabled); + *running += event->total_time_running + + atomic64_read(&event->child_total_time_running); + + list_for_each_entry(child, &event->child_list, child_list) { + perf_event_read(child); + total += perf_event_count(child); + *enabled += child->total_time_enabled; + *running += child->total_time_running; + } + + return total; +} + /* * Remove all orphanes events from the context. */ @@ -3742,7 +3765,6 @@ static void orphans_remove_work(struct work_struct *work) u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running) { - struct perf_event *child; u64 total = 0; *enabled = 0; @@ -3751,19 +3773,8 @@ u64 perf_event_read_value(struct perf_event *event, u64 *enabled, u64 *running) mutex_lock(&event->child_mutex); perf_event_read(event); - total += perf_event_count(event); + total = perf_event_compute(event, enabled, running); - *enabled += event->total_time_enabled + - atomic64_read(&event->child_total_time_enabled); - *running += event->total_time_running + - atomic64_read(&event->child_total_time_running); - - list_for_each_entry(child, &event->child_list, child_list) { - perf_event_read(child); - total += perf_event_count(child); - *enabled += child->total_time_enabled; - *running += child->total_time_running; - } mutex_unlock(&event->child_mutex); return total;
Move the part of perf_event_read_value() that computes the event counts and event times into a new function, perf_event_compute(). This would allow us to call perf_event_compute() independently. Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> Changelog[v3] Rather than move perf_event_read() into callers and then rename, just move the computations into a separate function (redesign to address comment from Peter Zijlstra). --- kernel/events/core.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-)