[PATCH v10 net-next 0/2] load imm64 insn and uapi/linux/bpf.h

Hi,

V9->V10
- no changes, added Daniel's ack

Note they're on top of Hannes's patch in the same area [1]

V8 thread with 'why' reasoning and end goal [2]

Original set [3] of ~28 patches I'm planning to present in 4 stages:

  I. this 2 patches to fork off llvm upstreaming
 II. bpf syscall with manpage and map implementation
III. bpf program load/unload with verifier testsuite (1st user of
     instruction macros from bpf.h and 1st user of load imm64 insn)
 IV. tracing, etc

[1] http://patchwork.ozlabs.org/patch/385266/
[2] https://lkml.org/lkml/2014/8/27/628
[3] https://lkml.org/lkml/2014/8/26/859

allow user space to generate eBPF programs

uapi/linux/bpf.h: eBPF instruction set definition

linux/filter.h: the rest

This patch only moves macro definitions, but practically it freezes existing
eBPF instruction set, though new instructions can still be added in the future.

These eBPF definitions cannot go into uapi/linux/filter.h, since the names
may conflict with existing applications.

Full eBPF ISA description is in Documentation/networking/filter.txt

Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
Acked-by: Daniel Borkmann <redacted>
---
 include/linux/filter.h    |   56 +-------------------------------------
 include/uapi/linux/Kbuild |    1 +
 include/uapi/linux/bpf.h  |   65 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 55 deletions(-)
 create mode 100644 include/uapi/linux/bpf.h

diff --git a/include/linux/filter.h b/include/linux/filter.h
index bf323da77950..8f82ef3f1cdd 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h

@@ -10,58 +10,12 @@
 #include <linux/workqueue.h>
 #include <uapi/linux/filter.h>
 #include <asm/cacheflush.h>
+#include <uapi/linux/bpf.h>
 
 struct sk_buff;
 struct sock;
 struct seccomp_data;
 
