Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1199579 > unrolled thread

[PATCH v6 0/4] bpf: Introduce the new ability of eBPF programs to access hardware PMU counter

Back to article view | Back to linux.kernel

Contents

  [PATCH v6 0/4] bpf: Introduce the new ability of eBPF programs to access hardware PMU counter Kaixu Xia <xiakaixu@huawei.com> - 2015-08-04 11:10 +0200
    [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter Kaixu Xia <xiakaixu@huawei.com> - 2015-08-04 11:10 +0200
      Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that  get the selected hardware PMU conuter Alexei Starovoitov <ast@plumgrid.com> - 2015-08-04 20:00 +0200
        Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read()  that get the selected hardware PMU conuter xiakaixu <xiakaixu@huawei.com> - 2015-08-05 04:10 +0200
      Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read()  that get the selected hardware PMU conuter Peter Zijlstra <peterz@infradead.org> - 2015-08-05 12:10 +0200
        Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read()  that get the selected hardware PMU conuter Peter Zijlstra <peterz@infradead.org> - 2015-08-05 12:20 +0200
          Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that  get the selected hardware PMU conuter Alexei Starovoitov <ast@plumgrid.com> - 2015-08-05 18:10 +0200
        Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read()  that get the selected hardware PMU conuter xiakaixu <xiakaixu@huawei.com> - 2015-08-05 12:40 +0200
        Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read()  that get the selected hardware PMU conuter Peter Zijlstra <peterz@infradead.org> - 2015-08-05 16:00 +0200
          Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read()  that get the selected hardware PMU conuter Peter Zijlstra <peterz@infradead.org> - 2015-08-05 16:00 +0200
          Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that  get the selected hardware PMU conuter Alexei Starovoitov <ast@plumgrid.com> - 2015-08-05 18:10 +0200
            Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read()  that get the selected hardware PMU conuter Peter Zijlstra <peterz@infradead.org> - 2015-08-05 18:30 +0200
          Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read()  that get the selected hardware PMU conuter xiakaixu <xiakaixu@huawei.com> - 2015-08-06 04:50 +0200
        Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that  get the selected hardware PMU conuter Alexei Starovoitov <ast@plumgrid.com> - 2015-08-05 18:00 +0200
    Re: [PATCH v6 0/4] bpf: Introduce the new ability of eBPF programs  to access hardware PMU counter Peter Zijlstra <peterz@infradead.org> - 2015-08-05 12:10 +0200

#1199579 — [PATCH v6 0/4] bpf: Introduce the new ability of eBPF programs to access hardware PMU counter

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1199582 — [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

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 <xiakaixu@huawei.com>
---
 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(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d0b394a..db9f781 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -190,6 +190,7 @@ extern const struct bpf_func_proto bpf_map_lookup_elem_proto;
 extern const struct bpf_func_proto bpf_map_update_elem_proto;
 extern const struct bpf_func_proto bpf_map_delete_elem_proto;
 
+extern const struct bpf_func_proto bpf_perf_event_read_proto;
 extern const struct bpf_func_proto bpf_get_prandom_u32_proto;
 extern const struct bpf_func_proto bpf_get_smp_processor_id_proto;
 extern const struct bpf_func_proto bpf_tail_call_proto;
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 81fc99e..6f1e448 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -643,6 +643,7 @@ extern void perf_event_free_task(struct task_struct *task);
 extern void perf_event_delayed_put(struct task_struct *task);
 extern struct perf_event *perf_event_get(unsigned int fd);
 extern struct perf_event_attr *perf_event_attrs(struct perf_event *event);
+extern u64 perf_event_read_internal(struct perf_event *event);
 extern void perf_event_print_debug(void);
 extern void perf_pmu_disable(struct pmu *pmu);
 extern void perf_pmu_enable(struct pmu *pmu);
@@ -986,6 +987,7 @@ static inline struct perf_event_attr *perf_event_attrs(struct perf_event *event)
 {
 	return ERR_PTR(-EINVAL);
 }
+static inline u64 perf_event_read_internal(struct perf_event *event)	{ return -EINVAL; }
 static inline void perf_event_print_debug(void)				{ }
 static inline int perf_event_task_disable(void)				{ return -EINVAL; }
 static inline int perf_event_task_enable(void)				{ return -EINVAL; }
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 69a1f6b..b9b13ce 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -250,6 +250,7 @@ enum bpf_func_id {
 	 * Return: 0 on success
 	 */
 	BPF_FUNC_get_current_comm,
+	BPF_FUNC_perf_event_read,	/* u64 bpf_perf_event_read(&map, index) */
 	__BPF_FUNC_MAX_ID,
 };
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 039d866..45fae14 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -238,6 +238,14 @@ static const char * const reg_type_str[] = {
 	[CONST_IMM]		= "imm",
 };
 
+static const struct {
+	int map_type;
+	int func_id;
+} func_limit[] = {
+	{BPF_MAP_TYPE_PROG_ARRAY, BPF_FUNC_tail_call},
+	{BPF_MAP_TYPE_PERF_EVENT_ARRAY, BPF_FUNC_perf_event_read},
+};
+
 static void print_verifier_state(struct verifier_env *env)
 {
 	enum bpf_reg_type t;
@@ -833,6 +841,29 @@ static int check_func_arg(struct verifier_env *env, u32 regno,
 	return err;
 }
 
+static int check_func_limit(struct bpf_map **mapp, int func_id)
+{
+	struct bpf_map *map = *mapp;
+	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;
+	}
+
+	return 0;
+}
+
 static int check_call(struct verifier_env *env, int func_id)
 {
 	struct verifier_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:
-		 * only allow to pass it into bpf_tail_call() for now.
-		 * bpf_map_delete_elem() can be allowed in the future,
-		 * while bpf_map_update_elem() must only be done via syscall
-		 */
-		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)
+		return err;
 
 	return 0;
 }
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 6251b53..726ca1b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8599,6 +8599,25 @@ struct perf_event_attr *perf_event_attrs(struct perf_event *event)
 	return &event->attr;
 }
 
+u64 perf_event_read_internal(struct perf_event *event)
+{
+	if (!event)
+		return -EINVAL;
+
+	if (unlikely(event->state != PERF_EVENT_STATE_ACTIVE))
+		return -EINVAL;
+
+	if (event->oncpu != raw_smp_processor_id() &&
+	    event->ctx->task != current)
+		return -EINVAL;
+
+	if (unlikely(event->attr.inherit))
+		return -EINVAL;
+
+	__perf_event_read(event);
+	return perf_event_count(event);
+}
+
 /*
  * inherit a event from parent task to child task:
  */
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 88a041a..7d7b724 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -158,6 +158,35 @@ const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
 	return &bpf_trace_printk_proto;
 }
 
