lkml.org 
[lkml]   [2016]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCHv2 perf/core 2/2] tools lib bpf: Sync with samples/bpf/libbpf
From
Date
I'm also working on improving bpf.c. Please have a look at:

https://lkml.org/lkml/2016/11/14/1078

Since bpf.c is simple, I think we can add more functions and fixes
gradually, instead of a full copy.

See my inline comment below.

On 2016/11/17 1:43, Joe Stringer wrote:
> Extend the tools/ version of libbpf to include all of the functionality
> provided in the samples/bpf version.
>
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
> v2: Don't shift non-bpf changes across.
> Various type cleanups, removal of extraneous declarations
> ---
> tools/lib/bpf/bpf.c | 107 ++++++++++++++++++++------
> tools/lib/bpf/bpf.h | 202 +++++++++++++++++++++++++++++++++++++++++++++++--
> tools/lib/bpf/libbpf.c | 3 +-
> 3 files changed, 279 insertions(+), 33 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 4212ed62235b..5e061851ac00 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -20,10 +20,17 @@
> */
>
> #include <stdlib.h>
> -#include <memory.h>
> +#include <stdio.h>
> #include <unistd.h>
> #include <asm/unistd.h>
> +#include <string.h>
> +#include <linux/netlink.h>
> #include <linux/bpf.h>
> +#include <errno.h>
> +#include <net/ethernet.h>
> +#include <net/if.h>
> +#include <linux/if_packet.h>
> +#include <arpa/inet.h>
> #include "bpf.h"
>

Why we need these network related headers?

> /*
> @@ -53,24 +60,71 @@ static int sys_bpf(enum bpf_cmd cmd, union bpf_attr *attr,
> return syscall(__NR_bpf, cmd, attr, size);
> }
>
> -int bpf_create_map(enum bpf_map_type map_type, int key_size,
> - int value_size, int max_entries)
> +int bpf_create_map(enum bpf_map_type map_type, int key_size, int value_size,
> + int max_entries, int map_flags)
> {
> - union bpf_attr attr;
> + union bpf_attr attr = {
> + .map_type = map_type,
> + .key_size = key_size,
> + .value_size = value_size,
> + .max_entries = max_entries,
> + .map_flags = map_flags,
> + };
>
> - memset(&attr, '\0', sizeof(attr));
> + return sys_bpf(BPF_MAP_CREATE, &attr, sizeof(attr));
> +}
>

I lost map_flags in original bpf.c. Thanks to your patch. map_flags is
useful
when creating BPF_MAP_TYPE_PERCPU_HASH: BPF_F_NO_PREALLOC is meanful in this
case.

Although it is okay in samples, I still prefer a explicit bzero() or
memset(),
because kernel checks if unused field in this union is zero. However
I'll check
c standard to see how unused field would be initialized.


<SNIP>

> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index e8ba54087497..4dba36995771 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -23,16 +23,202 @@
>
> #include <linux/bpf.h>
>
> +struct bpf_insn;
> +
> int bpf_create_map(enum bpf_map_type map_type, int key_size, int value_size,
> - int max_entries);
> + int max_entries, int map_flags);
> +int bpf_update_elem(int fd, void *key, void *value, unsigned long long flags);
> +int bpf_lookup_elem(int fd, void *key, void *value);
> +int bpf_delete_elem(int fd, void *key);
> +int bpf_get_next_key(int fd, void *key, void *next_key);
> +
> +int bpf_load_program(enum bpf_prog_type prog_type,
> + const struct bpf_insn *insns, int insn_len,
> + const char *license, int kern_version,
> + char *log_buf, size_t log_buf_sz);
> +
> +int bpf_obj_pin(int fd, const char *pathname);
> +int bpf_obj_get(const char *pathname);
>
> -/* Recommend log buffer size */
> #define BPF_LOG_BUF_SIZE 65536
> -int bpf_load_program(enum bpf_prog_type type, struct bpf_insn *insns,
> - size_t insns_cnt, char *license,
> - u32 kern_version, char *log_buf,
> - size_t log_buf_sz);
>
> -int bpf_map_update_elem(int fd, void *key, void *value,
> - u64 flags);
> +/* ALU ops on registers, bpf_add|sub|...: dst_reg += src_reg */
> +
> +#define BPF_ALU64_REG(OP, DST, SRC) \
> + ((struct bpf_insn) { \
> + .code = BPF_ALU64 | BPF_OP(OP) | BPF_X, \
> + .dst_reg = DST, \
> + .src_reg = SRC, \
> + .off = 0, \
> + .imm = 0 })
> +

Should we define these macros here? They are in include/linux/filter.h
and duplicated in tools/include/linux/filter.h. Redefining them here
would cause conflict.

Thank you.


\
 
 \ /
  Last update: 2016-11-17 03:11    [W:0.155 / U:0.352 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site