-/* Internally used and optimized filter representation with extended
- * instruction set based on top of classic BPF.
- */
-
-/* instruction classes */
-#define BPF_ALU64	0x07	/* alu mode in double word width */
-
-/* ld/ldx fields */
-#define BPF_DW		0x18	/* double word */
-#define BPF_XADD	0xc0	/* exclusive add */
-
-/* alu/jmp fields */
-#define BPF_MOV		0xb0	/* mov reg to reg */
-#define BPF_ARSH	0xc0	/* sign extending arithmetic shift right */
-
-/* change endianness of a register */
-#define BPF_END		0xd0	/* flags for endianness conversion: */
-#define BPF_TO_LE	0x00	/* convert to little-endian */
-#define BPF_TO_BE	0x08	/* convert to big-endian */
-#define BPF_FROM_LE	BPF_TO_LE
-#define BPF_FROM_BE	BPF_TO_BE
-
-#define BPF_JNE		0x50	/* jump != */
-#define BPF_JSGT	0x60	/* SGT is signed '>', GT in x86 */
-#define BPF_JSGE	0x70	/* SGE is signed '>=', GE in x86 */
-#define BPF_CALL	0x80	/* function call */
-#define BPF_EXIT	0x90	/* function return */
-
-/* Register numbers */
-enum {
-	BPF_REG_0 = 0,
-	BPF_REG_1,
-	BPF_REG_2,
-	BPF_REG_3,
-	BPF_REG_4,
-	BPF_REG_5,
-	BPF_REG_6,
-	BPF_REG_7,
-	BPF_REG_8,
-	BPF_REG_9,
-	BPF_REG_10,
-	__MAX_BPF_REG,
-};
-
-/* BPF has 10 general purpose 64-bit registers and stack frame. */
-#define MAX_BPF_REG	__MAX_BPF_REG
-
 /* ArgX, context and stack frame pointer register positions. Note,
  * Arg1, Arg2, Arg3, etc are used as argument mappings of function
  * calls in BPF_CALL instruction.

@@ -322,14 +276,6 @@ enum {
 #define SK_RUN_FILTER(filter, ctx) \
 	(*filter->prog->bpf_func)(ctx, filter->prog->insnsi)
 
-struct bpf_insn {
-	__u8	code;		/* opcode */
-	__u8	dst_reg:4;	/* dest register */
-	__u8	src_reg:4;	/* source register */
-	__s16	off;		/* signed offset */
-	__s32	imm;		/* signed immediate constant */
-};
-
 #ifdef CONFIG_COMPAT
 /* A struct sock_filter is architecture independent. */
 struct compat_sock_fprog {

diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 24e9033f8b3f..fb3f7b675229 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild

@@ -67,6 +67,7 @@ header-y += bfs_fs.h
 header-y += binfmts.h
 header-y += blkpg.h
 header-y += blktrace_api.h
+header-y += bpf.h
 header-y += bpqether.h
 header-y += bsg.h
 header-y += btrfs.h

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
new file mode 100644
index 000000000000..479ed0b6be16
--- /dev/null
+++ b/include/uapi/linux/bpf.h

@@ -0,0 +1,65 @@
+/* Copyright (c) 2011-2014 PLUMgrid, http://plumgrid.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ */
+#ifndef _UAPI__LINUX_BPF_H__
+#define _UAPI__LINUX_BPF_H__
+
+#include <linux/types.h>
+
+/* Extended instruction set based on top of classic BPF */
+
+/* instruction classes */
+#define BPF_ALU64	0x07	/* alu mode in double word width */
+
+/* ld/ldx fields */
+#define BPF_DW		0x18	/* double word */
+#define BPF_XADD	0xc0	/* exclusive add */
+
+/* alu/jmp fields */
+#define BPF_MOV		0xb0	/* mov reg to reg */
+#define BPF_ARSH	0xc0	/* sign extending arithmetic shift right */
+
+/* change endianness of a register */
+#define BPF_END		0xd0	/* flags for endianness conversion: */
+#define BPF_TO_LE	0x00	/* convert to little-endian */
+#define BPF_TO_BE	0x08	/* convert to big-endian */
+#define BPF_FROM_LE	BPF_TO_LE
+#define BPF_FROM_BE	BPF_TO_BE
+
+#define BPF_JNE		0x50	/* jump != */
+#define BPF_JSGT	0x60	/* SGT is signed '>', GT in x86 */
+#define BPF_JSGE	0x70	/* SGE is signed '>=', GE in x86 */
+#define BPF_CALL	0x80	/* function call */
+#define BPF_EXIT	0x90	/* function return */
+
+/* Register numbers */
+enum {
+	BPF_REG_0 = 0,
+	BPF_REG_1,
+	BPF_REG_2,
+	BPF_REG_3,
+	BPF_REG_4,
+	BPF_REG_5,
+	BPF_REG_6,
+	BPF_REG_7,
+	BPF_REG_8,
+	BPF_REG_9,
+	BPF_REG_10,
+	__MAX_BPF_REG,
+};
+
+/* BPF has 10 general purpose 64-bit registers and stack frame. */
+#define MAX_BPF_REG	__MAX_BPF_REG
+
+struct bpf_insn {
+	__u8	code;		/* opcode */
+	__u8	dst_reg:4;	/* dest register */
+	__u8	src_reg:4;	/* source register */
+	__s16	off;		/* signed offset */
+	__s32	imm;		/* signed immediate constant */
+};
+
+#endif /* _UAPI__LINUX_BPF_H__ */

-- 
1.7.9.5

add BPF_LD_IMM64 instruction to load 64-bit immediate value into a register.
All previous instructions were 8-byte. This is first 16-byte instruction.
Two consecutive 'struct bpf_insn' blocks are interpreted as single instruction:
insn[0].code = BPF_LD | BPF_DW | BPF_IMM
insn[0].dst_reg = destination register
insn[0].imm = lower 32-bit
insn[1].code = 0
insn[1].imm = upper 32-bit
All unused fields must be zero.

Classic BPF has similar instruction: BPF_LD | BPF_W | BPF_IMM
which loads 32-bit immediate value into a register.

x64 JITs it as single 'movabsq %rax, imm64'
arm64 may JIT as sequence of four 'movk x0, #imm16, lsl #shift' insn

Note that old eBPF programs are binary compatible with new interpreter.

It helps eBPF programs load 64-bit constant into a register with one
instruction instead of using two registers and 4 instructions:
BPF_MOV32_IMM(R1, imm32)
BPF_ALU64_IMM(BPF_LSH, R1, 32)
BPF_MOV32_IMM(R2, imm32)
BPF_ALU64_REG(BPF_OR, R1, R2)

User space generated programs will use this instruction to load constants only.

To tell kernel that user space needs a pointer the _pseudo_ variant of
this instruction may be added later, which will use extra bits of encoding
to indicate what type of pointer user space is asking kernel to provide.
For example 'off' or 'src_reg' fields can be used for such purpose.
src_reg = 1 could mean that user space is asking kernel to validate and
load in-kernel map pointer.
src_reg = 2 could mean that user space needs readonly data section pointer
src_reg = 3 could mean that user space needs a pointer to per-cpu local data
All such future pseudo instructions will not be carrying the actual pointer
as part of the instruction, but rather will be treated as a request to kernel
to provide one. The kernel will verify the request_for_a_pointer, then
will drop _pseudo_ marking and will store actual internal pointer inside
the instruction, so the end result is the interpreter and JITs never
see pseudo BPF_LD_IMM64 insns and only operate on generic BPF_LD_IMM64 that
loads 64-bit immediate into a register. User space never operates on direct
pointers and verifier can easily recognize request_for_pointer vs other
instructions.

Signed-off-by: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
---
 Documentation/networking/filter.txt |    8 +++++++-
 arch/x86/net/bpf_jit_comp.c         |   17 +++++++++++++++++
 include/linux/filter.h              |   18 ++++++++++++++++++
 kernel/bpf/core.c                   |    5 +++++
 lib/test_bpf.c                      |   21 +++++++++++++++++++++
 5 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/filter.txt b/Documentation/networking/filter.txt
index c48a9704bda8..81916ab5d96f 100644
--- a/Documentation/networking/filter.txt
+++ b/Documentation/networking/filter.txt

@@ -951,7 +951,7 @@ Size modifier is one of ...
 
 Mode modifier is one of:
 
-  BPF_IMM  0x00  /* classic BPF only, reserved in eBPF */
+  BPF_IMM  0x00  /* used for 32-bit mov in classic BPF and 64-bit in eBPF */
   BPF_ABS  0x20
   BPF_IND  0x40
   BPF_MEM  0x60

@@ -995,6 +995,12 @@ BPF_XADD | BPF_DW | BPF_STX: lock xadd *(u64 *)(dst_reg + off16) += src_reg
 Where size is one of: BPF_B or BPF_H or BPF_W or BPF_DW. Note that 1 and
 2 byte atomic increments are not supported.
 
+eBPF has one 16-byte instruction: BPF_LD | BPF_DW | BPF_IMM which consists
+of two consecutive 'struct bpf_insn' 8-byte blocks and interpreted as single
+instruction that loads 64-bit immediate value into a dst_reg.
+Classic BPF has similar instruction: BPF_LD | BPF_W | BPF_IMM which loads
+32-bit immediate value into a register.
+
 Testing
 -------
 

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 39ccfbb4a723..06f8c17f5484 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c

@@ -393,6 +393,23 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 			EMIT1_off32(add_1reg(0xB8, dst_reg), imm32);
 			break;
 
+		case BPF_LD | BPF_IMM | BPF_DW:
+			if (insn[1].code != 0 || insn[1].src_reg != 0 ||
+			    insn[1].dst_reg != 0 || insn[1].off != 0) {
+				/* verifier must catch invalid insns */
+				pr_err("invalid BPF_LD_IMM64 insn\n");
+				return -EINVAL;
+			}
+
+			/* movabsq %rax, imm64 */
+			EMIT2(add_1mod(0x48, dst_reg), add_1reg(0xB8, dst_reg));
+			EMIT(insn[0].imm, 4);
+			EMIT(insn[1].imm, 4);
+
+			insn++;
+			i++;
+			break;
+
 			/* dst %= src, dst /= src, dst %= imm32, dst /= imm32 */
 		case BPF_ALU | BPF_MOD | BPF_X:
 		case BPF_ALU | BPF_DIV | BPF_X:

diff --git a/include/linux/filter.h b/include/linux/filter.h
index c78994593355..bf323da77950 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h

@@ -166,6 +166,24 @@ enum {
 		.off   = 0,					\
 		.imm   = IMM })
 
