Messages in this thread | | | Date | Thu, 23 Jul 2015 10:12:53 +0800 | From | xiakaixu <> | Subject | Re: [PATCH v2 4/5] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter |
| |
于 2015/7/23 9:14, Alexei Starovoitov 写道: > On 7/22/15 1:09 AM, Kaixu Xia wrote: >> According to the perf_event_map_fd and key, 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. >> >> The key can't be passed to bpf_perf_event_read() directly >> because the function argument constraint is lacked. > > I don't understand above sentence. > >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >> index 69a1f6b..e3bb181 100644 >> --- a/include/uapi/linux/bpf.h >> +++ b/include/uapi/linux/bpf.h >> @@ -250,6 +250,8 @@ enum bpf_func_id { >> * Return: 0 on success >> */ >> BPF_FUNC_get_current_comm, >> + >> + BPF_FUNC_perf_event_read, /* u64 bpf_perf_event_read(&map, &key) */ > > no need for extra empty line. > >> + >> +static u64 bpf_perf_event_read(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5) >> +{ >> + struct bpf_map *map = (struct bpf_map *) (unsigned long) r1; >> + void *key = (void *) (unsigned long) r2; >> + struct perf_event *event; >> + void *ptr; >> + >> + if (map->map_type != BPF_MAP_TYPE_PERF_EVENT_ARRAY) >> + return -EINVAL; > > please check this statically in verifier instead of in run-time. > >> + >> + rcu_read_lock(); > > unnecessary. > >> + ptr = map->ops->map_lookup_elem(map, key); >> + rcu_read_unlock(); >> + if (!ptr || !(*(unsigned long *)ptr)) >> + return -EBADF; > > all these casts can be removed. First cast of 'r1' into > perf_event_array will be enough.
So you mean like this?
u64 bpf_perf_event_read(u64 r1, u64 index,...) { struct bpf_perf_event_array *array = (void *) (long) r1; struct perf_event *event; ... event = array->events[index]; ... } > >> +const struct bpf_func_proto bpf_perf_event_read_proto = { >> + .func = bpf_perf_event_read, >> + .gpl_only = false, >> + .ret_type = RET_INTEGER, >> + .arg1_type = ARG_CONST_MAP_PTR, >> + .arg2_type = ARG_PTR_TO_MAP_KEY, > > make it arg2_type = ARG_ANYTHING then you'll just index > into array the way prog_array does and similar to bpf_tail_call.
ARG_ANYTHING means any (initialized) argument is ok, but we here really want is map key. So I'm not sure ARG_ANYTHING is suitable. You know ARG_ANYTHING is not checked enough in verifier.
Thanks. > > > . >
| |