Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-26 Thread Alexei Starovoitov
On 10/26/15 5:54 AM, Wangnan (F) wrote: On 2015/10/26 20:32, Peter Zijlstra wrote: On Sun, Oct 25, 2015 at 09:23:36AM -0700, Alexei Starovoitov wrote: bpf_perf_event_read() muxes of -EINVAL into return value, but it's non ambiguous to the program whether it got an error or real counter

Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-26 Thread David Miller
From: Alexei Starovoitov Date: Thu, 22 Oct 2015 17:10:14 -0700 > Fix safety checks for bpf_perf_event_read(): > - only non-inherited events can be added to perf_event_array map > (do this check statically at map insertion time) > - dynamically check that event is local and

Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-26 Thread Wangnan (F)
On 2015/10/26 20:32, Peter Zijlstra wrote: On Sun, Oct 25, 2015 at 09:23:36AM -0700, Alexei Starovoitov wrote: bpf_perf_event_read() muxes of -EINVAL into return value, but it's non ambiguous to the program whether it got an error or real counter value. How can that be, the (u64)-EINVAL

Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-26 Thread Peter Zijlstra
On Sun, Oct 25, 2015 at 09:23:36AM -0700, Alexei Starovoitov wrote: > bpf_perf_event_read() muxes of -EINVAL into return value, but it's non > ambiguous to the program whether it got an error or real counter value. How can that be, the (u64)-EINVAL value is a valid counter value.. unlikely maybe,

Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-25 Thread Ingo Molnar
* Alexei Starovoitov wrote: > On 10/23/15 5:03 AM, Peter Zijlstra wrote: > >So the bpf_perf_event_read() returns the count value, does this not also > >mean that returning -EINVAL here is also 'wrong'? > > > >I mean, sure an actual count value that high is unlikely, but its

Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-25 Thread Alexei Starovoitov
On 10/25/15 2:21 AM, Ingo Molnar wrote: Then old crap can be de-emphasised and eventually removed, instead of having to live with crap forever ... strongly disagree. none of the helpers are 'crap'. bpf_perf_event_read() muxes of -EINVAL into return value, but it's non ambiguous to the program

Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-23 Thread Alexei Starovoitov
On 10/23/15 5:03 AM, Peter Zijlstra wrote: So the bpf_perf_event_read() returns the count value, does this not also mean that returning -EINVAL here is also 'wrong'? I mean, sure an actual count value that high is unlikely, but its still a broken interface. Agree. that's not pretty interface.

Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-23 Thread Peter Zijlstra
On Thu, Oct 22, 2015 at 05:10:14PM -0700, Alexei Starovoitov wrote: > +++ b/kernel/trace/bpf_trace.c > @@ -199,6 +199,11 @@ static u64 bpf_perf_event_read(u64 r1, u64 index, u64 > r3, u64 r4, u64 r5) > if (!event) > return -ENOENT; > > + /* make sure event is local and

Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-23 Thread Peter Zijlstra
On Fri, Oct 23, 2015 at 07:42:22AM -0700, Alexei Starovoitov wrote: > On 10/23/15 5:03 AM, Peter Zijlstra wrote: > >So the bpf_perf_event_read() returns the count value, does this not also > >mean that returning -EINVAL here is also 'wrong'? > > > >I mean, sure an actual count value that high is

Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-22 Thread Wangnan (F)
On 2015/10/23 8:10, Alexei Starovoitov wrote: Fix safety checks for bpf_perf_event_read(): - only non-inherited events can be added to perf_event_array map (do this check statically at map insertion time) - dynamically check that event is local and !pmu->count Otherwise buggy bpf program

Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-22 Thread Wangnan (F)
On 2015/10/23 8:10, Alexei Starovoitov wrote: Fix safety checks for bpf_perf_event_read(): - only non-inherited events can be added to perf_event_array map (do this check statically at map insertion time) - dynamically check that event is local and !pmu->count Otherwise buggy bpf program

Re: [PATCH v3 net-next] bpf: fix bpf_perf_event_read() helper

2015-10-22 Thread Alexei Starovoitov
On 10/22/15 7:21 PM, Wangnan (F) wrote: +if (attr->inherit) +goto err; + Since Peter suggest it is pointless for a system-wide perf_event has inherit bit set [1], I think it should be safe to enable system-wide perf_event pass this check? here we don't know whether it's system