+/* BPF_LD_IMM64 macro encodes single 'load 64-bit immediate' insn */
+#define BPF_LD_IMM64(DST, IMM)					\
+	BPF_LD_IMM64_RAW(DST, 0, IMM)
+
+#define BPF_LD_IMM64_RAW(DST, SRC, IMM)				\
+	((struct bpf_insn) {					\
+		.code  = BPF_LD | BPF_DW | BPF_IMM,		\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = 0,					\
+		.imm   = (__u32) (IMM) }),			\
+	((struct bpf_insn) {					\
+		.code  = 0, /* zero is reserved opcode */	\
+		.dst_reg = 0,					\
+		.src_reg = 0,					\
+		.off   = 0,					\
+		.imm   = ((__u64) (IMM)) >> 32 })
+
 /* Short form of mov based on type, BPF_X: dst_reg = src_reg, BPF_K: dst_reg = imm32 */
 
 #define BPF_MOV64_RAW(TYPE, DST, SRC, IMM)			\

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index b54bb2c2e494..2c2bfaacce66 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c

@@ -242,6 +242,7 @@ static unsigned int __bpf_prog_run(void *ctx, const struct bpf_insn *insn)
 		[BPF_LD | BPF_IND | BPF_W] = &&LD_IND_W,
 		[BPF_LD | BPF_IND | BPF_H] = &&LD_IND_H,
 		[BPF_LD | BPF_IND | BPF_B] = &&LD_IND_B,
