Re: [PATCH bpf-next] bpf: Add drgn script to list progs/maps

[Andrey Ignatov <rdna@fb.com>]

Andrey Ignatov <rdna@fb.com> [Fri, 2020-02-28 12:11 -0800]:
> Omar Sandoval <osandov@fb.com> [Thu, 2020-02-27 14:19 -0800]:
> > On 2/27/20 1:32 PM, Daniel Borkmann wrote:
> > > On 2/27/20 10:11 PM, Daniel Borkmann wrote:
> > >> [ +tj ]
> > >>
> > >> On 2/27/20 7:26 PM, Andrey Ignatov wrote:
> > >>> Stanislav Fomichev <sdf@fomichev.me> [Thu, 2020-02-27 10:01 -0800]:
> > >>>> On 02/26, Andrey Ignatov wrote:

[...]

> > >>>
> > >>> In this specifc case I'd love to see feedback from Omar and BPF
> > >>> maintainers.
> > >>
> > >> I can certainly see both sides given that drgn tools have been added to
> > >> tools/cgroup/ already. I presume if so, then these could live in tools/drgn/
> > >> which would also make it more clear what is needed to run as dependency
> > >> plus there should be be a proper high-level readme to document what developers
> > >> need to run in order to run them. But from looking at [1], I can also see that
> > >> those scripts would depend on new helpers being added/updated/deleted, so it
> > >> might be easier to add drgn/tools/ directory where scripts could be updated
> > >> in one go with updates to drgn helpers. Either way, I think it would be nice
> > >> to add documentation somewhere for getting people started.
> > > 
> > > One example that should definitely be avoided is 9ea37e24d4a9 ("iocost: Fix
> > > iocost_monitor.py due to helper type mismatch") due to both living in separate
> > > places. A third option to think about (if this is to be adapted by more subsystems)
> > > could be to have all the kernel-specific helpers from drgn/helpers/linux under
> > > tools/drgn/helpers/ in the kernel tree and the tools living under
> > > tools/drgn/<subsys>/ e.g. tools/drgn/bpf/.
> > > 
> > >>> [1] https://github.com/osandov/drgn/tree/master/drgn/helpers/linux
> > >>> [2] https://github.com/osandov/drgn/tree/master/examples/linux
> > > 
> > I can think of a few benefits of having this tool (and others like it)
> > in the drgn repository:
> > 
> > * Easier to keep in sync with new helpers/API changes
> > * More examples in one centralized place for people building new tools
> > * Potential to identify pain points in the API and possible new helpers
> > 
> > I think this would benefit the drgn project as a whole.
> > 
> > The downsides:
> > 
> > * More maintenance for me
> > * Tools will have to support multiple kernel versions (as opposed to
> >   only supporting the kernel that they shipped with)
> > * Less visibility for kernel developers
> > 
> > That second point is true of the helpers bundled with drgn anyways, so I
> > don't think it's a big deal. The third point will improve over time as
> > we get more people on the drgn train :)
> > 
> > I may come to regret the first point, but I think the upsides are worth
> > it. Andrey, feel free to submit a PR adding this to the drgn repository
> > under a new top-level tools/ directory.
> 
> Daniel, Omar, thanks for your input. This all makes sense and it seems
> we're coming to an agreement that it'll be easier to support tools like
> this in drgn repo. That works for me and with this in mind I see the
> followig way forward:
> 
> - I'll submit PR to drgn repo to create a new tools/ directory and place
>   the script there using the bpf_inspect.py name suggested by Andrii.
>   This way the script will be in sync with all the recent changes to 
>   drgn helpers;
> 
> - As soon as PR is merged I'll add documentation to kernel tree to make
>   folks aware that such a tool exists, if there are any recommendation
>   where to add the documentation to I'd appreciate it.
> 
> - I'll try to make the script less dependent on kernel version
>   (Quentin's feedba) since it'll exist separate from the kernel
>   sources.
> 
> 
> Hope it works for everyone, please lmk if it doesn't for some reason of
> if I missed something. Thanks.

For anyone following: https://github.com/osandov/drgn/pull/49

-- 
Andrey Ignatov