On 03/02/2015 12:51 PM, Masami Hiramatsu wrote:

(2015/03/02 20:10), Daniel Borkmann wrote:

quoted

On 03/02/2015 11:53 AM, Masami Hiramatsu wrote:
...

quoted

Hmm, it seems that this still doesn't hide some APIs which is provided
only when CONFIG_BPF_SYSCALL. For example bpf_register_map_type etc.
I think all those APIs should be hidden in #ifdef or at least be commented
so that the user doesn't refer that without the kconfig.
(I don't think we need to provide dummy functions for those APIs,
   but it's better to clarify which API we can use with which kconfig)

Well, currently all possible map types (hash table, array map) that
would actually call into bpf_register_map_type() are only built when
CONFIG_BPF_SYSCALL is enabled (see kernel/bpf/Makefile). I don't think
new map additions should be added that are not under kernel/bpf/ and/or
enabled outside the CONFIG_BPF_SYSCALL, as it should be considered
part of the eBPF core code.

The difference here (this patch) is simply that we don't want users to
build ifdef spaghetti constructs in user code, so the API that is
actually used by eBPF _users_ is being properly ifdef'ed in the headers.

So, I don't think this is a big problem, but we could move these bits
under the ifdef CONFIG_BPF_SYSCALL w/o providing a dummy in the else part.
I can do that outside of the scope of this set.

Or, maybe we'd better move them into new include/linux/bpf_prog.h which
includes basic include/linux/bpf.h. Then, user can include the bpf_prog.h
instead of bpf.h. Also, we can check CONFIG_BPF_SYSCAL=y at the top of
bpf_prog.h. This makes things clearer :)

I'm preferring the 1st variant, though. We have currently two native eBPF
users, that is, socket filters and tc's cls_bpf (queued in net-next) and
looking at the code/API usage, it's really not that hard, where it would
justify to move this to an extra header file, really.

I'm cooking a patch for net-next right now with the first variant (which is
on top of this patch that resides in net-next as-is as well).

Thanks,
Daniel