+		[BPF_LD | BPF_IMM | BPF_DW] = &&LD_IMM_DW,
 	};
 	void *ptr;
 	int off;

@@ -301,6 +302,10 @@ select_insn:
 	ALU64_MOV_K:
 		DST = IMM;
 		CONT;
+	LD_IMM_DW:
+		DST = (u64) (u32) insn[0].imm | ((u64) (u32) insn[1].imm) << 32;
+		insn++;
+		CONT;
 	ALU64_ARSH_X:
 		(*(s64 *) &DST) >>= SRC;
 		CONT;

diff --git a/lib/test_bpf.c b/lib/test_bpf.c
index 9a67456ba29a..413890815d3e 100644
--- a/lib/test_bpf.c
+++ b/lib/test_bpf.c

@@ -1735,6 +1735,27 @@ static struct bpf_test tests[] = {
 		{ },
 		{ { 1, 0 } },
 	},
+	{
+		"load 64-bit immediate",
+		.u.insns_int = {
+			BPF_LD_IMM64(R1, 0x567800001234L),
+			BPF_MOV64_REG(R2, R1),
+			BPF_MOV64_REG(R3, R2),
+			BPF_ALU64_IMM(BPF_RSH, R2, 32),
+			BPF_ALU64_IMM(BPF_LSH, R3, 32),
+			BPF_ALU64_IMM(BPF_RSH, R3, 32),
+			BPF_ALU64_IMM(BPF_MOV, R0, 0),
+			BPF_JMP_IMM(BPF_JEQ, R2, 0x5678, 1),
+			BPF_EXIT_INSN(),
+			BPF_JMP_IMM(BPF_JEQ, R3, 0x1234, 1),
+			BPF_EXIT_INSN(),
+			BPF_ALU64_IMM(BPF_MOV, R0, 1),
+			BPF_EXIT_INSN(),
+		},
+		INTERNAL,
+		{ },
+		{ { 0, 1 } }
+	},
 };
 
 static struct net_device dev;

-- 
1.7.9.5

On Thu, Sep 04, 2014 at 10:17:18PM -0700, Alexei Starovoitov wrote:

allow user space to generate eBPF programs

uapi/linux/bpf.h: eBPF instruction set definition

linux/filter.h: the rest

This patch only moves macro definitions, but practically it freezes existing
eBPF instruction set, though new instructions can still be added in the future.

These eBPF definitions cannot go into uapi/linux/filter.h, since the names
may conflict with existing applications.

Full eBPF ISA description is in Documentation/networking/filter.txt

I think you need to have at least one single interface using this
before you can expose it to userspace. So this should come in the
small batch that introduces the first interface of your ebpf code in
userspace. AFAIK, this has been the policy so far.

On Sat, Sep 6, 2014 at 7:10 AM, Pablo Neira Ayuso [off-list ref] wrote:

On Thu, Sep 04, 2014 at 10:17:18PM -0700, Alexei Starovoitov wrote:

quoted

allow user space to generate eBPF programs

uapi/linux/bpf.h: eBPF instruction set definition

linux/filter.h: the rest

This patch only moves macro definitions, but practically it freezes existing
eBPF instruction set, though new instructions can still be added in the future.