+static u64 bpf_perf_event_read(u64 r1, u64 index, u64 r3, u64 r4, u64 r5)
+{
+	struct bpf_map *map = (struct bpf_map *) (unsigned long) r1;
+	struct bpf_array *array = container_of(map, struct bpf_array, map);
+	struct perf_event *event;
+
+	if (unlikely(index >= array->map.max_entries))
+		return -E2BIG;
+
+	event = (struct perf_event *)array->ptrs[index];
+	if (!event)
+		return -ENOENT;
+
+	/*
+	 * we don't know if the function is run successfully by the
+	 * return value. It can be judged in other places, such as
+	 * eBPF programs.
+	 */
+	return perf_event_read_internal(event);
+}
+
+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_ANYTHING,
+};
+
 static const struct bpf_func_proto *kprobe_prog_func_proto(enum bpf_func_id func_id)
 {
 	switch (func_id) {
@@ -183,6 +212,8 @@ static const struct bpf_func_proto *kprobe_prog_func_proto(enum bpf_func_id func
 		return bpf_get_trace_printk_proto();
 	case BPF_FUNC_get_smp_processor_id:
 		return &bpf_get_smp_processor_id_proto;
+	case BPF_FUNC_perf_event_read:
+		return &bpf_perf_event_read_proto;
 	default:
 		return NULL;
 	}
-- 
1.8.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200191 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

On 8/4/15 1:58 AM, Kaixu Xia wrote:
> +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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200355 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

于 2015/8/5 1:55, Alexei Starovoitov 写道:
> On 8/4/15 1:58 AM, Kaixu Xia wrote:
>> +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.

Thanks for your review! I will follow them in the next set.
> 
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200620 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

On Tue, Aug 04, 2015 at 08:58:15AM +0000, Kaixu Xia wrote:
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 6251b53..726ca1b 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8599,6 +8599,25 @@ struct perf_event_attr *perf_event_attrs(struct perf_event *event)
>  	return &event->attr;
>  }
>  
> +u64 perf_event_read_internal(struct perf_event *event)

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?

> +	__perf_event_read(event);
> +	return perf_event_count(event);
> +}

