On 9/1/17 1:50 PM, Peter Zijlstra wrote:
On Fri, Sep 01, 2017 at 01:29:17PM -0700, Alexei Starovoitov wrote:
+BPF_CALL_4(bpf_perf_read_counter_time, struct bpf_map *, map, u64, flags,
+ struct bpf_perf_counter_time *, buf, u32, size)
+{
+ struct perf_event *pe;
+ u64 now;
+ int err;
+
+ if (unlikely(size != sizeof(struct bpf_perf_counter_time)))
+ return -EINVAL;
+ err = get_map_perf_counter(map, flags, &buf->counter, &pe);
+ if (err)
+ return err;
+
+ calc_timer_values(pe, &now, &buf->time.enabled, &buf->time.running);
+ return 0;
+}
Peter,
I believe we're doing it correctly above.
It's a copy paste of the same logic as in total_time_enabled/running.
We cannot expose total_time_enabled/running to bpf, since they are
different counters. The above two are specific to bpf usage.
See commit log.
No, the patch is atrocious and the usage is wrong.
Exporting a function called 'calc_timer_values' is a horrible violation
of the namespace.
And its wrong because it should be done in conjunction with
perf_event_read_local(). You cannot afterwards call this because you
don't know if the event was active when you read it and you don't have
temporal guarantees; that is, reading these timestamps long after or
before the read is wrong, and this interface allows it.
Thanks for explanation. Will push the read/calculate time
enabled/running inside the perf_event_read_local then.
So no, sorry this is just fail.