These eBPF definitions cannot go into uapi/linux/filter.h, since the names
may conflict with existing applications.

Full eBPF ISA description is in Documentation/networking/filter.txt

I think you need to have at least one single interface using this
before you can expose it to userspace. So this should come in the
small batch that introduces the first interface of your ebpf code in
userspace. AFAIK, this has been the policy so far.

That's what I've been doing over the last year.
My first eBPF patch was in Sep of 2013!
since then I've been only tweaking and massaging it.
Nothing fundamentally changed.
Last few month I've been posting these series with not only
first user, but with multiple. Many examples, test cases and so on.
The series became big and Dave asked to split them.
Please see the cover letter. These two patches is stage I.
More examples and use cases in stage II, stage III, stage IV, etc
All these patches are ready. I'm only submitting them one at
a time to make review easier.
Please see them in my tree if you interested.

Thanks

On Sat, Sep 06, 2014 at 09:04:23AM -0700, Alexei Starovoitov wrote:

On Sat, Sep 6, 2014 at 7:10 AM, Pablo Neira Ayuso [off-list ref] wrote:

quoted

On Thu, Sep 04, 2014 at 10:17:18PM -0700, Alexei Starovoitov wrote:

quoted

allow user space to generate eBPF programs

uapi/linux/bpf.h: eBPF instruction set definition

linux/filter.h: the rest

This patch only moves macro definitions, but practically it freezes existing
eBPF instruction set, though new instructions can still be added in the future.

These eBPF definitions cannot go into uapi/linux/filter.h, since the names
may conflict with existing applications.

Full eBPF ISA description is in Documentation/networking/filter.txt

I think you need to have at least one single interface using this
before you can expose it to userspace. So this should come in the
small batch that introduces the first interface of your ebpf code in
userspace. AFAIK, this has been the policy so far.

That's what I've been doing over the last year.
My first eBPF patch was in Sep of 2013!
since then I've been only tweaking and massaging it.
Nothing fundamentally changed.
Last few month I've been posting these series with not only
first user, but with multiple. Many examples, test cases and so on.
The series became big and Dave asked to split them.
Please see the cover letter. These two patches is stage I.
More examples and use cases in stage II, stage III, stage IV, etc

If the patches that provide the very first user interface don't get in
time to this merge window round for whatever reason, we'll have the
layout of this exposed to userspace in the next kernel version with no
clients at all, that doesn't make sense to me.

I don't think the speed up of the llvm submission is a good argument,
this sounds to me similar to the "please apply this patch that
reserves this new netlink family in include/linux/netlink.h, I promise
this new subsystem will be submitted soon though. Meanwhile this will
speed up submission of my userspace software to distributions for
packaging" argument.

I think you have to find the way to send a small batch with the very
essencial stuff that, if merged mainstream, will provide just one new
feature while leaving the repository in consistent state. Then, send
follow up patches that enhance your thing and that add new clients of
it.

On Sun, Sep 7, 2014 at 11:07 AM, Pablo Neira Ayuso [off-list ref] wrote:

If the patches that provide the very first user interface don't get in
time to this merge window round for whatever reason, we'll have the
layout of this exposed to userspace in the next kernel version with no
clients at all, that doesn't make sense to me.

eBPF cannot have the first user without verifier and tracing
fully reviewed, so first user cannot be in the first
patch no matter what.

In particular this patch only exposed eBPF as an _instruction set_
to user space. llvm and gcc are only two users.
llvm patches _were_ submitted to the list.
Compilers are not some fictitious users.
eBPF ISA is solid. Two backends is a proof.

For eBPF to be loaded, verifier, syscall and other pieces need to
come in gradually. So I logically split them in series:
stage I - expose instruction set
stage II - bpf syscall for maps and manpage
stage III - programs, verifier and user space testsuite
stage IV - ebpf+tracing (the first user of ebpf isa and syscall)
stage V - ebpf+sockets
stage VI - ebpf+ovs
all of the patches _were_ submitted in the past.
Split is done to make review and integration easier.

After stage III user space will be able to load eBPF programs,
but they will still be useless, because they cannot be attached
to anything. Realistically only stage IV makes first real use of
them in tracing.
You know this, yet, you're saying the first user must be
in the first series. Really, what this 'feedback' is about?

