From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stanislav Fomichev Subject: Re: [PATCH v4 bpf-next 5/7] bpftool: add loadall command Date: Fri, 9 Nov 2018 07:42:46 -0800 Message-ID: <20181109154246.f4vpeewji6wk6iww@mini-arch> References: <20181109002213.5914-1-s@fomichev.me> <20181109002213.5914-6-s@fomichev.me> <30359d0f-40c0-27c9-cd4e-bba60c6cb138@netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, linux-kselftest@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, shuah@kernel.org, jakub.kicinski@netronome.com, guro@fb.com, jiong.wang@netronome.com, sdf@google.com, bhole_prashant_q7@lab.ntt.co.jp, john.fastabend@gmail.com, jbenc@redhat.com, treeze.taeung@gmail.com, yhs@fb.com, osk@fb.com, sandipan@linux.vnet.ibm.com To: Quentin Monnet Return-path: Received: from mail-pg1-f194.google.com ([209.85.215.194]:42088 "EHLO mail-pg1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728351AbeKJBXy (ORCPT ); Fri, 9 Nov 2018 20:23:54 -0500 Received: by mail-pg1-f194.google.com with SMTP id i4-v6so994041pgq.9 for ; Fri, 09 Nov 2018 07:42:48 -0800 (PST) Content-Disposition: inline In-Reply-To: <30359d0f-40c0-27c9-cd4e-bba60c6cb138@netronome.com> Sender: netdev-owner@vger.kernel.org List-ID: On 11/09, Quentin Monnet wrote: > 2018-11-08 16:22 UTC-0800 ~ Stanislav Fomichev > > From: Stanislav Fomichev > > > > This patch adds new *loadall* command which slightly differs from the > > existing *load*. *load* command loads all programs from the obj file, > > but pins only the first programs. *loadall* pins all programs from the > > obj file under specified directory. > > > > The intended usecase is flow_dissector, where we want to load a bunch > > of progs, pin them all and after that construct a jump table. > > > > Signed-off-by: Stanislav Fomichev > > --- > > .../bpftool/Documentation/bpftool-prog.rst | 14 +++- > > tools/bpf/bpftool/bash-completion/bpftool | 4 +- > > tools/bpf/bpftool/common.c | 31 ++++---- > > tools/bpf/bpftool/main.h | 1 + > > tools/bpf/bpftool/prog.c | 74 ++++++++++++++----- > > 5 files changed, 82 insertions(+), 42 deletions(-) > > > > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst > > index ac4e904b10fb..d943d9b67a1d 100644 > > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst > > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst > > > @@ -24,7 +25,7 @@ MAP COMMANDS > > | **bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual**}] > > | **bpftool** **prog dump jited** *PROG* [{**file** *FILE* | **opcodes**}] > > | **bpftool** **prog pin** *PROG* *FILE* > > -| **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] > > +| **bpftool** **prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] > > | **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP* > > | **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP* > > | **bpftool** **prog help** > > @@ -79,8 +80,13 @@ DESCRIPTION > > contain a dot character ('.'), which is reserved for future > > extensions of *bpffs*. > > > > - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] > > - Load bpf program from binary *OBJ* and pin as *FILE*. > > + **bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] > > + Load bpf program(s) from binary *OBJ* and pin as *FILE*. > > + Both **bpftool prog load** and **bpftool prog loadall** load > > + all maps and programs from the *OBJ* and differ only in > > + pinning. **load** pins only the first program from the *OBJ* > > + as *FILE*. **loadall** pins all programs from the *OBJ* > > + under *FILE* directory. > > **type** is optional, if not specified program type will be > > inferred from section names. > > By default bpftool will create new maps as declared in the ELF > > Thanks a lot for all the changes! The series looks really good to me > now. The last nit I might have is that we could maybe replace "FILE" > with "PATH" (as it can now be a directory), in the doc an below. No need > to respin just for this, though. Agreed, makes sense, will do another respin to address Jakub's comments anyway. Thanks for a review! > > @@ -1035,7 +1067,8 @@ static int do_help(int argc, char **argv) > > " %s %s dump xlated PROG [{ file FILE | opcodes | visual }]\n" > > " %s %s dump jited PROG [{ file FILE | opcodes }]\n" > > " %s %s pin PROG FILE\n" > > - " %s %s load OBJ FILE [type TYPE] [dev NAME] \\\n" > > + " %s %s { load | loadall } OBJ FILE \\\n" > > + " [type TYPE] [dev NAME] \\\n" > > " [map { idx IDX | name NAME } MAP]\n" > > " %s %s attach PROG ATTACH_TYPE MAP\n" > > " %s %s detach PROG ATTACH_TYPE MAP\n"