Also, you probably want a WARN_ON(in_nmi()) there, this function is
_NOT_ NMI safe.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200624 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

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:

> > +	    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).


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200942 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

On 8/5/15 3:15 AM, Peter Zijlstra wrote:
> 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:
>
>>> +	    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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200636 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

于 2015/8/5 18:04, Peter Zijlstra 写道:
> On Tue, Aug 04, 2015 at 08:58:15AM +0000, Kaixu Xia wrote:
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 6251b53..726ca1b 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -8599,6 +8599,25 @@ struct perf_event_attr *perf_event_attrs(struct perf_event *event)
>>  	return &event->attr;
>>  }
>>  
>> +u64 perf_event_read_internal(struct perf_event *event)
> 
> 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?

Thanks for your review! I've not tested it, will do that from now on.
> 
>> +	__perf_event_read(event);
>> +	return perf_event_count(event);
>> +}
> 
> Also, you probably want a WARN_ON(in_nmi()) there, this function is
> _NOT_ NMI safe.
> 
> 
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200796 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

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(+)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 2027809433b3..64e821dd64f0 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -659,6 +659,7 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr,
 				void *context);
 extern void perf_pmu_migrate_context(struct pmu *pmu,
 				int src_cpu, int dst_cpu);
+extern u64 perf_event_read_local(struct perf_event *event);
 extern u64 perf_event_read_value(struct perf_event *event,
 				 u64 *enabled, u64 *running);
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 39753bfd9520..7105d37763c1 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3222,6 +3222,59 @@ static inline u64 perf_event_count(struct perf_event *event)
 	return __perf_event_count(event);
 }
 
+/*
+ * 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);
+
+	/* 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)
 {
 	/*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200807 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200940 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

On 8/5/15 6:53 AM, Peter Zijlstra wrote:
> +	/*
> +	 * 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?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200967 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

On Wed, Aug 05, 2015 at 09:08:32AM -0700, Alexei Starovoitov wrote:
> On 8/5/15 6:53 AM, Peter Zijlstra wrote:
> >+	/*
> >+	 * 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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201399 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

于 2015/8/5 21:53, Peter Zijlstra 写道:
> 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(+)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 2027809433b3..64e821dd64f0 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -659,6 +659,7 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr,
>  				void *context);
>  extern void perf_pmu_migrate_context(struct pmu *pmu,
>  				int src_cpu, int dst_cpu);
> +extern u64 perf_event_read_local(struct perf_event *event);
>  extern u64 perf_event_read_value(struct perf_event *event,
>  				 u64 *enabled, u64 *running);
>  
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 39753bfd9520..7105d37763c1 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3222,6 +3222,59 @@ static inline u64 perf_event_count(struct perf_event *event)
>  	return __perf_event_count(event);
>  }
>  
> +/*
> + * 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);

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)
>  {
>  	/*
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200930 — Re: [PATCH v6 3/4] bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter

On 8/5/15 3:04 AM, Peter Zijlstra wrote:
>> >+	__perf_event_read(event);
>> >+	return perf_event_count(event);
>> >+}
> 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;
...

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200619 — Re: [PATCH v6 0/4] bpf: Introduce the new ability of eBPF programs to access hardware PMU counter

Please split out the core perf API stuff into separate patches.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web