| Message ID | 20201117145644.1166255-2-danieltimlee@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: remove bpf_load loader completely | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 139 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Nov 17, 2020 at 02:56:36PM +0000, Daniel T. Lee wrote: > Under the samples/bpf directory, similar tracing helpers are > fragmented around. To keep consistent of tracing programs, this commit > moves the helper and define locations to increase the reuse of each > helper function. > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > --- > samples/bpf/Makefile | 2 +- > samples/bpf/hbm.c | 51 ++++----------------- > tools/testing/selftests/bpf/trace_helpers.c | 33 ++++++++++++- > tools/testing/selftests/bpf/trace_helpers.h | 3 ++ > 4 files changed, 45 insertions(+), 44 deletions(-) > > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > index aeebf5d12f32..3e83cd5ca1c2 100644 > --- a/samples/bpf/Makefile > +++ b/samples/bpf/Makefile > @@ -110,7 +110,7 @@ xdp_fwd-objs := xdp_fwd_user.o > task_fd_query-objs := bpf_load.o task_fd_query_user.o $(TRACE_HELPERS) > xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) > ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) > -hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS) > +hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) > > # Tell kbuild to always build the programs > always-y := $(tprogs-y) > diff --git a/samples/bpf/hbm.c b/samples/bpf/hbm.c > index 400e741a56eb..b9f9f771dd81 100644 > --- a/samples/bpf/hbm.c > +++ b/samples/bpf/hbm.c > @@ -48,6 +48,7 @@ > > #include "bpf_load.h" > #include "bpf_rlimit.h" > +#include "trace_helpers.h" > #include "cgroup_helpers.h" > #include "hbm.h" > #include "bpf_util.h" > @@ -65,51 +66,12 @@ bool no_cn_flag; > bool edt_flag; > > static void Usage(void); > -static void read_trace_pipe2(void); > static void do_error(char *msg, bool errno_flag); > > -#define DEBUGFS "/sys/kernel/debug/tracing/" > - > struct bpf_object *obj; > int bpfprog_fd; > int cgroup_storage_fd; > > -static void read_trace_pipe2(void) > -{ > - int trace_fd; > - FILE *outf; > - char *outFname = "hbm_out.log"; > - > - trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0); > - if (trace_fd < 0) { > - printf("Error opening trace_pipe\n"); > - return; > - } > - > -// Future support of ingress > -// if (!outFlag) > -// outFname = "hbm_in.log"; > - outf = fopen(outFname, "w"); > - > - if (outf == NULL) > - printf("Error creating %s\n", outFname); > - > - while (1) { > - static char buf[4097]; > - ssize_t sz; > - > - sz = read(trace_fd, buf, sizeof(buf) - 1); > - if (sz > 0) { > - buf[sz] = 0; > - puts(buf); > - if (outf != NULL) { > - fprintf(outf, "%s\n", buf); > - fflush(outf); > - } > - } > - } > -} > - > static void do_error(char *msg, bool errno_flag) > { > if (errno_flag) > @@ -392,8 +354,15 @@ static int run_bpf_prog(char *prog, int cg_id) > fclose(fout); > } > > - if (debugFlag) > - read_trace_pipe2(); > + if (debugFlag) { > + char *out_fname = "hbm_out.log"; > + /* Future support of ingress */ > + // if (!outFlag) > + // out_fname = "hbm_in.log"; > + > + read_trace_pipe2(out_fname); > + } > + > return rc; > err: > rc = 1; > diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c > index 1bbd1d9830c8..b7c184e109e8 100644 > --- a/tools/testing/selftests/bpf/trace_helpers.c > +++ b/tools/testing/selftests/bpf/trace_helpers.c > @@ -11,8 +11,6 @@ > #include <sys/mman.h> > #include "trace_helpers.h" > > -#define DEBUGFS "/sys/kernel/debug/tracing/" Is this change needed? > - > #define MAX_SYMS 300000 > static struct ksym syms[MAX_SYMS]; > static int sym_cnt; > @@ -136,3 +134,34 @@ void read_trace_pipe(void) > } > } > } > + > +void read_trace_pipe2(char *filename) > +{ > + int trace_fd; > + FILE *outf; > + > + trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0); > + if (trace_fd < 0) { > + printf("Error opening trace_pipe\n"); > + return; > + } > + > + outf = fopen(filename, "w"); > + if (!outf) > + printf("Error creating %s\n", filename); > + > + while (1) { > + static char buf[4096]; > + ssize_t sz; > + > + sz = read(trace_fd, buf, sizeof(buf) - 1); > + if (sz > 0) { > + buf[sz] = 0; > + puts(buf); > + if (outf) { > + fprintf(outf, "%s\n", buf); > + fflush(outf); > + } > + } > + } It needs a fclose(). IIUC, this function will never return. I am not sure this is something that should be made available to selftests.
On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > Under the samples/bpf directory, similar tracing helpers are > fragmented around. To keep consistent of tracing programs, this commit > moves the helper and define locations to increase the reuse of each > helper function. > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > --- > samples/bpf/Makefile | 2 +- > samples/bpf/hbm.c | 51 ++++----------------- > tools/testing/selftests/bpf/trace_helpers.c | 33 ++++++++++++- > tools/testing/selftests/bpf/trace_helpers.h | 3 ++ > 4 files changed, 45 insertions(+), 44 deletions(-) > > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > index aeebf5d12f32..3e83cd5ca1c2 100644 > --- a/samples/bpf/Makefile > +++ b/samples/bpf/Makefile > @@ -110,7 +110,7 @@ xdp_fwd-objs := xdp_fwd_user.o > task_fd_query-objs := bpf_load.o task_fd_query_user.o $(TRACE_HELPERS) > xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) > ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) > -hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS) > +hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) > > # Tell kbuild to always build the programs > always-y := $(tprogs-y) > diff --git a/samples/bpf/hbm.c b/samples/bpf/hbm.c > index 400e741a56eb..b9f9f771dd81 100644 > --- a/samples/bpf/hbm.c > +++ b/samples/bpf/hbm.c > @@ -48,6 +48,7 @@ > > #include "bpf_load.h" > #include "bpf_rlimit.h" > +#include "trace_helpers.h" > #include "cgroup_helpers.h" > #include "hbm.h" > #include "bpf_util.h" > @@ -65,51 +66,12 @@ bool no_cn_flag; > bool edt_flag; > > static void Usage(void); > -static void read_trace_pipe2(void); > static void do_error(char *msg, bool errno_flag); > > -#define DEBUGFS "/sys/kernel/debug/tracing/" > - > struct bpf_object *obj; > int bpfprog_fd; > int cgroup_storage_fd; > > -static void read_trace_pipe2(void) This is used only in hbm.c, why move it into trace_helpers.c? > -{ > - int trace_fd; > - FILE *outf; > - char *outFname = "hbm_out.log"; > - [...]
On Wed, Nov 18, 2020 at 10:19 AM Martin KaFai Lau <kafai@fb.com> wrote: > > On Tue, Nov 17, 2020 at 02:56:36PM +0000, Daniel T. Lee wrote: > > Under the samples/bpf directory, similar tracing helpers are > > fragmented around. To keep consistent of tracing programs, this commit > > moves the helper and define locations to increase the reuse of each > > helper function. > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > > > --- > > samples/bpf/Makefile | 2 +- > > samples/bpf/hbm.c | 51 ++++----------------- > > tools/testing/selftests/bpf/trace_helpers.c | 33 ++++++++++++- > > tools/testing/selftests/bpf/trace_helpers.h | 3 ++ > > 4 files changed, 45 insertions(+), 44 deletions(-) > > > > [...] > > > > -#define DEBUGFS "/sys/kernel/debug/tracing/" > Is this change needed? This macro can be used in other samples such as the 4th' patch of this patchset, task_fd_query. > > - > > #define MAX_SYMS 300000 > > static struct ksym syms[MAX_SYMS]; > > static int sym_cnt; > > @@ -136,3 +134,34 @@ void read_trace_pipe(void) > > } > > } > > } > > + > > +void read_trace_pipe2(char *filename) > > +{ > > + int trace_fd; > > + FILE *outf; > > + > > + trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0); > > + if (trace_fd < 0) { > > + printf("Error opening trace_pipe\n"); > > + return; > > + } > > + > > + outf = fopen(filename, "w"); > > + if (!outf) > > + printf("Error creating %s\n", filename); > > + > > + while (1) { > > + static char buf[4096]; > > + ssize_t sz; > > + > > + sz = read(trace_fd, buf, sizeof(buf) - 1); > > + if (sz > 0) { > > + buf[sz] = 0; > > + puts(buf); > > + if (outf) { > > + fprintf(outf, "%s\n", buf); > > + fflush(outf); > > + } > > + } > > + } > It needs a fclose(). > > IIUC, this function will never return. I am not sure > this is something that should be made available to selftests. Actually, read_trace_pipe and read_trace_pipe2 are helpers that are only used under samples directory. Since these helpers are not used in selftests, What do you think about moving these helpers to something like samples/bpf/trace_pipe.h? Thanks for your time and effort for the review.
On Wed, Nov 18, 2020 at 10:58 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > Under the samples/bpf directory, similar tracing helpers are > > fragmented around. To keep consistent of tracing programs, this commit > > moves the helper and define locations to increase the reuse of each > > helper function. > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > > > --- > > [...] > > -static void read_trace_pipe2(void) > > This is used only in hbm.c, why move it into trace_helpers.c? > I think this function can be made into a helper that can be used in other programs. Which is basically same as 'read_trace_pipe' and also writes the content of that pipe to file either. Well, it's not used anywhere else, but I moved this function for the potential of reuse. Since these 'read_trace_pipe's are helpers that are only used under samples directory, what do you think about moving these helpers to something like samples/bpf/trace_pipe.h? Thanks for your time and effort for the review.
On Tue, Nov 17, 2020 at 6:55 PM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > On Wed, Nov 18, 2020 at 10:58 AM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Nov 17, 2020 at 6:57 AM Daniel T. Lee <danieltimlee@gmail.com> wrote: > > > > > > Under the samples/bpf directory, similar tracing helpers are > > > fragmented around. To keep consistent of tracing programs, this commit > > > moves the helper and define locations to increase the reuse of each > > > helper function. > > > > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > > > > > > --- > > > [...] > > > -static void read_trace_pipe2(void) > > > > This is used only in hbm.c, why move it into trace_helpers.c? > > > > I think this function can be made into a helper that can be used in > other programs. Which is basically same as 'read_trace_pipe' and > also writes the content of that pipe to file either. Well, it's not used > anywhere else, but I moved this function for the potential of reuse. Let's not make premature extraction of helpers. We generally add helpers when we have a repeated need for them. It's not currently the case for read_trace_pipe2(). > > Since these 'read_trace_pipe's are helpers that are only used > under samples directory, what do you think about moving these > helpers to something like samples/bpf/trace_pipe.h? Nope, not yet. I'd just drop this patch for now (see my comments on another patch regarding DEBUGFS). > > Thanks for your time and effort for the review. > > -- > Best, > Daniel T. Lee
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile index aeebf5d12f32..3e83cd5ca1c2 100644 --- a/samples/bpf/Makefile +++ b/samples/bpf/Makefile @@ -110,7 +110,7 @@ xdp_fwd-objs := xdp_fwd_user.o task_fd_query-objs := bpf_load.o task_fd_query_user.o $(TRACE_HELPERS) xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS) ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS) -hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS) +hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS) $(TRACE_HELPERS) # Tell kbuild to always build the programs always-y := $(tprogs-y) diff --git a/samples/bpf/hbm.c b/samples/bpf/hbm.c index 400e741a56eb..b9f9f771dd81 100644 --- a/samples/bpf/hbm.c +++ b/samples/bpf/hbm.c @@ -48,6 +48,7 @@ #include "bpf_load.h" #include "bpf_rlimit.h" +#include "trace_helpers.h" #include "cgroup_helpers.h" #include "hbm.h" #include "bpf_util.h" @@ -65,51 +66,12 @@ bool no_cn_flag; bool edt_flag; static void Usage(void); -static void read_trace_pipe2(void); static void do_error(char *msg, bool errno_flag); -#define DEBUGFS "/sys/kernel/debug/tracing/" - struct bpf_object *obj; int bpfprog_fd; int cgroup_storage_fd; -static void read_trace_pipe2(void) -{ - int trace_fd; - FILE *outf; - char *outFname = "hbm_out.log"; - - trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0); - if (trace_fd < 0) { - printf("Error opening trace_pipe\n"); - return; - } - -// Future support of ingress -// if (!outFlag) -// outFname = "hbm_in.log"; - outf = fopen(outFname, "w"); - - if (outf == NULL) - printf("Error creating %s\n", outFname); - - while (1) { - static char buf[4097]; - ssize_t sz; - - sz = read(trace_fd, buf, sizeof(buf) - 1); - if (sz > 0) { - buf[sz] = 0; - puts(buf); - if (outf != NULL) { - fprintf(outf, "%s\n", buf); - fflush(outf); - } - } - } -} - static void do_error(char *msg, bool errno_flag) { if (errno_flag) @@ -392,8 +354,15 @@ static int run_bpf_prog(char *prog, int cg_id) fclose(fout); } - if (debugFlag) - read_trace_pipe2(); + if (debugFlag) { + char *out_fname = "hbm_out.log"; + /* Future support of ingress */ + // if (!outFlag) + // out_fname = "hbm_in.log"; + + read_trace_pipe2(out_fname); + } + return rc; err: rc = 1; diff --git a/tools/testing/selftests/bpf/trace_helpers.c b/tools/testing/selftests/bpf/trace_helpers.c index 1bbd1d9830c8..b7c184e109e8 100644 --- a/tools/testing/selftests/bpf/trace_helpers.c +++ b/tools/testing/selftests/bpf/trace_helpers.c @@ -11,8 +11,6 @@ #include <sys/mman.h> #include "trace_helpers.h" -#define DEBUGFS "/sys/kernel/debug/tracing/" - #define MAX_SYMS 300000 static struct ksym syms[MAX_SYMS]; static int sym_cnt; @@ -136,3 +134,34 @@ void read_trace_pipe(void) } } } + +void read_trace_pipe2(char *filename) +{ + int trace_fd; + FILE *outf; + + trace_fd = open(DEBUGFS "trace_pipe", O_RDONLY, 0); + if (trace_fd < 0) { + printf("Error opening trace_pipe\n"); + return; + } + + outf = fopen(filename, "w"); + if (!outf) + printf("Error creating %s\n", filename); + + while (1) { + static char buf[4096]; + ssize_t sz; + + sz = read(trace_fd, buf, sizeof(buf) - 1); + if (sz > 0) { + buf[sz] = 0; + puts(buf); + if (outf) { + fprintf(outf, "%s\n", buf); + fflush(outf); + } + } + } +} diff --git a/tools/testing/selftests/bpf/trace_helpers.h b/tools/testing/selftests/bpf/trace_helpers.h index f62fdef9e589..68c23bf55897 100644 --- a/tools/testing/selftests/bpf/trace_helpers.h +++ b/tools/testing/selftests/bpf/trace_helpers.h @@ -2,6 +2,8 @@ #ifndef __TRACE_HELPER_H #define __TRACE_HELPER_H +#define DEBUGFS "/sys/kernel/debug/tracing/" + #include <bpf/libbpf.h> struct ksym { @@ -17,5 +19,6 @@ long ksym_get_addr(const char *name); int kallsyms_find(const char *sym, unsigned long long *addr); void read_trace_pipe(void); +void read_trace_pipe2(char *filename); #endif
Under the samples/bpf directory, similar tracing helpers are fragmented around. To keep consistent of tracing programs, this commit moves the helper and define locations to increase the reuse of each helper function. Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> --- samples/bpf/Makefile | 2 +- samples/bpf/hbm.c | 51 ++++----------------- tools/testing/selftests/bpf/trace_helpers.c | 33 ++++++++++++- tools/testing/selftests/bpf/trace_helpers.h | 3 ++ 4 files changed, 45 insertions(+), 44 deletions(-)