Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 30 Nov 2017 19:04:54 -0800
From:   Jakub Kicinski <jakub.kicinski@netronome.com>
To:     Roman Gushchin <guro@fb.com>
Cc:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <kernel-team@fb.com>, <ast@kernel.org>, <daniel@iogearbox.net>,
        <kafai@fb.com>
Subject: Re: [PATCH net-next 0/5] bpftool: cgroup bpf operations
Message-ID: <20171130190454.26723962@cakuba.netronome.com>
In-Reply-To: <20171130134302.2840-1-guro@fb.com>
References: <20171130134302.2840-1-guro@fb.com>
Organization: Netronome Systems, Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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! :)

From 1585498885155684842@xxx Thu Nov 30 13:46:05 +0000 2017
X-GM-THRID: 1585498885155684842
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread