RE: [PATCH bpf-next 6/8] bpf: Add XDP_REDIRECT support to XDP for bpf_prog_run()

Toke Høiland-Jørgensen wrote:

Toke Høiland-Jørgensen [off-list ref] writes:

quoted

John Fastabend [off-list ref] writes:

quoted

Toke Høiland-Jørgensen wrote:

quoted

This adds support for doing real redirects when an XDP program returns
XDP_REDIRECT in bpf_prog_run(). To achieve this, we create a page pool
instance while setting up the test run, and feed pages from that into the
XDP program. The setup cost of this is amortised over the number of
repetitions specified by userspace.

To support performance testing use case, we further optimise the setup step
so that all pages in the pool are pre-initialised with the packet data, and
pre-computed context and xdp_frame objects stored at the start of each
page. This makes it possible to entirely avoid touching the page content on
each XDP program invocation, and enables sending up to 11.5 Mpps/core on my
test box.

Because the data pages are recycled by the page pool, and the test runner
doesn't re-initialise them for each run, subsequent invocations of the XDP
program will see the packet data in the state it was after the last time it
ran on that particular page. This means that an XDP program that modifies
the packet before redirecting it has to be careful about which assumptions
it makes about the packet content, but that is only an issue for the most
naively written programs.

Previous uses of bpf_prog_run() for XDP returned the modified packet data
and return code to userspace, which is a different semantic then this new
redirect mode. For this reason, the caller has to set the new
BPF_F_TEST_XDP_DO_REDIRECT flag when calling bpf_prog_run() to opt in to
the different semantics. Enabling this flag is only allowed if not setting
ctx_out and data_out in the test specification, since it means frames will
be redirected somewhere else, so they can't be returned.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---

[...]

quoted

+static int bpf_test_run_xdp_redirect(struct bpf_test_timer *t,
+				     struct bpf_prog *prog, struct xdp_buff *orig_ctx)
+{
+	void *data, *data_end, *data_meta;
+	struct xdp_frame *frm;
+	struct xdp_buff *ctx;
+	struct page *page;
+	int ret, err = 0;
+
+	page = page_pool_dev_alloc_pages(t->xdp.pp);
+	if (!page)
+		return -ENOMEM;
+
+	ctx = ctx_from_page(page);
+	data = ctx->data;
+	data_meta = ctx->data_meta;
+	data_end = ctx->data_end;
+
+	ret = bpf_prog_run_xdp(prog, ctx);
+	if (ret == XDP_REDIRECT) {
+		frm = (struct xdp_frame *)(ctx + 1);
+		/* if program changed pkt bounds we need to update the xdp_frame */

Because this reuses the frame repeatedly is there any issue with also
updating the ctx each time? Perhaps if the prog keeps shrinking
the pkt it might wind up with 0 len pkt? Just wanted to ask.

Sure, it could. But the data buffer comes from userspace anyway, and
there's nothing preventing userspace from passing a 0-length packet
anyway, so I just mentally put this in the "don't do that, then" bucket :)

At least I don't *think* there's actually any problem with this that we
don't have already? A regular XDP program can also shrink an incoming
packet to zero, then redirect it, no?

Another thought is that we could of course do the opposite here: instead
of updating the xdp_frame when the program resizes the packet, just
reset the pointers so that the next invocation will get the original
size again? The data would still be changed, but maybe that behaviour is
less surprising? WDYT?

Should read my email from newest to oldest :)

I think resetting it back to the original size is less surprising. And
if I want to benchmark a helper that moves the pointers it will be
easier. For example benchmarking shrinking a packet with current
code wouldn't really work because eventually the packet will be 0
and my test will stop doing what I expect.

Lets do the reset back to original size.

Thanks,
John

-Toke