* [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] @ 2019-06-21 22:33 Takshak Chahande 2019-06-24 14:22 ` Daniel Borkmann 0 siblings, 1 reply; 15+ messages in thread From: Takshak Chahande @ 2019-06-21 22:33 UTC (permalink / raw) To: netdev; +Cc: ast, daniel, rdna, ctakshak, kernel-team With different bpf attach_flags available to attach bpf programs specially with BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI, the list of effective bpf-programs available to any sub-cgroups really needs to be available for easy debugging. Using BPF_F_QUERY_EFFECTIVE flag, one can get the list of not only attached bpf-programs to a cgroup but also the inherited ones from parent cgroup. So "-e" option is introduced to use BPF_F_QUERY_EFFECTIVE query flag here to list all the effective bpf-programs available for execution at a specified cgroup. Reused modified test program test_cgroup_attach from tools/testing/selftests/bpf: # ./test_cgroup_attach With old bpftool (without -e option): # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ ID AttachType AttachFlags Name 271 egress multi pkt_cntr_1 272 egress multi pkt_cntr_2 Attached new program pkt_cntr_4 in cg2 gives following: # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2 ID AttachType AttachFlags Name 273 egress override pkt_cntr_4 And with new "-e" option it shows all effective programs for cg2: # bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2 ID AttachType AttachFlags Name 273 egress override pkt_cntr_4 271 egress override pkt_cntr_1 272 egress override pkt_cntr_2 Signed-off-by: Takshak Chahande <ctakshak@fb.com> Acked-by: Andrey Ignatov <rdna@fb.com> --- tools/bpf/bpftool/Documentation/bpftool-cgroup.rst | 8 +++++++- tools/bpf/bpftool/Documentation/bpftool.rst | 6 +++++- tools/bpf/bpftool/bash-completion/bpftool | 2 +- tools/bpf/bpftool/cgroup.c | 7 ++++--- tools/bpf/bpftool/main.c | 7 ++++++- tools/bpf/bpftool/main.h | 3 ++- 6 files changed, 25 insertions(+), 8 deletions(-) diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst index 36807735e2a5..5e515aac36b3 100644 --- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst @@ -12,7 +12,8 @@ SYNOPSIS **bpftool** [*OPTIONS*] **cgroup** *COMMAND* - *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } } + *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } + | { **-e** | **--effective** } } *COMMANDS* := { **show** | **list** | **tree** | **attach** | **detach** | **help** } @@ -117,6 +118,11 @@ OPTIONS Print all logs available from libbpf, including debug-level information. + -e, --effective + Retrieve effective programs that will execute for events + within a cgroup. This includes inherited along with attached + ones. + EXAMPLES ======== | diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst index 6a9c52ef84a9..d2f76b55988d 100644 --- a/tools/bpf/bpftool/Documentation/bpftool.rst +++ b/tools/bpf/bpftool/Documentation/bpftool.rst @@ -19,7 +19,7 @@ SYNOPSIS *OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** | **feature** } *OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** } - | { **-j** | **--json** } [{ **-p** | **--pretty** }] } + | { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** | **--effective** } } *MAP-COMMANDS* := { **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext** @@ -71,6 +71,10 @@ OPTIONS includes logs from libbpf as well as from the verifier, when attempting to load programs. + -e, --effective + Retrieve effective programs that will execute for events + within a cgroup. This includes inherited along with attached ones. + SEE ALSO ======== **bpf**\ (2), diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool index 2725e27dfa42..72fd832072a3 100644 --- a/tools/bpf/bpftool/bash-completion/bpftool +++ b/tools/bpf/bpftool/bash-completion/bpftool @@ -187,7 +187,7 @@ _bpftool() # Deal with options if [[ ${words[cword]} == -* ]]; then - local c='--version --json --pretty --bpffs --mapcompat --debug' + local c='--version --json --pretty --bpffs --mapcompat --debug --effective' COMPREPLY=( $( compgen -W "$c" -- "$cur" ) ) return 0 fi diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c index 7e22f115c8c1..86f9ac8c4599 100644 --- a/tools/bpf/bpftool/cgroup.c +++ b/tools/bpf/bpftool/cgroup.c @@ -101,7 +101,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type) __u32 prog_cnt = 0; int ret; - ret = bpf_prog_query(cgroup_fd, type, 0, NULL, NULL, &prog_cnt); + ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL, + &prog_cnt); if (ret) return -1; @@ -119,8 +120,8 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type, int ret; prog_cnt = ARRAY_SIZE(prog_ids); - ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids, - &prog_cnt); + ret = bpf_prog_query(cgroup_fd, type, query_flags, &attach_flags, + prog_ids, &prog_cnt); if (ret) return ret; diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c index 4879f6395c7e..42e9ddfbbbe0 100644 --- a/tools/bpf/bpftool/main.c +++ b/tools/bpf/bpftool/main.c @@ -27,6 +27,7 @@ bool json_output; bool show_pinned; bool block_mount; bool verifier_logs; +unsigned int query_flags; int bpf_flags; struct pinned_obj_table prog_table; struct pinned_obj_table map_table; @@ -327,6 +328,7 @@ int main(int argc, char **argv) { "mapcompat", no_argument, NULL, 'm' }, { "nomount", no_argument, NULL, 'n' }, { "debug", no_argument, NULL, 'd' }, + { "effective", no_argument, NULL, 'e' }, { 0 } }; int opt, ret; @@ -342,7 +344,7 @@ int main(int argc, char **argv) hash_init(map_table.table); opterr = 0; - while ((opt = getopt_long(argc, argv, "Vhpjfmnd", + while ((opt = getopt_long(argc, argv, "Vhpjfmnde", options, NULL)) >= 0) { switch (opt) { case 'V': @@ -376,6 +378,9 @@ int main(int argc, char **argv) libbpf_set_print(print_all_levels); verifier_logs = true; break; + case 'e': + query_flags = BPF_F_QUERY_EFFECTIVE; + break; default: p_err("unrecognized option '%s'", argv[optind - 1]); if (json_output) diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index 28a2a5857e14..fddec15c454a 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -45,7 +45,7 @@ "PROG := { id PROG_ID | pinned FILE | tag PROG_TAG }" #define HELP_SPEC_OPTIONS \ "OPTIONS := { {-j|--json} [{-p|--pretty}] | {-f|--bpffs} |\n" \ - "\t {-m|--mapcompat} | {-n|--nomount} }" + "\t {-m|--mapcompat} | {-n|--nomount} | {-e|--effective} }" #define HELP_SPEC_MAP \ "MAP := { id MAP_ID | pinned FILE }" @@ -92,6 +92,7 @@ extern bool json_output; extern bool show_pinned; extern bool block_mount; extern bool verifier_logs; +extern unsigned int query_flags; extern int bpf_flags; extern struct pinned_obj_table prog_table; extern struct pinned_obj_table map_table; -- 2.17.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-21 22:33 [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] Takshak Chahande @ 2019-06-24 14:22 ` Daniel Borkmann 2019-06-24 21:51 ` Jakub Kicinski 0 siblings, 1 reply; 15+ messages in thread From: Daniel Borkmann @ 2019-06-24 14:22 UTC (permalink / raw) To: Takshak Chahande, netdev; +Cc: ast, rdna, kernel-team On 06/22/2019 12:33 AM, Takshak Chahande wrote: > With different bpf attach_flags available to attach bpf programs specially > with BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI, the list of effective > bpf-programs available to any sub-cgroups really needs to be available for > easy debugging. > > Using BPF_F_QUERY_EFFECTIVE flag, one can get the list of not only attached > bpf-programs to a cgroup but also the inherited ones from parent cgroup. > > So "-e" option is introduced to use BPF_F_QUERY_EFFECTIVE query flag here to > list all the effective bpf-programs available for execution at a specified > cgroup. > > Reused modified test program test_cgroup_attach from tools/testing/selftests/bpf: > # ./test_cgroup_attach > > With old bpftool (without -e option): > > # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ > ID AttachType AttachFlags Name > 271 egress multi pkt_cntr_1 > 272 egress multi pkt_cntr_2 > > Attached new program pkt_cntr_4 in cg2 gives following: > > # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2 > ID AttachType AttachFlags Name > 273 egress override pkt_cntr_4 > > And with new "-e" option it shows all effective programs for cg2: > > # bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2 > ID AttachType AttachFlags Name > 273 egress override pkt_cntr_4 > 271 egress override pkt_cntr_1 > 272 egress override pkt_cntr_2 > > Signed-off-by: Takshak Chahande <ctakshak@fb.com> > Acked-by: Andrey Ignatov <rdna@fb.com> Applied, thanks! ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-24 14:22 ` Daniel Borkmann @ 2019-06-24 21:51 ` Jakub Kicinski 2019-06-24 22:16 ` Andrey Ignatov 0 siblings, 1 reply; 15+ messages in thread From: Jakub Kicinski @ 2019-06-24 21:51 UTC (permalink / raw) To: Daniel Borkmann Cc: Takshak Chahande, netdev, ast, rdna, kernel-team, Stanislav Fomichev On Mon, 24 Jun 2019 16:22:25 +0200, Daniel Borkmann wrote: > On 06/22/2019 12:33 AM, Takshak Chahande wrote: > > With different bpf attach_flags available to attach bpf programs specially > > with BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI, the list of effective > > bpf-programs available to any sub-cgroups really needs to be available for > > easy debugging. > > > > Using BPF_F_QUERY_EFFECTIVE flag, one can get the list of not only attached > > bpf-programs to a cgroup but also the inherited ones from parent cgroup. > > > > So "-e" option is introduced to use BPF_F_QUERY_EFFECTIVE query flag here to > > list all the effective bpf-programs available for execution at a specified > > cgroup. > > > > Reused modified test program test_cgroup_attach from tools/testing/selftests/bpf: > > # ./test_cgroup_attach > > > > With old bpftool (without -e option): > > > > # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ > > ID AttachType AttachFlags Name > > 271 egress multi pkt_cntr_1 > > 272 egress multi pkt_cntr_2 > > > > Attached new program pkt_cntr_4 in cg2 gives following: > > > > # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2 > > ID AttachType AttachFlags Name > > 273 egress override pkt_cntr_4 > > > > And with new "-e" option it shows all effective programs for cg2: > > > > # bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2 > > ID AttachType AttachFlags Name > > 273 egress override pkt_cntr_4 > > 271 egress override pkt_cntr_1 > > 272 egress override pkt_cntr_2 > > > > Signed-off-by: Takshak Chahande <ctakshak@fb.com> > > Acked-by: Andrey Ignatov <rdna@fb.com> > > Applied, thanks! This is a cgroup-specific flag, right? It should be a parameter to cgroup show, not a global flag. Can we please drop this patch from the tree? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-24 21:51 ` Jakub Kicinski @ 2019-06-24 22:16 ` Andrey Ignatov 2019-06-24 22:43 ` Jakub Kicinski 0 siblings, 1 reply; 15+ messages in thread From: Andrey Ignatov @ 2019-06-24 22:16 UTC (permalink / raw) To: Jakub Kicinski Cc: Daniel Borkmann, Takshak Chahande, netdev, ast, Kernel Team, Stanislav Fomichev Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2019-06-24 14:51 -0700]: > On Mon, 24 Jun 2019 16:22:25 +0200, Daniel Borkmann wrote: > > On 06/22/2019 12:33 AM, Takshak Chahande wrote: > > > With different bpf attach_flags available to attach bpf programs specially > > > with BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI, the list of effective > > > bpf-programs available to any sub-cgroups really needs to be available for > > > easy debugging. > > > > > > Using BPF_F_QUERY_EFFECTIVE flag, one can get the list of not only attached > > > bpf-programs to a cgroup but also the inherited ones from parent cgroup. > > > > > > So "-e" option is introduced to use BPF_F_QUERY_EFFECTIVE query flag here to > > > list all the effective bpf-programs available for execution at a specified > > > cgroup. > > > > > > Reused modified test program test_cgroup_attach from tools/testing/selftests/bpf: > > > # ./test_cgroup_attach > > > > > > With old bpftool (without -e option): > > > > > > # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ > > > ID AttachType AttachFlags Name > > > 271 egress multi pkt_cntr_1 > > > 272 egress multi pkt_cntr_2 > > > > > > Attached new program pkt_cntr_4 in cg2 gives following: > > > > > > # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2 > > > ID AttachType AttachFlags Name > > > 273 egress override pkt_cntr_4 > > > > > > And with new "-e" option it shows all effective programs for cg2: > > > > > > # bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2 > > > ID AttachType AttachFlags Name > > > 273 egress override pkt_cntr_4 > > > 271 egress override pkt_cntr_1 > > > 272 egress override pkt_cntr_2 > > > > > > Signed-off-by: Takshak Chahande <ctakshak@fb.com> > > > Acked-by: Andrey Ignatov <rdna@fb.com> > > > > Applied, thanks! > > This is a cgroup-specific flag, right? It should be a parameter > to cgroup show, not a global flag. Can we please drop this patch > from the tree? Hey Jakub, I had same thought about cgroup-specific flag while reviewing the patch, but then found out that all flags in bpftool are now global, no mater if they're sub-command-specific or not. For example, --mapcompat is used only in prog-subcommand, but the option is global; --bpffs is used in prog- and map-subcommands, but the option is global as well, etc (there are more examples). I agree that limiting the scope of an option is a good idea in the long term and it'd be great to rework all existing options to be available only for corresponding sub-commands, but I don't see how the new `-e` options is different from existing options and why it should be dropped. Or you were counfused by the example in the commit log? Since all options are global they can be specific anywhere on the command line, i.e. instead of: # bpftool -e cgroup show /path/to/cgroup it can be: # bpftool cgroup show -e /path/to/cgroup -- Andrey Ignatov ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-24 22:16 ` Andrey Ignatov @ 2019-06-24 22:43 ` Jakub Kicinski 2019-06-24 23:38 ` Alexei Starovoitov 0 siblings, 1 reply; 15+ messages in thread From: Jakub Kicinski @ 2019-06-24 22:43 UTC (permalink / raw) To: Andrey Ignatov Cc: Daniel Borkmann, Takshak Chahande, netdev, ast, Kernel Team, Stanislav Fomichev On Mon, 24 Jun 2019 22:16:02 +0000, Andrey Ignatov wrote: > Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2019-06-24 14:51 -0700]: > > This is a cgroup-specific flag, right? It should be a parameter > > to cgroup show, not a global flag. Can we please drop this patch > > from the tree? > > Hey Jakub, > > I had same thought about cgroup-specific flag while reviewing the patch, > but then found out that all flags in bpftool are now global, no mater if > they're sub-command-specific or not. > > For example, --mapcompat is used only in prog-subcommand, but the option > is global; --bpffs is used in prog- and map-subcommands, but the option > is global as well, etc (there are more examples). I don't think these are equivalent. BPF_F_QUERY_EFFECTIVE is a flag for a syscall corresponding to a subcommand quite clearly. > I agree that limiting the scope of an option is a good idea in the long > term and it'd be great to rework all existing options to be available > only for corresponding sub-commands, but I don't see how the new `-e` > options is different from existing options and why it should be dropped. Agreed, TBH, but we can't change existing options, people may be using them. Let's drop the patch and make sure we're not making this mistake again :) Thanks! ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-24 22:43 ` Jakub Kicinski @ 2019-06-24 23:38 ` Alexei Starovoitov 2019-06-25 0:16 ` Jakub Kicinski 0 siblings, 1 reply; 15+ messages in thread From: Alexei Starovoitov @ 2019-06-24 23:38 UTC (permalink / raw) To: Jakub Kicinski, Andrey Ignatov Cc: Daniel Borkmann, Takshak Chahande, netdev, ast, Kernel Team, Stanislav Fomichev On 6/24/19 3:43 PM, Jakub Kicinski wrote: > On Mon, 24 Jun 2019 22:16:02 +0000, Andrey Ignatov wrote: >> Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2019-06-24 14:51 -0700]: >>> This is a cgroup-specific flag, right? It should be a parameter >>> to cgroup show, not a global flag. Can we please drop this patch >>> from the tree? >> >> Hey Jakub, >> >> I had same thought about cgroup-specific flag while reviewing the patch, >> but then found out that all flags in bpftool are now global, no mater if >> they're sub-command-specific or not. >> >> For example, --mapcompat is used only in prog-subcommand, but the option >> is global; --bpffs is used in prog- and map-subcommands, but the option >> is global as well, etc (there are more examples). > > I don't think these are equivalent. BPF_F_QUERY_EFFECTIVE is a flag > for a syscall corresponding to a subcommand quite clearly. > >> I agree that limiting the scope of an option is a good idea in the long >> term and it'd be great to rework all existing options to be available >> only for corresponding sub-commands, but I don't see how the new `-e` >> options is different from existing options and why it should be dropped. > > Agreed, TBH, but we can't change existing options, people may be using > them. Let's drop the patch and make sure we're not making this mistake > again :) I don't think this patch should be penalized. I'd rather see we fix them all. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-24 23:38 ` Alexei Starovoitov @ 2019-06-25 0:16 ` Jakub Kicinski 2019-06-25 0:21 ` Alexei Starovoitov 0 siblings, 1 reply; 15+ messages in thread From: Jakub Kicinski @ 2019-06-25 0:16 UTC (permalink / raw) To: Alexei Starovoitov Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande, netdev, ast, Kernel Team, Stanislav Fomichev On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote: > I don't think this patch should be penalized. > I'd rather see we fix them all. So we are going to add this broken option just to remove it? I don't understand. I'm happy to spend the 15 minutes rewriting this if you don't want to penalize Takshak. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-25 0:16 ` Jakub Kicinski @ 2019-06-25 0:21 ` Alexei Starovoitov 2019-06-25 0:30 ` Jakub Kicinski 0 siblings, 1 reply; 15+ messages in thread From: Alexei Starovoitov @ 2019-06-25 0:21 UTC (permalink / raw) To: Jakub Kicinski Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande, netdev, ast, Kernel Team, Stanislav Fomichev On 6/24/19 5:16 PM, Jakub Kicinski wrote: > On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote: >> I don't think this patch should be penalized. >> I'd rather see we fix them all. > > So we are going to add this broken option just to remove it? > I don't understand. > I'm happy to spend the 15 minutes rewriting this if you don't > want to penalize Takshak. hmm. I don't understand the 'broken' part. The only issue I see that it could have been local vs global, but they all should have been local. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-25 0:21 ` Alexei Starovoitov @ 2019-06-25 0:30 ` Jakub Kicinski 2019-06-25 0:40 ` Alexei Starovoitov 0 siblings, 1 reply; 15+ messages in thread From: Jakub Kicinski @ 2019-06-25 0:30 UTC (permalink / raw) To: Alexei Starovoitov Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande, netdev, ast, Kernel Team, Stanislav Fomichev On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote: > On 6/24/19 5:16 PM, Jakub Kicinski wrote: > > On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote: > >> I don't think this patch should be penalized. > >> I'd rather see we fix them all. > > > > So we are going to add this broken option just to remove it? > > I don't understand. > > I'm happy to spend the 15 minutes rewriting this if you don't > > want to penalize Takshak. > > hmm. I don't understand the 'broken' part. > The only issue I see that it could have been local vs global, > but they all should have been local. I don't think all of them. Only --mapcompat and --bpffs. bpffs could be argued. On mapcompat I must have not read the patch fully, I was under the impression its a global libbpf flag :( --json, --pretty, --nomount, --debug are global because they affect global behaviour of bpftool. The difference here is that we basically add a syscall parameter as a global option. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-25 0:30 ` Jakub Kicinski @ 2019-06-25 0:40 ` Alexei Starovoitov 2019-06-25 0:47 ` Jakub Kicinski 0 siblings, 1 reply; 15+ messages in thread From: Alexei Starovoitov @ 2019-06-25 0:40 UTC (permalink / raw) To: Jakub Kicinski Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande, netdev, ast, Kernel Team, Stanislav Fomichev On 6/24/19 5:30 PM, Jakub Kicinski wrote: > On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote: >> On 6/24/19 5:16 PM, Jakub Kicinski wrote: >>> On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote: >>>> I don't think this patch should be penalized. >>>> I'd rather see we fix them all. >>> >>> So we are going to add this broken option just to remove it? >>> I don't understand. >>> I'm happy to spend the 15 minutes rewriting this if you don't >>> want to penalize Takshak. >> >> hmm. I don't understand the 'broken' part. >> The only issue I see that it could have been local vs global, >> but they all should have been local. > > I don't think all of them. Only --mapcompat and --bpffs. bpffs could > be argued. On mapcompat I must have not read the patch fully, I was > under the impression its a global libbpf flag :( > > --json, --pretty, --nomount, --debug are global because they affect > global behaviour of bpftool. The difference here is that we basically > add a syscall parameter as a global option. sure. I only disagreed about not touching older flags. --effective should be local. If follow up patch means 90% rewrite then revert is better. If it's 10% fixup then it's different story. Takshak, could you check which way is cleaner? Revert and new patch or follow up fix? But bpftool doesn't have a way to do local, no? so it's kinda new feature and other flags should become local too. hence it feels more like follow up. Just my .02 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-25 0:40 ` Alexei Starovoitov @ 2019-06-25 0:47 ` Jakub Kicinski 2019-06-25 0:57 ` Jakub Kicinski 2019-06-25 1:22 ` Alexei Starovoitov 0 siblings, 2 replies; 15+ messages in thread From: Jakub Kicinski @ 2019-06-25 0:47 UTC (permalink / raw) To: Alexei Starovoitov Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande, netdev, ast, Kernel Team, Stanislav Fomichev On Tue, 25 Jun 2019 00:40:09 +0000, Alexei Starovoitov wrote: > On 6/24/19 5:30 PM, Jakub Kicinski wrote: > > On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote: > >> On 6/24/19 5:16 PM, Jakub Kicinski wrote: > >>> On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote: > >>>> I don't think this patch should be penalized. > >>>> I'd rather see we fix them all. > >>> > >>> So we are going to add this broken option just to remove it? > >>> I don't understand. > >>> I'm happy to spend the 15 minutes rewriting this if you don't > >>> want to penalize Takshak. > >> > >> hmm. I don't understand the 'broken' part. > >> The only issue I see that it could have been local vs global, > >> but they all should have been local. > > > > I don't think all of them. Only --mapcompat and --bpffs. bpffs could > > be argued. On mapcompat I must have not read the patch fully, I was > > under the impression its a global libbpf flag :( > > > > --json, --pretty, --nomount, --debug are global because they affect > > global behaviour of bpftool. The difference here is that we basically > > add a syscall parameter as a global option. > > sure. I only disagreed about not touching older flags. > --effective should be local. > If follow up patch means 90% rewrite then revert is better. > If it's 10% fixup then it's different story. I see. The local flag would not an option in getopt_long() sense, what I was thinking was about adding an "effective" keyword: # bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ becomes: # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ effective That's how we handle flags for update calls for instance.. So I think a revert :( > Takshak, > could you check which way is cleaner? Revert and new patch or follow up fix? > But bpftool doesn't have a way to do local, no? > so it's kinda new feature and other flags should become local too. > hence it feels more like follow up. Just my .02 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-25 0:47 ` Jakub Kicinski @ 2019-06-25 0:57 ` Jakub Kicinski 2019-06-25 1:23 ` Alexei Starovoitov 2019-06-25 1:22 ` Alexei Starovoitov 1 sibling, 1 reply; 15+ messages in thread From: Jakub Kicinski @ 2019-06-25 0:57 UTC (permalink / raw) To: Alexei Starovoitov Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande, netdev, ast, Kernel Team, Stanislav Fomichev On Mon, 24 Jun 2019 17:47:26 -0700, Jakub Kicinski wrote: > I see. The local flag would not an option in getopt_long() sense, what > I was thinking was about adding an "effective" keyword: Something like this, untested: --->8------------ The BPF_F_QUERY_EFFECTIVE is a syscall flag, and fits nicely as a subcommand option. We want to move away from global options, anyway. We need a global variable because of nftw limitations. Clean this flag on every invocations in case we run in batch mode. NOTE the argv[1] use on the error path in do_show() looks like a bug on it's own. --- .../bpftool/Documentation/bpftool-cgroup.rst | 24 +++---- tools/bpf/bpftool/Documentation/bpftool.rst | 6 +- tools/bpf/bpftool/bash-completion/bpftool | 17 ++--- tools/bpf/bpftool/cgroup.c | 62 ++++++++++++------- tools/bpf/bpftool/main.c | 7 +-- tools/bpf/bpftool/main.h | 3 +- 6 files changed, 66 insertions(+), 53 deletions(-) diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst index 324df15bf4cc..4fde3dfad395 100644 --- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst @@ -12,8 +12,7 @@ SYNOPSIS **bpftool** [*OPTIONS*] **cgroup** *COMMAND* - *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } - | { **-e** | **--effective** } } + *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } } *COMMANDS* := { **show** | **list** | **tree** | **attach** | **detach** | **help** } @@ -21,8 +20,8 @@ SYNOPSIS CGROUP COMMANDS =============== -| **bpftool** **cgroup { show | list }** *CGROUP* -| **bpftool** **cgroup tree** [*CGROUP_ROOT*] +| **bpftool** **cgroup { show | list }** *CGROUP* [**effective**] +| **bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**] | **bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*] | **bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG* | **bpftool** **cgroup help** @@ -35,13 +34,17 @@ CGROUP COMMANDS DESCRIPTION =========== - **bpftool cgroup { show | list }** *CGROUP* + **bpftool cgroup { show | list }** *CGROUP* [**effective**] List all programs attached to the cgroup *CGROUP*. Output will start with program ID followed by attach type, attach flags and program name. - **bpftool cgroup tree** [*CGROUP_ROOT*] + If **effective** is specified retrieve effective programs that + will execute for events within a cgroup. This includes + inherited along with attached ones. + + **bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**] Iterate over all cgroups in *CGROUP_ROOT* and list all attached programs. If *CGROUP_ROOT* is not specified, bpftool uses cgroup v2 mountpoint. @@ -50,6 +53,10 @@ DESCRIPTION commands: it starts with absolute cgroup path, followed by program ID, attach type, attach flags and program name. + If **effective** is specified retrieve effective programs that + will execute for events within a cgroup. This includes + inherited along with attached ones. + **bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*] Attach program *PROG* to the cgroup *CGROUP* with attach type *ATTACH_TYPE* and optional *ATTACH_FLAGS*. @@ -122,11 +129,6 @@ OPTIONS Print all logs available from libbpf, including debug-level information. - -e, --effective - Retrieve effective programs that will execute for events - within a cgroup. This includes inherited along with attached - ones. - EXAMPLES ======== | diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst index d2f76b55988d..6a9c52ef84a9 100644 --- a/tools/bpf/bpftool/Documentation/bpftool.rst +++ b/tools/bpf/bpftool/Documentation/bpftool.rst @@ -19,7 +19,7 @@ SYNOPSIS *OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** | **feature** } *OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** } - | { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** | **--effective** } } + | { **-j** | **--json** } [{ **-p** | **--pretty** }] } *MAP-COMMANDS* := { **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext** @@ -71,10 +71,6 @@ OPTIONS includes logs from libbpf as well as from the verifier, when attempting to load programs. - -e, --effective - Retrieve effective programs that will execute for events - within a cgroup. This includes inherited along with attached ones. - SEE ALSO ======== **bpf**\ (2), diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool index c98cb99867f6..de84ae06ae4e 100644 --- a/tools/bpf/bpftool/bash-completion/bpftool +++ b/tools/bpf/bpftool/bash-completion/bpftool @@ -187,7 +187,7 @@ _bpftool() # Deal with options if [[ ${words[cword]} == -* ]]; then - local c='--version --json --pretty --bpffs --mapcompat --debug --effective' + local c='--version --json --pretty --bpffs --mapcompat --debug' COMPREPLY=( $( compgen -W "$c" -- "$cur" ) ) return 0 fi @@ -678,12 +678,15 @@ _bpftool() ;; cgroup) case $command in - show|list) - _filedir - return 0 - ;; - tree) - _filedir + show|list|tree) + case $cword in + 3) + _filedir + ;; + 4) + COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) ) + ;; + esac return 0 ;; attach|detach) diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c index 1bb2a751107a..88b80616d47b 100644 --- a/tools/bpf/bpftool/cgroup.c +++ b/tools/bpf/bpftool/cgroup.c @@ -28,6 +28,8 @@ " connect6 | sendmsg4 | sendmsg6 |\n" \ " recvmsg4 | recvmsg6 | sysctl }" +static unsigned int query_flags; + static const char * const attach_type_strings[] = { [BPF_CGROUP_INET_INGRESS] = "ingress", [BPF_CGROUP_INET_EGRESS] = "egress", @@ -104,8 +106,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type) __u32 prog_cnt = 0; int ret; - ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL, - &prog_cnt); + ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, + NULL, &prog_cnt); if (ret) return -1; @@ -156,20 +158,30 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type, static int do_show(int argc, char **argv) { enum bpf_attach_type type; + const char *path; int cgroup_fd; int ret = -1; - if (argc < 1) { - p_err("too few parameters for cgroup show"); - goto exit; - } else if (argc > 1) { - p_err("too many parameters for cgroup show"); - goto exit; + query_flags = 0; + + if (!REQ_ARGS(1)) + return -1; + path = GET_ARG(); + + while (argc) { + if (is_prefix(*argv, "effective")) { + query_flags |= BPF_F_QUERY_EFFECTIVE; + NEXT_ARG(); + } else { + p_err("expected no more arguments, 'effective', got: '%s'?", + *argv); + return -1; + } } - cgroup_fd = open(argv[0], O_RDONLY); + cgroup_fd = open(path, O_RDONLY); if (cgroup_fd < 0) { - p_err("can't open cgroup %s", argv[1]); + p_err("can't open cgroup %s", path); goto exit; } @@ -295,23 +307,29 @@ static int do_show_tree(int argc, char **argv) char *cgroup_root; int ret; - switch (argc) { - case 0: + query_flags = 0; + + if (!argc) { cgroup_root = find_cgroup_root(); if (!cgroup_root) { p_err("cgroup v2 isn't mounted"); return -1; } - break; - case 1: - cgroup_root = argv[0]; - break; - default: - p_err("too many parameters for cgroup tree"); - return -1; + } else { + cgroup_root = GET_ARG(); + + while (argc) { + if (is_prefix(*argv, "effective")) { + query_flags |= BPF_F_QUERY_EFFECTIVE; + NEXT_ARG(); + } else { + p_err("expected no more arguments, 'effective', got: '%s'?", + *argv); + return -1; + } + } } - if (json_output) jsonw_start_array(json_wtr); else @@ -457,8 +475,8 @@ static int do_help(int argc, char **argv) } fprintf(stderr, - "Usage: %s %s { show | list } CGROUP\n" - " %s %s tree [CGROUP_ROOT]\n" + "Usage: %s %s { show | list } CGROUP [**effective**]\n" + " %s %s tree [CGROUP_ROOT] [**effective**]\n" " %s %s attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]\n" " %s %s detach CGROUP ATTACH_TYPE PROG\n" " %s %s help\n" diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c index 42e9ddfbbbe0..4879f6395c7e 100644 --- a/tools/bpf/bpftool/main.c +++ b/tools/bpf/bpftool/main.c @@ -27,7 +27,6 @@ bool json_output; bool show_pinned; bool block_mount; bool verifier_logs; -unsigned int query_flags; int bpf_flags; struct pinned_obj_table prog_table; struct pinned_obj_table map_table; @@ -328,7 +327,6 @@ int main(int argc, char **argv) { "mapcompat", no_argument, NULL, 'm' }, { "nomount", no_argument, NULL, 'n' }, { "debug", no_argument, NULL, 'd' }, - { "effective", no_argument, NULL, 'e' }, { 0 } }; int opt, ret; @@ -344,7 +342,7 @@ int main(int argc, char **argv) hash_init(map_table.table); opterr = 0; - while ((opt = getopt_long(argc, argv, "Vhpjfmnde", + while ((opt = getopt_long(argc, argv, "Vhpjfmnd", options, NULL)) >= 0) { switch (opt) { case 'V': @@ -378,9 +376,6 @@ int main(int argc, char **argv) libbpf_set_print(print_all_levels); verifier_logs = true; break; - case 'e': - query_flags = BPF_F_QUERY_EFFECTIVE; - break; default: p_err("unrecognized option '%s'", argv[optind - 1]); if (json_output) diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index fddec15c454a..28a2a5857e14 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -45,7 +45,7 @@ "PROG := { id PROG_ID | pinned FILE | tag PROG_TAG }" #define HELP_SPEC_OPTIONS \ "OPTIONS := { {-j|--json} [{-p|--pretty}] | {-f|--bpffs} |\n" \ - "\t {-m|--mapcompat} | {-n|--nomount} | {-e|--effective} }" + "\t {-m|--mapcompat} | {-n|--nomount} }" #define HELP_SPEC_MAP \ "MAP := { id MAP_ID | pinned FILE }" @@ -92,7 +92,6 @@ extern bool json_output; extern bool show_pinned; extern bool block_mount; extern bool verifier_logs; -extern unsigned int query_flags; extern int bpf_flags; extern struct pinned_obj_table prog_table; extern struct pinned_obj_table map_table; -- 2.21.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-25 0:57 ` Jakub Kicinski @ 2019-06-25 1:23 ` Alexei Starovoitov 2019-06-25 1:39 ` Takshak Chahande 0 siblings, 1 reply; 15+ messages in thread From: Alexei Starovoitov @ 2019-06-25 1:23 UTC (permalink / raw) To: Jakub Kicinski Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande, netdev, ast, Kernel Team, Stanislav Fomichev On 6/24/19 5:57 PM, Jakub Kicinski wrote: > On Mon, 24 Jun 2019 17:47:26 -0700, Jakub Kicinski wrote: >> I see. The local flag would not an option in getopt_long() sense, what >> I was thinking was about adding an "effective" keyword: > > Something like this, untested: > > --->8------------ > > The BPF_F_QUERY_EFFECTIVE is a syscall flag, and fits nicely > as a subcommand option. We want to move away from global > options, anyway. > > We need a global variable because of nftw limitations. > Clean this flag on every invocations in case we run in > batch mode. > > NOTE the argv[1] use on the error path in do_show() looks > like a bug on it's own. > --- > .../bpftool/Documentation/bpftool-cgroup.rst | 24 +++---- > tools/bpf/bpftool/Documentation/bpftool.rst | 6 +- > tools/bpf/bpftool/bash-completion/bpftool | 17 ++--- > tools/bpf/bpftool/cgroup.c | 62 ++++++++++++------- > tools/bpf/bpftool/main.c | 7 +-- > tools/bpf/bpftool/main.h | 3 +- > 6 files changed, 66 insertions(+), 53 deletions(-) > > diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst > index 324df15bf4cc..4fde3dfad395 100644 > --- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst > +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst > @@ -12,8 +12,7 @@ SYNOPSIS > > **bpftool** [*OPTIONS*] **cgroup** *COMMAND* > > - *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } > - | { **-e** | **--effective** } } > + *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } } > > *COMMANDS* := > { **show** | **list** | **tree** | **attach** | **detach** | **help** } > @@ -21,8 +20,8 @@ SYNOPSIS > CGROUP COMMANDS > =============== > > -| **bpftool** **cgroup { show | list }** *CGROUP* > -| **bpftool** **cgroup tree** [*CGROUP_ROOT*] > +| **bpftool** **cgroup { show | list }** *CGROUP* [**effective**] > +| **bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**] > | **bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*] > | **bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG* > | **bpftool** **cgroup help** > @@ -35,13 +34,17 @@ CGROUP COMMANDS > > DESCRIPTION > =========== > - **bpftool cgroup { show | list }** *CGROUP* > + **bpftool cgroup { show | list }** *CGROUP* [**effective**] > List all programs attached to the cgroup *CGROUP*. > > Output will start with program ID followed by attach type, > attach flags and program name. > > - **bpftool cgroup tree** [*CGROUP_ROOT*] > + If **effective** is specified retrieve effective programs that > + will execute for events within a cgroup. This includes > + inherited along with attached ones. > + > + **bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**] > Iterate over all cgroups in *CGROUP_ROOT* and list all > attached programs. If *CGROUP_ROOT* is not specified, > bpftool uses cgroup v2 mountpoint. > @@ -50,6 +53,10 @@ DESCRIPTION > commands: it starts with absolute cgroup path, followed by > program ID, attach type, attach flags and program name. > > + If **effective** is specified retrieve effective programs that > + will execute for events within a cgroup. This includes > + inherited along with attached ones. > + > **bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*] > Attach program *PROG* to the cgroup *CGROUP* with attach type > *ATTACH_TYPE* and optional *ATTACH_FLAGS*. > @@ -122,11 +129,6 @@ OPTIONS > Print all logs available from libbpf, including debug-level > information. > > - -e, --effective > - Retrieve effective programs that will execute for events > - within a cgroup. This includes inherited along with attached > - ones. > - > EXAMPLES > ======== > | > diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst > index d2f76b55988d..6a9c52ef84a9 100644 > --- a/tools/bpf/bpftool/Documentation/bpftool.rst > +++ b/tools/bpf/bpftool/Documentation/bpftool.rst > @@ -19,7 +19,7 @@ SYNOPSIS > *OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** | **feature** } > > *OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** } > - | { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** | **--effective** } } > + | { **-j** | **--json** } [{ **-p** | **--pretty** }] } > > *MAP-COMMANDS* := > { **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext** > @@ -71,10 +71,6 @@ OPTIONS > includes logs from libbpf as well as from the verifier, when > attempting to load programs. > > - -e, --effective > - Retrieve effective programs that will execute for events > - within a cgroup. This includes inherited along with attached ones. > - > SEE ALSO > ======== > **bpf**\ (2), > diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool > index c98cb99867f6..de84ae06ae4e 100644 > --- a/tools/bpf/bpftool/bash-completion/bpftool > +++ b/tools/bpf/bpftool/bash-completion/bpftool > @@ -187,7 +187,7 @@ _bpftool() > > # Deal with options > if [[ ${words[cword]} == -* ]]; then > - local c='--version --json --pretty --bpffs --mapcompat --debug --effective' > + local c='--version --json --pretty --bpffs --mapcompat --debug' > COMPREPLY=( $( compgen -W "$c" -- "$cur" ) ) > return 0 > fi > @@ -678,12 +678,15 @@ _bpftool() > ;; > cgroup) > case $command in > - show|list) > - _filedir > - return 0 > - ;; > - tree) > - _filedir > + show|list|tree) > + case $cword in > + 3) > + _filedir > + ;; > + 4) > + COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) ) > + ;; > + esac > return 0 > ;; > attach|detach) > diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c > index 1bb2a751107a..88b80616d47b 100644 > --- a/tools/bpf/bpftool/cgroup.c > +++ b/tools/bpf/bpftool/cgroup.c > @@ -28,6 +28,8 @@ > " connect6 | sendmsg4 | sendmsg6 |\n" \ > " recvmsg4 | recvmsg6 | sysctl }" > > +static unsigned int query_flags; > + > static const char * const attach_type_strings[] = { > [BPF_CGROUP_INET_INGRESS] = "ingress", > [BPF_CGROUP_INET_EGRESS] = "egress", > @@ -104,8 +106,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type) > __u32 prog_cnt = 0; > int ret; > > - ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL, > - &prog_cnt); > + ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, > + NULL, &prog_cnt); > if (ret) > return -1; > > @@ -156,20 +158,30 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type, > static int do_show(int argc, char **argv) > { > enum bpf_attach_type type; > + const char *path; > int cgroup_fd; > int ret = -1; > > - if (argc < 1) { > - p_err("too few parameters for cgroup show"); > - goto exit; > - } else if (argc > 1) { > - p_err("too many parameters for cgroup show"); > - goto exit; > + query_flags = 0; > + > + if (!REQ_ARGS(1)) > + return -1; > + path = GET_ARG(); > + > + while (argc) { > + if (is_prefix(*argv, "effective")) { > + query_flags |= BPF_F_QUERY_EFFECTIVE; > + NEXT_ARG(); > + } else { > + p_err("expected no more arguments, 'effective', got: '%s'?", > + *argv); > + return -1; > + } > } > > - cgroup_fd = open(argv[0], O_RDONLY); > + cgroup_fd = open(path, O_RDONLY); > if (cgroup_fd < 0) { > - p_err("can't open cgroup %s", argv[1]); > + p_err("can't open cgroup %s", path); > goto exit; > } > > @@ -295,23 +307,29 @@ static int do_show_tree(int argc, char **argv) > char *cgroup_root; > int ret; > > - switch (argc) { > - case 0: > + query_flags = 0; > + > + if (!argc) { > cgroup_root = find_cgroup_root(); > if (!cgroup_root) { > p_err("cgroup v2 isn't mounted"); > return -1; > } > - break; > - case 1: > - cgroup_root = argv[0]; > - break; > - default: > - p_err("too many parameters for cgroup tree"); > - return -1; > + } else { > + cgroup_root = GET_ARG(); > + > + while (argc) { > + if (is_prefix(*argv, "effective")) { > + query_flags |= BPF_F_QUERY_EFFECTIVE; > + NEXT_ARG(); > + } else { > + p_err("expected no more arguments, 'effective', got: '%s'?", > + *argv); > + return -1; > + } > + } > } > > - > if (json_output) > jsonw_start_array(json_wtr); > else > @@ -457,8 +475,8 @@ static int do_help(int argc, char **argv) > } > > fprintf(stderr, > - "Usage: %s %s { show | list } CGROUP\n" > - " %s %s tree [CGROUP_ROOT]\n" > + "Usage: %s %s { show | list } CGROUP [**effective**]\n" > + " %s %s tree [CGROUP_ROOT] [**effective**]\n" lgtm. Takshak, Andrey, wdyt? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-25 1:23 ` Alexei Starovoitov @ 2019-06-25 1:39 ` Takshak Chahande 0 siblings, 0 replies; 15+ messages in thread From: Takshak Chahande @ 2019-06-25 1:39 UTC (permalink / raw) To: Alexei Starovoitov Cc: Jakub Kicinski, Andrey Ignatov, Daniel Borkmann, netdev, ast, Kernel Team, Stanislav Fomichev Alexei Starovoitov <ast@fb.com> wrote on Mon [2019-Jun-24 18:23:28 -0700]: > On 6/24/19 5:57 PM, Jakub Kicinski wrote: > > On Mon, 24 Jun 2019 17:47:26 -0700, Jakub Kicinski wrote: > >> I see. The local flag would not an option in getopt_long() sense, what > >> I was thinking was about adding an "effective" keyword: > > > > Something like this, untested: > > > > --->8------------ > > > > The BPF_F_QUERY_EFFECTIVE is a syscall flag, and fits nicely > > as a subcommand option. We want to move away from global > > options, anyway. > > > > We need a global variable because of nftw limitations. > > Clean this flag on every invocations in case we run in > > batch mode. > > > > NOTE the argv[1] use on the error path in do_show() looks > > like a bug on it's own. > > --- > > .../bpftool/Documentation/bpftool-cgroup.rst | 24 +++---- > > tools/bpf/bpftool/Documentation/bpftool.rst | 6 +- > > tools/bpf/bpftool/bash-completion/bpftool | 17 ++--- > > tools/bpf/bpftool/cgroup.c | 62 ++++++++++++------- > > tools/bpf/bpftool/main.c | 7 +-- > > tools/bpf/bpftool/main.h | 3 +- > > 6 files changed, 66 insertions(+), 53 deletions(-) > > > > diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst > > index 324df15bf4cc..4fde3dfad395 100644 > > --- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst > > +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst > > @@ -12,8 +12,7 @@ SYNOPSIS > > > > **bpftool** [*OPTIONS*] **cgroup** *COMMAND* > > > > - *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } > > - | { **-e** | **--effective** } } > > + *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } } > > > > *COMMANDS* := > > { **show** | **list** | **tree** | **attach** | **detach** | **help** } > > @@ -21,8 +20,8 @@ SYNOPSIS > > CGROUP COMMANDS > > =============== > > > > -| **bpftool** **cgroup { show | list }** *CGROUP* > > -| **bpftool** **cgroup tree** [*CGROUP_ROOT*] > > +| **bpftool** **cgroup { show | list }** *CGROUP* [**effective**] > > +| **bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**] > > | **bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*] > > | **bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG* > > | **bpftool** **cgroup help** > > @@ -35,13 +34,17 @@ CGROUP COMMANDS > > > > DESCRIPTION > > =========== > > - **bpftool cgroup { show | list }** *CGROUP* > > + **bpftool cgroup { show | list }** *CGROUP* [**effective**] > > List all programs attached to the cgroup *CGROUP*. > > > > Output will start with program ID followed by attach type, > > attach flags and program name. > > > > - **bpftool cgroup tree** [*CGROUP_ROOT*] > > + If **effective** is specified retrieve effective programs that > > + will execute for events within a cgroup. This includes > > + inherited along with attached ones. > > + > > + **bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**] > > Iterate over all cgroups in *CGROUP_ROOT* and list all > > attached programs. If *CGROUP_ROOT* is not specified, > > bpftool uses cgroup v2 mountpoint. > > @@ -50,6 +53,10 @@ DESCRIPTION > > commands: it starts with absolute cgroup path, followed by > > program ID, attach type, attach flags and program name. > > > > + If **effective** is specified retrieve effective programs that > > + will execute for events within a cgroup. This includes > > + inherited along with attached ones. > > + > > **bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*] > > Attach program *PROG* to the cgroup *CGROUP* with attach type > > *ATTACH_TYPE* and optional *ATTACH_FLAGS*. > > @@ -122,11 +129,6 @@ OPTIONS > > Print all logs available from libbpf, including debug-level > > information. > > > > - -e, --effective > > - Retrieve effective programs that will execute for events > > - within a cgroup. This includes inherited along with attached > > - ones. > > - > > EXAMPLES > > ======== > > | > > diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst > > index d2f76b55988d..6a9c52ef84a9 100644 > > --- a/tools/bpf/bpftool/Documentation/bpftool.rst > > +++ b/tools/bpf/bpftool/Documentation/bpftool.rst > > @@ -19,7 +19,7 @@ SYNOPSIS > > *OBJECT* := { **map** | **program** | **cgroup** | **perf** | **net** | **feature** } > > > > *OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** } > > - | { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-e** | **--effective** } } > > + | { **-j** | **--json** } [{ **-p** | **--pretty** }] } > > > > *MAP-COMMANDS* := > > { **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext** > > @@ -71,10 +71,6 @@ OPTIONS > > includes logs from libbpf as well as from the verifier, when > > attempting to load programs. > > > > - -e, --effective > > - Retrieve effective programs that will execute for events > > - within a cgroup. This includes inherited along with attached ones. > > - > > SEE ALSO > > ======== > > **bpf**\ (2), > > diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool > > index c98cb99867f6..de84ae06ae4e 100644 > > --- a/tools/bpf/bpftool/bash-completion/bpftool > > +++ b/tools/bpf/bpftool/bash-completion/bpftool > > @@ -187,7 +187,7 @@ _bpftool() > > > > # Deal with options > > if [[ ${words[cword]} == -* ]]; then > > - local c='--version --json --pretty --bpffs --mapcompat --debug --effective' > > + local c='--version --json --pretty --bpffs --mapcompat --debug' > > COMPREPLY=( $( compgen -W "$c" -- "$cur" ) ) > > return 0 > > fi > > @@ -678,12 +678,15 @@ _bpftool() > > ;; > > cgroup) > > case $command in > > - show|list) > > - _filedir > > - return 0 > > - ;; > > - tree) > > - _filedir > > + show|list|tree) > > + case $cword in > > + 3) > > + _filedir > > + ;; > > + 4) > > + COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) ) > > + ;; > > + esac > > return 0 > > ;; > > attach|detach) > > diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c > > index 1bb2a751107a..88b80616d47b 100644 > > --- a/tools/bpf/bpftool/cgroup.c > > +++ b/tools/bpf/bpftool/cgroup.c > > @@ -28,6 +28,8 @@ > > " connect6 | sendmsg4 | sendmsg6 |\n" \ > > " recvmsg4 | recvmsg6 | sysctl }" > > > > +static unsigned int query_flags; > > + > > static const char * const attach_type_strings[] = { > > [BPF_CGROUP_INET_INGRESS] = "ingress", > > [BPF_CGROUP_INET_EGRESS] = "egress", > > @@ -104,8 +106,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type) > > __u32 prog_cnt = 0; > > int ret; > > > > - ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL, > > - &prog_cnt); > > + ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, > > + NULL, &prog_cnt); > > if (ret) > > return -1; > > > > @@ -156,20 +158,30 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type, > > static int do_show(int argc, char **argv) > > { > > enum bpf_attach_type type; > > + const char *path; > > int cgroup_fd; > > int ret = -1; > > > > - if (argc < 1) { > > - p_err("too few parameters for cgroup show"); > > - goto exit; > > - } else if (argc > 1) { > > - p_err("too many parameters for cgroup show"); > > - goto exit; > > + query_flags = 0; > > + > > + if (!REQ_ARGS(1)) > > + return -1; > > + path = GET_ARG(); > > + > > + while (argc) { > > + if (is_prefix(*argv, "effective")) { > > + query_flags |= BPF_F_QUERY_EFFECTIVE; > > + NEXT_ARG(); > > + } else { > > + p_err("expected no more arguments, 'effective', got: '%s'?", > > + *argv); > > + return -1; > > + } > > } > > > > - cgroup_fd = open(argv[0], O_RDONLY); > > + cgroup_fd = open(path, O_RDONLY); > > if (cgroup_fd < 0) { > > - p_err("can't open cgroup %s", argv[1]); > > + p_err("can't open cgroup %s", path); > > goto exit; > > } > > > > @@ -295,23 +307,29 @@ static int do_show_tree(int argc, char **argv) > > char *cgroup_root; > > int ret; > > > > - switch (argc) { > > - case 0: > > + query_flags = 0; > > + > > + if (!argc) { > > cgroup_root = find_cgroup_root(); > > if (!cgroup_root) { > > p_err("cgroup v2 isn't mounted"); > > return -1; > > } > > - break; > > - case 1: > > - cgroup_root = argv[0]; > > - break; > > - default: > > - p_err("too many parameters for cgroup tree"); > > - return -1; > > + } else { > > + cgroup_root = GET_ARG(); > > + > > + while (argc) { > > + if (is_prefix(*argv, "effective")) { > > + query_flags |= BPF_F_QUERY_EFFECTIVE; > > + NEXT_ARG(); > > + } else { > > + p_err("expected no more arguments, 'effective', got: '%s'?", > > + *argv); > > + return -1; > > + } > > + } > > } > > > > - > > if (json_output) > > jsonw_start_array(json_wtr); > > else > > @@ -457,8 +475,8 @@ static int do_help(int argc, char **argv) > > } > > > > fprintf(stderr, > > - "Usage: %s %s { show | list } CGROUP\n" > > - " %s %s tree [CGROUP_ROOT]\n" > > + "Usage: %s %s { show | list } CGROUP [**effective**]\n" > > + " %s %s tree [CGROUP_ROOT] [**effective**]\n" > > lgtm. > Takshak, Andrey, wdyt? ya, looks fine to me. --Takshak ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] 2019-06-25 0:47 ` Jakub Kicinski 2019-06-25 0:57 ` Jakub Kicinski @ 2019-06-25 1:22 ` Alexei Starovoitov 1 sibling, 0 replies; 15+ messages in thread From: Alexei Starovoitov @ 2019-06-25 1:22 UTC (permalink / raw) To: Jakub Kicinski Cc: Andrey Ignatov, Daniel Borkmann, Takshak Chahande, netdev, ast, Kernel Team, Stanislav Fomichev On 6/24/19 5:47 PM, Jakub Kicinski wrote: > On Tue, 25 Jun 2019 00:40:09 +0000, Alexei Starovoitov wrote: >> On 6/24/19 5:30 PM, Jakub Kicinski wrote: >>> On Tue, 25 Jun 2019 00:21:57 +0000, Alexei Starovoitov wrote: >>>> On 6/24/19 5:16 PM, Jakub Kicinski wrote: >>>>> On Mon, 24 Jun 2019 23:38:11 +0000, Alexei Starovoitov wrote: >>>>>> I don't think this patch should be penalized. >>>>>> I'd rather see we fix them all. >>>>> >>>>> So we are going to add this broken option just to remove it? >>>>> I don't understand. >>>>> I'm happy to spend the 15 minutes rewriting this if you don't >>>>> want to penalize Takshak. >>>> >>>> hmm. I don't understand the 'broken' part. >>>> The only issue I see that it could have been local vs global, >>>> but they all should have been local. >>> >>> I don't think all of them. Only --mapcompat and --bpffs. bpffs could >>> be argued. On mapcompat I must have not read the patch fully, I was >>> under the impression its a global libbpf flag :( >>> >>> --json, --pretty, --nomount, --debug are global because they affect >>> global behaviour of bpftool. The difference here is that we basically >>> add a syscall parameter as a global option. >> >> sure. I only disagreed about not touching older flags. >> --effective should be local. >> If follow up patch means 90% rewrite then revert is better. >> If it's 10% fixup then it's different story. > > I see. The local flag would not an option in getopt_long() sense, what > I was thinking was about adding an "effective" keyword: > > # bpftool -e cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ > > becomes: > > # bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/ effective > > That's how we handle flags for update calls for instance.. > > So I think a revert :( fair enough. removed it and force pushed bpf-next. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-06-25 1:40 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-21 22:33 [PATCH bpf-next] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree] Takshak Chahande 2019-06-24 14:22 ` Daniel Borkmann 2019-06-24 21:51 ` Jakub Kicinski 2019-06-24 22:16 ` Andrey Ignatov 2019-06-24 22:43 ` Jakub Kicinski 2019-06-24 23:38 ` Alexei Starovoitov 2019-06-25 0:16 ` Jakub Kicinski 2019-06-25 0:21 ` Alexei Starovoitov 2019-06-25 0:30 ` Jakub Kicinski 2019-06-25 0:40 ` Alexei Starovoitov 2019-06-25 0:47 ` Jakub Kicinski 2019-06-25 0:57 ` Jakub Kicinski 2019-06-25 1:23 ` Alexei Starovoitov 2019-06-25 1:39 ` Takshak Chahande 2019-06-25 1:22 ` Alexei Starovoitov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.