lkml.org 
[lkml]   [2017]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH net-next 0/5] bpftool: cgroup bpf operations
    Hi Roman!

    On Thu, 30 Nov 2017 13:42:57 +0000, Roman Gushchin wrote:
    > This patchset adds basic cgroup bpf operations to bpftool.
    >
    > Right now there is no convenient way to perform these operations.
    > The /samples/bpf/load_sock_ops.c implements attach/detacg operations,
    > but only for BPF_CGROUP_SOCK_OPS programs. Bps (part of bcc) implements
    > bpf introspection, but lacks any cgroup-related specific.
    >
    > I find having a tool to perform these basic operations in the kernel tree
    > very useful, as it can be used in the corresponding bpf documentation
    > without creating additional dependencies. And bpftool seems to be
    > a right tool to extend with such functionality.

    Could you place your code in a new file and add a new "object level"?
    I.e.
    bpftool cgroup list
    bpftool cgroup attach ...
    bpftool cgroup help
    etc? Note that you probably want the list to be first, so if someone
    types "bpftool cg" it runs list by default.

    Does it make sense to support pinned files and specifying programs by
    id? I used the "id"/"pinned" keywords so that users can choose to use
    either. Perhaps you should at least prefix the file to with "file"?
    So:
    $ bpftool cgattach file ./mybpfprog.o /sys/fs/cgroup/user.slice/ ingress
    $ bpftool cgattach id 19 /sys/fs/cgroup/user.slice/ ingress
    $ bpftool cgattach pin /bpf/prog /sys/fs/cgroup/user.slice/ ingress
    Would this make sense?

    Smaller nits on the coding style:
    - please try to run checkpatch, perhaps you did, but some people
    forget tools are in the kernel tree :)
    - please keep includes in alphabetical order;
    - please keep variable declarations in functions ordered longest to
    shortest, if that's impossible because of dependency between
    initializers - move the initializers to the code.

    Please also don't forget to update/create new man page.

    Thanks! :)

    \
     
     \ /
      Last update: 2017-12-01 08:43    [W:2.146 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site