I don't think the speed up of the llvm submission is a good argument,
this sounds to me similar to the "please apply this patch that
reserves this new netlink family in include/linux/netlink.h, I promise
this new subsystem will be submitted soon though. Meanwhile this will
speed up submission of my userspace software to distributions for
packaging" argument.

You're not correct here. I'm not saying 'I promise it will be submitted'.
There _were_ already submitted. I split them in chunks to make
review easier and to follow standard linux philosophy of making
small decisions that can be reverted.
We still have a month until merge window, so if stages II and III
don't make it in time, Dave can revert these small patches just
as easily. You're advocating first_user_must_be_in_first_patch
approach, which is against the linux methodology.

I think you have to find the way to send a small batch with the very
essencial stuff that, if merged mainstream, will provide just one new
feature while leaving the repository in consistent state. Then, send
follow up patches that enhance your thing and that add new clients of
it.

As I said above the _minimum_ useful program needs verifier
which is more than Dave's cutoff of 10 patches. Therefore I
split all very_essential_stuff into these stages.
Note I'm not sending radix-tree type of eBPF maps as part
of these stages, neither I send array type of eBPF maps,
though they're needed for my last stage (ebpf+ovs).
I'm not sending pointer leak detector for verifier either,
it will be needed before syscall can be exposed to unprivileged
users, etc. There is a lot of stuff that I need for ebpf+ovs that
is _not_ part of these stages. What you see is really
the minimum to make ebpf+tracing useful.
btw, all of ebpf+ovs was submitted to the list back in Sep 2013.

So there is no practical way to do first set of < 10 patches
that will be usable from user space on its own.
Even if I remove verifier and maps altogether, bpf programs
need to be attached to something like tracing and
that is again >10 patches.
These stages with small patches is only sensible approach.

* Alexei Starovoitov [off-list ref] wrote:

quoted

I don't think the speed up of the llvm submission is a good 
argument, this sounds to me similar to the "please apply this 
patch that reserves this new netlink family in 
include/linux/netlink.h, I promise this new subsystem will be 
submitted soon though. Meanwhile this will speed up 
submission of my userspace software to distributions for 
packaging" argument.

You're not correct here. I'm not saying 'I promise it will be 
submitted'. There _were_ already submitted. [...]

And this split-up smaller submissions was requested by David 
Miller, the networking maintainer, so if Pablo wants another 
submission format, he needs to take it up with David - we can't 
do both at once obviously.

Thanks,

	Ingo

From: Ingo Molnar <redacted>
Date: Mon, 8 Sep 2014 07:23:29 +0200

* Alexei Starovoitov [off-list ref] wrote:

quoted

quoted

I don't think the speed up of the llvm submission is a good 
argument, this sounds to me similar to the "please apply this 
patch that reserves this new netlink family in 
include/linux/netlink.h, I promise this new subsystem will be 
submitted soon though. Meanwhile this will speed up 
submission of my userspace software to distributions for 
packaging" argument.

You're not correct here. I'm not saying 'I promise it will be 
submitted'. There _were_ already submitted. [...]

And this split-up smaller submissions was requested by David 
Miller, the networking maintainer, so if Pablo wants another 
submission format, he needs to take it up with David - we can't 
do both at once obviously.

I think that just because I asked the submission size to be smaller,
it does not mean that you can submit things before you provide the
initial user as well.

And how to work that out and keep the submission size reasonable is
the submitter's problem, not mine.

On Sun, Sep 7, 2014 at 10:28 PM, David Miller [off-list ref] wrote:

From: Ingo Molnar <redacted>
Date: Mon, 8 Sep 2014 07:23:29 +0200

quoted

* Alexei Starovoitov [off-list ref] wrote:

quoted

quoted

I don't think the speed up of the llvm submission is a good
argument, this sounds to me similar to the "please apply this
patch that reserves this new netlink family in
include/linux/netlink.h, I promise this new subsystem will be
submitted soon though. Meanwhile this will speed up
submission of my userspace software to distributions for
packaging" argument.

You're not correct here. I'm not saying 'I promise it will be
submitted'. There _were_ already submitted. [...]

And this split-up smaller submissions was requested by David
Miller, the networking maintainer, so if Pablo wants another
submission format, he needs to take it up with David - we can't
do both at once obviously.

I think that just because I asked the submission size to be smaller,
it does not mean that you can submit things before you provide the
initial user as well.

