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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

John Fastabend <john.fastabend@gmail.com> writes:

> Toke Høiland-Jørgensen wrote:
>> Toke Høiland-Jørgensen <toke@redhat.com> writes:
>> 
>> > John Fastabend <john.fastabend@gmail.com> writes:
>> >
>> >> Toke Høiland-Jørgensen wrote:
>> >>> 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 <toke@redhat.com>
>> >>> ---
>> >>
>> >> [...]
>> >>
>> >>> +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.

Ah yes, good point!

> Lets do the reset back to original size.

Alright, will do; thanks! :)

-Toke