Previous patch v5 url:
https://lkml.org/lkml/2015/7/31/299
changes in V6:
- make the Patch 1/4 commit message more meaning and readable;
- remove the unnecessary comment in Patch 2/4 and make it clean;
- declare the function perf_event_release_kernel() in include/
linux/perf_event.h to fix the build error when CONFIG_PERF_EVENTS
isn't configured in Patch 2/4;
- add function perf_event_attrs() to get the struct perf_event_attr
in Patch 2/4.
- move the related code from kernel/trace/bpf_trace.c to kernel/
events/core.c and add function perf_event_read_internal() to
avoid poking inside of the event outside of perf code in Patch 3/4;
- generial the func & map match-pair with an array in Patch 3/4;
changes in V5:
- move struct fd_array_map_ops* fd_ops to bpf_map;
- move array perf event decrement refcnt function to
map_free;
- fix the NULL ptr of perf_event_get();
- move bpf_perf_event_read() to kernel/bpf/bpf_trace.c;
- get rid of the remaining struct bpf_prog;
- move the unnecessay cast on void *;
changes in V4:
- make the bpf_prog_array_map more generic;
- fix the bug of event refcnt leak;
- use more useful errno in bpf_perf_event_read();
changes in V3:
- collapse V2 patches 1-3 into one;
- drop the function map->ops->map_traverse_elem() and release
the struct perf_event in map_free;
- only allow to access bpf_perf_event_read() from programs;
- update the perf_event_array_map elem via xchg();
- pass index directly to bpf_perf_event_read() instead of
MAP_KEY;
changes in V2:
- put atomic_long_inc_not_zero() between fdget() and fdput();
- limit the event type to PERF_TYPE_RAW and PERF_TYPE_HARDWARE;
- Only read the event counter on current CPU or on current
process;
- add new map type BPF_MAP_TYPE_PERF_EVENT_ARRAY to store the
pointer to the struct perf_event;
- according to the perf_event_map_fd and key, the function
bpf_perf_event_read() can get the Hardware PMU counter value;
Patch 4/4 is a simple example and shows how to use this new eBPF
programs ability. The PMU counter data can be found in
/sys/kernel/debug/tracing/trace(trace_pipe).(the cycles PMU
value when 'kprobe/sys_write' sampling)
$ cat /sys/kernel/debug/tracing/trace_pipe
$ ./tracex6
...
syslog-ng-548 [000] d..1 76.905673: : CPU-0 681765271
syslog-ng-548 [000] d..1 76.905690: : CPU-0 681787855
syslog-ng-548 [000] d..1 76.905707: : CPU-0 681810504
syslog-ng-548 [000] d..1 76.905725: : CPU-0 681834771
syslog-ng-548 [000] d..1 76.905745: : CPU-0 681859519
syslog-ng-548 [000] d..1 76.905766: : CPU-0 681890419
syslog-ng-548 [000] d..1 76.905783: : CPU-0 681914045
syslog-ng-548 [000] d..1 76.905800: : CPU-0 681935950
syslog-ng-548 [000] d..1 76.905816: : CPU-0 681958299
ls-690 [005] d..1 82.241308: : CPU-5 3138451
sh-691 [004] d..1 82.244570: : CPU-4 7324988
<...>-699 [007] d..1 99.961387: : CPU-7 3194027
<...>-695 [003] d..1 99.961474: : CPU-3 288901
<...>-695 [003] d..1 99.961541: : CPU-3 383145
<...>-695 [003] d..1 99.961591: : CPU-3 450365
<...>-695 [003] d..1 99.961639: : CPU-3 515751
<...>-695 [003] d..1 99.961686: : CPU-3 579047
...
The detail of patches is as follow:
Patch 1/4 rewrites part of the bpf_prog_array map code and make it
more generic;
Patch 2/4 introduces a new bpf map type. This map only stores the
pointer to struct perf_event;
Patch 3/4 implements function bpf_perf_event_read() that get the
selected hardware PMU conuter;
Patch 4/4 gives a simple example.
Kaixu Xia (3):
bpf: Add new bpf map type to store the pointer to struct perf_event
bpf: Implement function bpf_perf_event_read() that get the selected
hardware PMU conuter
samples/bpf: example of get selected PMU counter value
Wang Nan (1):
bpf: Make the bpf_prog_array_map more generic
arch/x86/net/bpf_jit_comp.c | 6 +-
include/linux/bpf.h | 10 +++-
include/linux/perf_event.h | 10 ++++
include/uapi/linux/bpf.h | 2 +
kernel/bpf/arraymap.c | 137 ++++++++++++++++++++++++++++++++++----------
kernel/bpf/core.c | 2 +-
kernel/bpf/syscall.c | 2 +-
kernel/bpf/verifier.c | 49 +++++++++++-----
kernel/events/core.c | 44 ++++++++++++++
kernel/trace/bpf_trace.c | 31 ++++++++++
samples/bpf/Makefile | 4 ++
samples/bpf/bpf_helpers.h | 2 +
samples/bpf/tracex6_kern.c | 26 +++++++++
samples/bpf/tracex6_user.c | 68 ++++++++++++++++++++++
14 files changed, 340 insertions(+), 53 deletions(-)
create mode 100644 samples/bpf/tracex6_kern.c
create mode 100644 samples/bpf/tracex6_user.c
--
1.8.3.4
From: Wang Nan <redacted>
All the map backends are of generic nature. In order to avoid
adding much special code into the eBPF core, rewrite part of
the bpf_prog_array map code and make it more generic. So the
new perf_event_array map type can reuse most of code with
bpf_prog_array map and add fewer lines of special code.
Signed-off-by: Wang Nan <redacted>
Signed-off-by: Kaixu Xia <redacted>
---
arch/x86/net/bpf_jit_comp.c | 6 ++--
include/linux/bpf.h | 8 +++--
kernel/bpf/arraymap.c | 80 +++++++++++++++++++++++++++------------------
kernel/bpf/core.c | 2 +-
kernel/bpf/syscall.c | 2 +-
5 files changed, 60 insertions(+), 38 deletions(-)
@@ -24,6 +24,10 @@ struct bpf_map_ops {void*(*map_lookup_elem)(structbpf_map*map,void*key);int(*map_update_elem)(structbpf_map*map,void*key,void*value,u64flags);int(*map_delete_elem)(structbpf_map*map,void*key);++/* funcs called by prog_array and perf_event_array map */+void*(*map_fd_get_ptr)(structbpf_map*map,intfd);+void(*map_fd_put_ptr)(void*ptr);};structbpf_map{
@@ -150,15 +150,15 @@ static int __init register_array_map(void)}late_initcall(register_array_map);-staticstructbpf_map*prog_array_map_alloc(unionbpf_attr*attr)+staticstructbpf_map*fd_array_map_alloc(unionbpf_attr*attr){-/* only bpf_prog file descriptors can be stored in prog_array map */+/* only file descriptors can be stored in this type of map */if(attr->value_size!=sizeof(u32))returnERR_PTR(-EINVAL);returnarray_map_alloc(attr);}-staticvoidprog_array_map_free(structbpf_map*map)+staticvoidfd_array_map_free(structbpf_map*map){structbpf_array*array=container_of(map,structbpf_array,map);inti;
@@ -167,21 +167,21 @@ static void prog_array_map_free(struct bpf_map *map)/* make sure it's empty */for(i=0;i<array->map.max_entries;i++)-BUG_ON(array->prog[i]!=NULL);+BUG_ON(array->ptrs[i]!=NULL);kvfree(array);}-staticvoid*prog_array_map_lookup_elem(structbpf_map*map,void*key)+staticvoid*fd_array_map_lookup_elem(structbpf_map*map,void*key){returnNULL;}/* only called from syscall */-staticintprog_array_map_update_elem(structbpf_map*map,void*key,-void*value,u64map_flags)+staticintfd_array_map_update_elem(structbpf_map*map,void*key,+void*value,u64map_flags){structbpf_array*array=container_of(map,structbpf_array,map);-structbpf_prog*prog,*old_prog;+void*new_ptr,*old_ptr;u32index=*(u32*)key,ufd;if(map_flags!=BPF_ANY)
@@ -191,57 +191,75 @@ static int prog_array_map_update_elem(struct bpf_map *map, void *key,return-E2BIG;ufd=*(u32*)value;-prog=bpf_prog_get(ufd);-if(IS_ERR(prog))-returnPTR_ERR(prog);--if(!bpf_prog_array_compatible(array,prog)){-bpf_prog_put(prog);-return-EINVAL;-}+new_ptr=map->ops->map_fd_get_ptr(map,ufd);+if(IS_ERR(new_ptr))+returnPTR_ERR(new_ptr);-old_prog=xchg(array->prog+index,prog);-if(old_prog)-bpf_prog_put_rcu(old_prog);+old_ptr=xchg(array->ptrs+index,new_ptr);+if(old_ptr)+map->ops->map_fd_put_ptr(old_ptr);return0;}-staticintprog_array_map_delete_elem(structbpf_map*map,void*key)+staticintfd_array_map_delete_elem(structbpf_map*map,void*key){structbpf_array*array=container_of(map,structbpf_array,map);-structbpf_prog*old_prog;+void*old_ptr;u32index=*(u32*)key;if(index>=array->map.max_entries)return-E2BIG;-old_prog=xchg(array->prog+index,NULL);-if(old_prog){-bpf_prog_put_rcu(old_prog);+old_ptr=xchg(array->ptrs+index,NULL);+if(old_ptr){+map->ops->map_fd_put_ptr(old_ptr);return0;}else{return-ENOENT;}}+staticvoid*prog_fd_array_get_ptr(structbpf_map*map,intfd)+{+structbpf_array*array=container_of(map,structbpf_array,map);+structbpf_prog*prog=bpf_prog_get(fd);+if(IS_ERR(prog))+returnprog;++if(!bpf_prog_array_compatible(array,prog)){+bpf_prog_put(prog);+returnERR_PTR(-EINVAL);+}+returnprog;+}++staticvoidprog_fd_array_put_ptr(void*ptr)+{+structbpf_prog*prog=ptr;++bpf_prog_put_rcu(prog);+}+/* decrement refcnt of all bpf_progs that are stored in this map */-voidbpf_prog_array_map_clear(structbpf_map*map)+voidbpf_fd_array_map_clear(structbpf_map*map){structbpf_array*array=container_of(map,structbpf_array,map);inti;for(i=0;i<array->map.max_entries;i++)-prog_array_map_delete_elem(map,&i);+fd_array_map_delete_elem(map,&i);}staticconststructbpf_map_opsprog_array_ops={-.map_alloc=prog_array_map_alloc,-.map_free=prog_array_map_free,+.map_alloc=fd_array_map_alloc,+.map_free=fd_array_map_free,.map_get_next_key=array_map_get_next_key,-.map_lookup_elem=prog_array_map_lookup_elem,-.map_update_elem=prog_array_map_update_elem,-.map_delete_elem=prog_array_map_delete_elem,+.map_lookup_elem=fd_array_map_lookup_elem,+.map_update_elem=fd_array_map_update_elem,+.map_delete_elem=fd_array_map_delete_elem,+.map_fd_get_ptr=prog_fd_array_get_ptr,+.map_fd_put_ptr=prog_fd_array_put_ptr,};staticstructbpf_map_type_listprog_array_type__read_mostly={
This is a simple example and shows how to use the new ability
to get the selected Hardware PMU counter value.
Signed-off-by: Kaixu Xia <redacted>
---
samples/bpf/Makefile | 4 +++
samples/bpf/bpf_helpers.h | 2 ++
samples/bpf/tracex6_kern.c | 26 ++++++++++++++++++
samples/bpf/tracex6_user.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 100 insertions(+)
create mode 100644 samples/bpf/tracex6_kern.c
create mode 100644 samples/bpf/tracex6_user.c
@@ -51,6 +54,7 @@ HOSTLOADLIBES_tracex2 += -lelfHOSTLOADLIBES_tracex3+=-lelfHOSTLOADLIBES_tracex4+=-lelf-lrtHOSTLOADLIBES_tracex5+=-lelf+HOSTLOADLIBES_tracex6+=-lelfHOSTLOADLIBES_lathist+=-lelf# point this to your LLVM backend with bpf support
@@ -31,6 +31,8 @@ static unsigned long long (*bpf_get_current_uid_gid)(void) =(void*)BPF_FUNC_get_current_uid_gid;staticint(*bpf_get_current_comm)(void*buf,intbuf_size)=(void*)BPF_FUNC_get_current_comm;+staticint(*bpf_perf_event_read)(void*map,intindex)=+(void*)BPF_FUNC_perf_event_read;/* llvm builtin functions that eBPF C program may use to*emitBPF_LD_ABSandBPF_LD_INDinstructions
Introduce a new bpf map type 'BPF_MAP_TYPE_PERF_EVENT_ARRAY'.
This map only stores the pointer to struct perf_event. The
user space event FDs from perf_event_open() syscall are converted
to the pointer to struct perf_event and stored in map.
Signed-off-by: Kaixu Xia <redacted>
---
include/linux/bpf.h | 1 +
include/linux/perf_event.h | 8 +++++++
include/uapi/linux/bpf.h | 1 +
kernel/bpf/arraymap.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++
kernel/events/core.c | 25 ++++++++++++++++++++
5 files changed, 92 insertions(+)
According to the perf_event_map_fd and index, the function
bpf_perf_event_read() can convert the corresponding map
value to the pointer to struct perf_event and return the
Hardware PMU counter value.
Signed-off-by: Kaixu Xia <redacted>
---
include/linux/bpf.h | 1 +
include/linux/perf_event.h | 2 ++
include/uapi/linux/bpf.h | 1 +
kernel/bpf/verifier.c | 49 ++++++++++++++++++++++++++++++++--------------
kernel/events/core.c | 19 ++++++++++++++++++
kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++
6 files changed, 88 insertions(+), 15 deletions(-)
@@ -833,6 +841,29 @@ static int check_func_arg(struct verifier_env *env, u32 regno,returnerr;}+staticintcheck_func_limit(structbpf_map**mapp,intfunc_id)+{+structbpf_map*map=*mapp;+boolbool_map,bool_func;+inti;++if(!map)+return0;++for(i=0;i<=ARRAY_SIZE(func_limit);i++){+bool_map=(map->map_type==func_limit[i].map_type);+bool_func=(func_id==func_limit[i].func_id);+/* only when map & func pair match it can continue.+*don'tallowanyothermaptypetobepassedinto+*thespecialfunc;+*/+if(bool_map!=bool_func)+return-EINVAL;+}++return0;+}+staticintcheck_call(structverifier_env*env,intfunc_id){structverifier_state*state=&env->cur_state;
@@ -908,21 +939,9 @@ static int check_call(struct verifier_env *env, int func_id)return-EINVAL;}-if(map&&map->map_type==BPF_MAP_TYPE_PROG_ARRAY&&-func_id!=BPF_FUNC_tail_call)-/* prog_array map type needs extra care:-*onlyallowtopassitintobpf_tail_call()fornow.-*bpf_map_delete_elem()canbeallowedinthefuture,-*whilebpf_map_update_elem()mustonlybedoneviasyscall-*/-return-EINVAL;--if(func_id==BPF_FUNC_tail_call&&-map->map_type!=BPF_MAP_TYPE_PROG_ARRAY)-/* don't allow any other map type to be passed into-*bpf_tail_call()-*/-return-EINVAL;+err=check_func_limit(&map,func_id);+if(err)+returnerr;return0;}
@@ -273,3 +273,60 @@ static int __init register_prog_array_map(void)return0;}late_initcall(register_prog_array_map);++staticvoidperf_event_array_map_free(structbpf_map*map)+{+bpf_fd_array_map_clear(map);+fd_array_map_free(map);+}++staticvoid*perf_event_fd_array_get_ptr(structbpf_map*map,intfd)+{+structperf_event*event;+structperf_event_attr*attr;++event=perf_event_get(fd);+if(IS_ERR(event))+returnevent;++attr=perf_event_attrs(event);+if(IS_ERR(attr))+returnattr;++if(attr->type!=PERF_TYPE_RAW&&+attr->type!=PERF_TYPE_HARDWARE){+perf_event_release_kernel(event);+returnERR_PTR(-EINVAL);+}+returnevent;+}
I'm not sure whether Peter wanted to see the above function to be
in events/core.c or not.
imo it's fine here, since perf_event_attr is an uapi struct.
+static int check_func_limit(struct bpf_map **mapp, int func_id)
how about 'check_map_func_compatibility' or 'check_map_func_affinity' ?
+{
+ struct bpf_map *map = *mapp;
why pass pointer to a pointer? single pointer would be be fine.
+ bool bool_map, bool_func;
+ int i;
+
+ if (!map)
+ return 0;
+
+ for (i = 0; i <= ARRAY_SIZE(func_limit); i++) {
+ bool_map = (map->map_type == func_limit[i].map_type);
+ bool_func = (func_id == func_limit[i].func_id);
+ /* only when map & func pair match it can continue.
+ * don't allow any other map type to be passed into
+ * the special func;
+ */
+ if (bool_map != bool_func)
+ return -EINVAL;
+ }
nice simplification!
the rest of the changes look good.
please respin your next set against net-next.
+static int check_func_limit(struct bpf_map **mapp, int func_id)
how about 'check_map_func_compatibility' or 'check_map_func_affinity' ?
quoted
+{
+ struct bpf_map *map = *mapp;
why pass pointer to a pointer? single pointer would be be fine.
quoted
+ bool bool_map, bool_func;
+ int i;
+
+ if (!map)
+ return 0;
+
+ for (i = 0; i <= ARRAY_SIZE(func_limit); i++) {
+ bool_map = (map->map_type == func_limit[i].map_type);
+ bool_func = (func_id == func_limit[i].func_id);
+ /* only when map & func pair match it can continue.
+ * don't allow any other map type to be passed into
+ * the special func;
+ */
+ if (bool_map != bool_func)
+ return -EINVAL;
+ }
nice simplification!
the rest of the changes look good.
please respin your next set against net-next.
Thanks for your review! I will follow them in the next set.
I'm not sure whether Peter wanted to see the above function to be
in events/core.c or not.
imo it's fine here, since perf_event_attr is an uapi struct.
Right, aside from the const issue this is fine, for the exact reason you
state, perf_event_attr is an exposed interface.
Maybe: perf_event_read_local(), as this is this function only works for
events active on the current CPU.
+{
+ if (!event)
+ return -EINVAL;
+
+ if (unlikely(event->state != PERF_EVENT_STATE_ACTIVE))
+ return -EINVAL;
You can return perf_event_count() in that case.
+
+ if (event->oncpu != raw_smp_processor_id() &&
That _must_ be smp_processor_id(). If that gives a warning (ie.
preemption is not disabled or we're not affine to this one cpu) then the
warning is valid.
+ event->ctx->task != current)
Write it like:
if (event->ctx->task != current &&
event->oncpu != smp_processor_id())
That way you'll not evaluate smp_processor_id() for current task events.
+ return -EINVAL;
+
+ if (unlikely(event->attr.inherit))
+ return -EINVAL;
This should be in your accept function, inherited events should never
get this far.
You need IRQs disabled while calling __perf_event_read(), did you test
with lockdep enabled?
From: Peter Zijlstra <peterz@infradead.org> Date: 2015-08-05 10:15:20
On Wed, Aug 05, 2015 at 12:04:25PM +0200, Peter Zijlstra wrote:
On Tue, Aug 04, 2015 at 08:58:15AM +0000, Kaixu Xia wrote:
quoted
+ event->ctx->task != current)
Strictly speaking we should hold rcu_read_lock around dereferencing
event->ctx (or have IRQs disabled -- although I know Paul doesn't like
us relying on that).
Maybe: perf_event_read_local(), as this is this function only works for
events active on the current CPU.
quoted
+{
+ if (!event)
+ return -EINVAL;
+
+ if (unlikely(event->state != PERF_EVENT_STATE_ACTIVE))
+ return -EINVAL;
You can return perf_event_count() in that case.
quoted
+
+ if (event->oncpu != raw_smp_processor_id() &&
That _must_ be smp_processor_id(). If that gives a warning (ie.
preemption is not disabled or we're not affine to this one cpu) then the
warning is valid.
quoted
+ event->ctx->task != current)
Write it like:
if (event->ctx->task != current &&
event->oncpu != smp_processor_id())
That way you'll not evaluate smp_processor_id() for current task events.
quoted
+ return -EINVAL;
+
+ if (unlikely(event->attr.inherit))
+ return -EINVAL;
This should be in your accept function, inherited events should never
get this far.
You need IRQs disabled while calling __perf_event_read(), did you test
with lockdep enabled?
Thanks for your review! I've not tested it, will do that from now on.
From: Peter Zijlstra <peterz@infradead.org> Date: 2015-08-05 13:53:30
On Wed, Aug 05, 2015 at 12:04:25PM +0200, Peter Zijlstra wrote:
Also, you probably want a WARN_ON(in_nmi()) there, this function is
_NOT_ NMI safe.
I had a wee think about that, and I think the below is safe.
(with the obvious problem that WARN from NMI context is not safe)
It does not give you up-to-date overcommit times but your version didn't
either so I'm assuming you don't need those, if you do need those it
needs more but we can do that too.
---
include/linux/perf_event.h | 1 +
kernel/events/core.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
@@ -3222,6 +3222,59 @@ static inline u64 perf_event_count(struct perf_event *event)return__perf_event_count(event);}+/*+*NMI-safemethodtoreadalocalevent,thatisaneventthat+*is:+*-eitherforthecurrenttask,orforthisCPU+*-doesnothaveinheritset,forinheritedtaskevents+*willnotbelocalandwecannotreadthematomically+*-mustnothaveapmu::countmethod+*/+u64perf_event_read_local(structperf_event*event)+{+unsignedlongflags;+u64val;++/*+*Disablinginterruptsavoidsallcounterscheduling(context+*switches,timerbasedrotationandIPIs).+*/+local_irq_safe(flags);++/* If this is a per-task event, it must be for current */+WARN_ON_ONCE((event->attach_state&PERF_ATTACH_TASK)&&+event->hw.target!=current);++/* If this is a per-CPU event, it must be for this CPU */+WARN_ON_ONCE(!(event->attach_state&PERF_ATTACH_TASK)&&+event->cpu!=smp_processor_id());++/*+*Itmustnotbeaneventwithinheritset,wecannotread+*allchildcountersfromatomiccontext.+*/+WARN_ON_ONCE(event->attr.inherit);++/*+*Itmustnothaveapmu::countmethod,thosearenot+*NMIsafe.+*/+WARN_ON_ONCE(event->pmu->count);++/*+*IftheeventiscurrentlyonthisCPU,itseitheraper-taskevent,+*orlocaltothisCPU.FurthermoreitmeansitsACTIVE(otherwise+*oncpu==-1).+*/+if(event->oncpu==smp_processor_id())+event->pmu->read(event);++val=local64_read(&event->count);+local_irq_restore(flags);++returnval;+}+staticu64perf_event_read(structperf_event*event){/*
From: Peter Zijlstra <peterz@infradead.org> Date: 2015-08-05 13:59:37
On Wed, Aug 05, 2015 at 03:53:17PM +0200, Peter Zijlstra wrote:
+/*
+ * NMI-safe method to read a local event, that is an event that
+ * is:
+ * - either for the current task, or for this CPU
+ * - does not have inherit set, for inherited task events
+ * will not be local and we cannot read them atomically
+ * - must not have a pmu::count method
+ */
+u64 perf_event_read_local(struct perf_event *event)
+{
+ unsigned long flags;
+ u64 val;
+
+ /*
+ * Disabling interrupts avoids all counter scheduling (context
+ * switches, timer based rotation and IPIs).
+ */
+ local_irq_safe(flags);
Hmm, I think local_irq_save(flags) will compile much better. Obviously
this patch hasn't seen a compiler up close.
Also, you probably want a WARN_ON(in_nmi()) there, this function is
_NOT_ NMI safe.
we check that very early on:
unsigned int trace_call_bpf(struct bpf_prog *prog, void *ctx)
{
unsigned int ret;
if (in_nmi()) /* not supported yet */
return 1;
...
On Wed, Aug 05, 2015 at 12:04:25PM +0200, Peter Zijlstra wrote:
quoted
On Tue, Aug 04, 2015 at 08:58:15AM +0000, Kaixu Xia wrote:
quoted
quoted
+ event->ctx->task != current)
Strictly speaking we should hold rcu_read_lock around dereferencing
event->ctx (or have IRQs disabled -- although I know Paul doesn't like
us relying on that).
programs are always executing under rcu_read_lock, so we should be good
here too.
+ /*
+ * If the event is currently on this CPU, its either a per-task event,
+ * or local to this CPU. Furthermore it means its ACTIVE (otherwise
+ * oncpu == -1).
+ */
+ if (event->oncpu == smp_processor_id())
+ event->pmu->read(event);
+
+ val = local64_read(&event->count);
+ local_irq_restore(flags);
+
nice! cleaner and faster.
so raw_spin_lock(&ctx->lock) is not needed, because
update_*(event) methods are not called, right?
From: Peter Zijlstra <peterz@infradead.org> Date: 2015-08-05 16:22:05
On Wed, Aug 05, 2015 at 09:08:32AM -0700, Alexei Starovoitov wrote:
On 8/5/15 6:53 AM, Peter Zijlstra wrote:
quoted
+ /*
+ * If the event is currently on this CPU, its either a per-task event,
+ * or local to this CPU. Furthermore it means its ACTIVE (otherwise
+ * oncpu == -1).
+ */
+ if (event->oncpu == smp_processor_id())
+ event->pmu->read(event);
+
+ val = local64_read(&event->count);
+ local_irq_restore(flags);
+
nice! cleaner and faster.
so raw_spin_lock(&ctx->lock) is not needed, because
update_*(event) methods are not called, right?
Indeed, and by ensuring the event is indeed local (by force of WARN_ON)
disabling IRQs will avoid counter scheduling and result in a stable
event state.
On Wed, Aug 05, 2015 at 12:04:25PM +0200, Peter Zijlstra wrote:
quoted
Also, you probably want a WARN_ON(in_nmi()) there, this function is
_NOT_ NMI safe.
I had a wee think about that, and I think the below is safe.
(with the obvious problem that WARN from NMI context is not safe)
It does not give you up-to-date overcommit times but your version didn't
either so I'm assuming you don't need those, if you do need those it
needs more but we can do that too.
---
include/linux/perf_event.h | 1 +
kernel/events/core.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
s/local_irq_safe/local_irq_save, and I have compiled and tested this function
and it is fine. Will use it in the next set.
Thanks.
+
+ /* If this is a per-task event, it must be for current */
+ WARN_ON_ONCE((event->attach_state & PERF_ATTACH_TASK) &&
+ event->hw.target != current);
+
+ /* If this is a per-CPU event, it must be for this CPU */
+ WARN_ON_ONCE(!(event->attach_state & PERF_ATTACH_TASK) &&
+ event->cpu != smp_processor_id());
+
+ /*
+ * It must not be an event with inherit set, we cannot read
+ * all child counters from atomic context.
+ */
+ WARN_ON_ONCE(event->attr.inherit);
+
+ /*
+ * It must not have a pmu::count method, those are not
+ * NMI safe.
+ */
+ WARN_ON_ONCE(event->pmu->count);
+
+ /*
+ * If the event is currently on this CPU, its either a per-task event,
+ * or local to this CPU. Furthermore it means its ACTIVE (otherwise
+ * oncpu == -1).
+ */
+ if (event->oncpu == smp_processor_id())
+ event->pmu->read(event);
+
+ val = local64_read(&event->count);
+ local_irq_restore(flags);
+
+ return val;
+}
+
static u64 perf_event_read(struct perf_event *event)
{
/*
.