And how to work that out and keep the submission size reasonable is
the submitter's problem, not mine.

imo llvm is more than enough for the first user, no?
I think I've explained that it's practically impossible to have
first in-tree user in the first patch. What do you suggest?
I'm listening, but currently I see no way out.
The patch was large. I broke it down. The first patch is as tiny
as it can get, but Pablo is stalling it, like he did for the last year.
Honestly it doesn't feel fair.
When there were technical arguments (like global vs fd) or
(union attr vs long for syscall) I've listened and rewrote things.
Now it doesn't sound technical.

* David Miller [off-list ref] wrote:

From: Ingo Molnar <redacted>
Date: Mon, 8 Sep 2014 07:23:29 +0200

quoted

* Alexei Starovoitov [off-list ref] wrote:

quoted

quoted

I don't think the speed up of the llvm submission is a 
good argument, this sounds to me similar to the "please 
apply this patch that reserves this new netlink family in 
include/linux/netlink.h, I promise this new subsystem will 
be submitted soon though. Meanwhile this will speed up 
submission of my userspace software to distributions for 
packaging" argument.

You're not correct here. I'm not saying 'I promise it will 
be submitted'. There _were_ already submitted. [...]

And this split-up smaller submissions was requested by David 
Miller, the networking maintainer, so if Pablo wants another 
submission format, he needs to take it up with David - we 
can't do both at once obviously.

I think that just because I asked the submission size to be 
smaller, it does not mean that you can submit things before you 
provide the initial user as well.

And how to work that out and keep the submission size 
reasonable is the submitter's problem, not mine.

That's a pretty harsh requirement but might be doable 
technically: Alexei, please submit a series that is large enough 
to provide self-sufficient functionality as per Pablo's request,
but is also small and minimal, as per David's request.

If that fails then another route would be to decouple from 
networking initially and create something new and stand-alone in 
kernel/ebpf/ (or any other name really), with tracing and perf 
usecases, with networking integration and code deduplication to 
be done at the end, when there can be no legitimate argument 
about its utility.

Thanks,

	Ingo

From: Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org>
Date: Thu,  4 Sep 2014 22:17:16 -0700

V9->V10
- no changes, added Daniel's ack

Note they're on top of Hannes's patch in the same area [1]

V8 thread with 'why' reasoning and end goal [2]

Original set [3] of ~28 patches I'm planning to present in 4 stages:

  I. this 2 patches to fork off llvm upstreaming
 II. bpf syscall with manpage and map implementation
III. bpf program load/unload with verifier testsuite (1st user of
     instruction macros from bpf.h and 1st user of load imm64 insn)
 IV. tracing, etc

[1] http://patchwork.ozlabs.org/patch/385266/
[2] https://lkml.org/lkml/2014/8/27/628
[3] https://lkml.org/lkml/2014/8/26/859

Begrudgingly, I've applied this series.

Although I really wish you had included the mechanism for userland to
use the eBPF instructions alongside exporting them to userspace.

You kept saying "LLVM is the user" but that's a bullshit argument
because you aren't including the patches necessary to actually
propagate native eBPF programs into the kernel.

That's what, 1 or 2 patches, right?  Which is not an unreasonable
request.

Anyways, I'm just extremely frustrated with how you operate and work,
you push things way too hard.  I hate to say this, but you are the
kind of submitter who gets his way by being persistent rather than
making well formed pleasant submissions that are easy to integrate.

On Tue, Sep 9, 2014 at 10:30 AM, David Miller [off-list ref] wrote:

You kept saying "LLVM is the user" but that's a bullshit argument
because you aren't including the patches necessary to actually
propagate native eBPF programs into the kernel.

That's what, 1 or 2 patches, right?  Which is not an unreasonable
request.

I just don't see how I could do that in 2 patches.
In V8 series the first user was appearing in patch 22 out of 28.
Last two days I spent solely hacking the series to move it sooner.
Today the first user is still in patch 15.
Will try to shorten them even more.
So blame it on my skills and not my attitude.

Anyways, I'm just extremely frustrated with how you operate and work,
you push things way too hard.  I hate to say this, but you are the
kind of submitter who gets his way by being persistent rather than
making well formed pleasant submissions that are easy to integrate.

Understood. Will try to make patches shorter and hopefully
more pleasant. I think 'being persistent' is mandatory here :)