[PATCH net-next] bpf: update perf event helper function signature and documentation

[PATCH net-next] bpf: update perf event helper function signature and documentation

[Teng Qin]

From: Teng Qin <qinteng@fb.com>

This commit updates function signature of the bpf_perf_event_output and
bpf_perf_event_read helpers to match their implementation. Also updates
their documentation in the header files.

Signed-off-by: Teng Qin <qinteng@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 include/uapi/linux/bpf.h       | 11 +++++++----
 samples/bpf/bpf_helpers.h      |  5 ++---
 tools/include/uapi/linux/bpf.h | 11 +++++++----
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 94dfa9d..e78aece 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -313,8 +313,11 @@ union bpf_attr {
  *     @flags: room for future extensions
  *     Return: 0 on success or negative error
  *
- * u64 bpf_perf_event_read(&map, index)
- *     Return: Number events read or error code
+ * u64 bpf_perf_event_read(map, flags)
+ *     read perf event counter value
+ *     @map: pointer to perf_event_array map
+ *     @flags: index of event in the map or bitmask flags
+ *     Return: value of perf event counter read or error code
  *
  * int bpf_redirect(ifindex, flags)
  *     redirect to another netdev
@@ -328,11 +331,11 @@ union bpf_attr {
  *     @skb: pointer to skb
  *     Return: realm if != 0
  *
- * int bpf_perf_event_output(ctx, map, index, data, size)
+ * int bpf_perf_event_output(ctx, map, flags, data, size)
  *     output perf raw sample
  *     @ctx: struct pt_regs*
  *     @map: pointer to perf_event_array map
- *     @index: index of event in the map
+ *     @flags: index of event in the map or bitmask flags
  *     @data: data on stack to be output as raw data
  *     @size: size of data
  *     Return: 0 on success or negative error
diff --git a/samples/bpf/bpf_helpers.h b/samples/bpf/bpf_helpers.h
index 9a9c95f..a94ce42 100644
--- a/samples/bpf/bpf_helpers.h
+++ b/samples/bpf/bpf_helpers.h
@@ -37,9 +37,8 @@ static int (*bpf_clone_redirect)(void *ctx, int ifindex, int flags) =
 	(void *) BPF_FUNC_clone_redirect;
 static int (*bpf_redirect)(int ifindex, int flags) =
 	(void *) BPF_FUNC_redirect;
-static int (*bpf_perf_event_output)(void *ctx, void *map,
-				    unsigned long long flags, void *data,
-				    int size) =
+static int (*bpf_perf_event_output)(void *ctx, void *map, u64 flags,
+	                            void *data, int size) =
 	(void *) BPF_FUNC_perf_event_output;
 static int (*bpf_get_stackid)(void *ctx, void *map, int flags) =
 	(void *) BPF_FUNC_get_stackid;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 94dfa9d..e78aece 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -313,8 +313,11 @@ union bpf_attr {
  *     @flags: room for future extensions
  *     Return: 0 on success or negative error
  *
- * u64 bpf_perf_event_read(&map, index)
- *     Return: Number events read or error code
+ * u64 bpf_perf_event_read(map, flags)
+ *     read perf event counter value
+ *     @map: pointer to perf_event_array map
+ *     @flags: index of event in the map or bitmask flags
+ *     Return: value of perf event counter read or error code
  *
  * int bpf_redirect(ifindex, flags)
  *     redirect to another netdev
@@ -328,11 +331,11 @@ union bpf_attr {
  *     @skb: pointer to skb
  *     Return: realm if != 0
  *
- * int bpf_perf_event_output(ctx, map, index, data, size)
+ * int bpf_perf_event_output(ctx, map, flags, data, size)
  *     output perf raw sample
  *     @ctx: struct pt_regs*
  *     @map: pointer to perf_event_array map
- *     @index: index of event in the map
+ *     @flags: index of event in the map or bitmask flags
  *     @data: data on stack to be output as raw data
  *     @size: size of data
  *     Return: 0 on success or negative error
-- 
2.9.3

Re: [PATCH net-next] bpf: update perf event helper function signature and documentation

[David Miller]

From: Teng Qin <qinteng@fb.com>
Date: Tue, 23 May 2017 00:39:34 +0000

> diff --git a/samples/bpf/bpf_helpers.h b/samples/bpf/bpf_helpers.h
> index 9a9c95f..a94ce42 100644
> --- a/samples/bpf/bpf_helpers.h
> +++ b/samples/bpf/bpf_helpers.h
> @@ -37,9 +37,8 @@ static int (*bpf_clone_redirect)(void *ctx, int ifindex, int flags) =
>  	(void *) BPF_FUNC_clone_redirect;
>  static int (*bpf_redirect)(int ifindex, int flags) =
>  	(void *) BPF_FUNC_redirect;
> -static int (*bpf_perf_event_output)(void *ctx, void *map,
> -				    unsigned long long flags, void *data,
> -				    int size) =
> +static int (*bpf_perf_event_output)(void *ctx, void *map, u64 flags,
> +	                            void *data, int size) =
>  	(void *) BPF_FUNC_perf_event_output;
>  static int (*bpf_get_stackid)(void *ctx, void *map, int flags) =
>  	(void *) BPF_FUNC_get_stackid;

I think we've been intentionally avoiding the use of "u64", "u32",
etc. in this file.

But what do I know.

Re: [PATCH net-next] bpf: update perf event helper function signature and documentation

[Teng Qin]

On 5/22/17, 20:08, "David Miller" <davem@davemloft.net> wrote:

    From: Teng Qin <qinteng@fb.com>
    Date: Tue, 23 May 2017 00:39:34 +0000
    
    > diff --git a/samples/bpf/bpf_helpers.h b/samples/bpf/bpf_helpers.h
    > index 9a9c95f..a94ce42 100644
    > --- a/samples/bpf/bpf_helpers.h
    > +++ b/samples/bpf/bpf_helpers.h
    > @@ -37,9 +37,8 @@ static int (*bpf_clone_redirect)(void *ctx, int ifindex, int flags) =
    >  	(void *) BPF_FUNC_clone_redirect;
    >  static int (*bpf_redirect)(int ifindex, int flags) =
    >  	(void *) BPF_FUNC_redirect;
    > -static int (*bpf_perf_event_output)(void *ctx, void *map,
    > -				    unsigned long long flags, void *data,
    > -				    int size) =
    > +static int (*bpf_perf_event_output)(void *ctx, void *map, u64 flags,
    > +	                            void *data, int size) =
    >  	(void *) BPF_FUNC_perf_event_output;
    >  static int (*bpf_get_stackid)(void *ctx, void *map, int flags) =
    >  	(void *) BPF_FUNC_get_stackid;
    
    I think we've been intentionally avoiding the use of "u64", "u32",
    etc. in this file.
    
    But what do I know.

Alexei said it was due to Clang not taking u64, u32 etc. for compilation.
I didn’t know the context and just used them. But apparently, something
changed and now they build and run OK......

Re: [PATCH net-next] bpf: update perf event helper function signature and documentation

[David Miller]

From: Teng Qin <qinteng@fb.com>
Date: Tue, 23 May 2017 03:17:31 +0000

> Alexei said it was due to Clang not taking u64, u32 etc. for compilation.
> I didn’t know the context and just used them. But apparently, something
> changed and now they build and run OK......

These types are %100 compiler agnostic.  Clang doesn't care.  All
compilers can understand:

	typedef unsigned int u32;

So it's not a Clang specific issue.

The problem could only have to do with whether the types get typdef'd
or not with the header files that are used in conjunction with the
file we are talking about here.

And if we do get a proper set of defines from linux/types.h these days
we should:

1) Make that explicit, by including linux/types.h in bpf_helpers.h

2) Use those types universally in this file where we were avoiding
   doing so

This header file also uses "SEC" and expects to get this definition
implicitly, and therefore there is another header file that
bpf_helpers.h must include in order to explicitly and reliably have
that definition.

Just changing one prototype, and relying on getting the types
indirectly by other header files including by whoever incudes
bpf_helpers.h is not the way to go.

Thanks.