BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Borkmann <daniel@iogearbox.net>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: m@lambda.lt, joe@wand.net.nz, bpf@vger.kernel.org,
	netdev@vger.kernel.org
Subject: Re: [PATCH bpf-next 3/7] bpf: add netns cookie and enable it for bpf cgroup hooks
Date: Sat, 28 Mar 2020 03:16:06 +0100
Message-ID: <99b27ef9-d8c5-c00b-2562-1b385ac205d0@iogearbox.net> (raw)
In-Reply-To: <20200328014844.xz5s67j2cyvnf7lp@ast-mbp>

On 3/28/20 2:48 AM, Alexei Starovoitov wrote:
> On Fri, Mar 27, 2020 at 04:58:52PM +0100, Daniel Borkmann wrote:
>> + *
>> + * u64 bpf_get_netns_cookie(void *ctx)
>> + * 	Description
>> + * 		Retrieve the cookie (generated by the kernel) of the network
>> + * 		namespace the input *ctx* is associated with. The network
>> + * 		namespace cookie remains stable for its lifetime and provides
>> + * 		a global identifier that can be assumed unique. If *ctx* is
>> + * 		NULL, then the helper returns the cookie for the initial
>> + * 		network namespace. The cookie itself is very similar to that
>> + * 		of bpf_get_socket_cookie() helper, but for network namespaces
>> + * 		instead of sockets.
> 
> All new helpers in this patch and few others are missing 'flags' argument.
> Yes. It's kinda hard right now to come up with a use case for the flags,
> since all helpers look kinda trivial, simple, and single purpose.
> But the same thing happened with bpf_send_signal(). It felt that there is no
> way to extend it. Yet later we had to add bpf_send_signal_thread() which could
> have been handled with flags if they were there. So please add flags to all new
> helpers though it might seem redundant.

We have very similar helpers for almost 2yrs now, that is, bpf_get_socket_cookie()
and bpf_skb_ancestor_cgroup_id(). Both no extra 'unused flags' arg and they are
simple enough to do exactly what we expect them to do, I also haven't seen any
reason to extend them further so far (otherwise we have have new ones by now). The
two added here are very much analogue to this, so breaking this consistency is
super ugly just to add empty flags now. :/ Given the timeframe we have these by now
and given they do one simple thing, what is the harm to add a new helper in future
iff really needed rather than uglifying with flags now (I would understand it for
complex helpers though where we use this practice)? Just recently we added bpf_jiffies64()
(5576b991e9c1 ("bpf: Add BPF_FUNC_jiffies64")); no flag either and it has similar
simplicity.

Thanks,
Daniel

  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-27 15:58 [PATCH bpf-next 0/7] Various improvements to cgroup helpers Daniel Borkmann
2020-03-27 15:58 ` [PATCH bpf-next 1/7] bpf: enable retrieval of socket cookie for bind/post-bind hook Daniel Borkmann
2020-03-27 15:58 ` [PATCH bpf-next 2/7] bpf: enable perf event rb output for bpf cgroup progs Daniel Borkmann
2020-03-27 15:58 ` [PATCH bpf-next 3/7] bpf: add netns cookie and enable it for bpf cgroup hooks Daniel Borkmann
2020-03-28  0:32   ` Andrii Nakryiko
2020-03-28  1:30     ` Daniel Borkmann
2020-03-28  1:48   ` Alexei Starovoitov
2020-03-28  2:16     ` Daniel Borkmann [this message]
2020-03-28  2:56       ` Alexei Starovoitov
2020-03-27 15:58 ` [PATCH bpf-next 4/7] bpf: allow to retrieve cgroup v1 classid from v2 hooks Daniel Borkmann
2020-03-28  0:41   ` Andrii Nakryiko
2020-03-28  1:56     ` Daniel Borkmann
2020-03-28 20:27       ` Andrii Nakryiko
2020-03-27 15:58 ` [PATCH bpf-next 5/7] bpf: enable bpf cgroup hooks to retrieve cgroup v2 and ancestor id Daniel Borkmann
2020-03-28  0:43   ` Andrii Nakryiko
2020-03-27 15:58 ` [PATCH bpf-next 6/7] bpf: enable retrival of pid/tgid/comm from bpf cgroup hooks Daniel Borkmann
2020-03-28  0:49   ` Andrii Nakryiko
2020-03-28  1:40     ` Daniel Borkmann
2020-03-27 15:58 ` [PATCH bpf-next 7/7] bpf: add selftest cases for ctx_or_null argument type Daniel Borkmann
2020-03-28  0:51   ` Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=99b27ef9-d8c5-c00b-2562-1b385ac205d0@iogearbox.net \
    --to=daniel@iogearbox.net \
    --cc=alexei.starovoitov@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=joe@wand.net.nz \
    --cc=m@lambda.lt \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git