From mboxrd@z Thu Jan  1 00:00:00 1970
From: Roman Gushchin <guro@fb.com>
Subject: Re: [PATCH net-next 0/5] bpftool: cgroup bpf operations
Date: Fri, 1 Dec 2017 17:06:25 +0000
Message-ID: <20171201170624.GC27436@castle.DHCP.thefacebook.com>
References: <20171130134302.2840-1-guro@fb.com>
 <20171130190454.26723962@cakuba.netronome.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Cc: <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <kernel-team@fb.com>, <ast@kernel.org>, <daniel@iogearbox.net>,
        <kafai@fb.com>
To: Jakub Kicinski <jakub.kicinski@netronome.com>
Return-path: <linux-kernel-owner@vger.kernel.org>
Content-Disposition: inline
In-Reply-To: <20171130190454.26723962@cakuba.netronome.com>
Sender: linux-kernel-owner@vger.kernel.org
List-Id: netdev.vger.kernel.org

On Thu, Nov 30, 2017 at 07:04:54PM -0800, Jakub Kicinski wrote:
> 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.

Ok, I'll try to address these comments in v2.

Thank you!