Message-ID: <CAADnVQLWjbm03-3NHYyEx98tWRN68LSaOd3R9fjJoHY5cYoEJg@mail.gmail.com>
Date:   Sat, 8 Jan 2022 11:28:22 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v7 1/3] bpf: Add "live packet" mode for XDP in bpf_prog_run()

On Sat, Jan 8, 2022 at 5:19 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Sure, can do. Doesn't look like BPF_PROG_RUN is documented in there at
> all, so guess I can start such a document :)

prog_run was simple enough.
This live packet mode is a different level of complexity.
Just look at the length of this thread.
We keep finding implementation details that will be relevant
to anyone trying to use this interface.
They all will become part of uapi.

> > Another question comes to mind:
> > What happens when a program modifies the packet?
> > Does it mean that the 2nd frame will see the modified data?
> > It will not, right?
> > It's the page pool size of packets that will be inited the same way
> > at the beginning. Which is NAPI_POLL_WEIGHT * 2 == 128 packets.
> > Why this number?
>
> Yes, you're right: the next run won't see the modified packet data. The
> 128 pages is because we run the program loop in batches of 64 (like NAPI
> does, the fact that TEST_XDP_BATCH and NAPI_POLL_WEIGHT are the same is
> not a coincidence).
>
> We need 2x because we want enough pages so we can keep running without
> allocating more, and the first batch can still be in flight on a
> different CPU while we're processing batch 2.
>
> I experimented with different values, and 128 was the minimum size that
> didn't have a significant negative impact on performance, and above that
> saw diminishing returns.

I guess it's ok-ish to get stuck with 128.
It will be uapi that we cannot change though.
Are you comfortable with that?

> > Should it be configurable?
> > Then the user can say: init N packets with this one pattern
> > and the program will know that exactly N invocation will be
> > with the same data, but N+1 it will see the 1st packet again
> > that potentially was modified by the program.
> > Is it accurate?
>
> I thought about making it configurable, but the trouble is that it's not
> quite as straight-forward as the first N packets being "pristine": it
> depends on what happens to the packet afterwards:
>
> On XDP_DROP, the page will be recycled immediately, whereas on
> XDP_{TX,REDIRECT} it will go through the egress driver after sitting in
> the bulk queue for a little while, so you can get reordering compared to
> the original execution order.
>
> On XDP_PASS the kernel will release the page entirely from the pool when
> building an skb, so you'll never see that particular page again (and
> eventually page_pool will allocate a new batch that will be
> re-initialised to the original value).

That all makes sense. Thanks for explaining.
Please document it and update the selftest.
Looks like XDP_DROP is not tested.
Single packet TX and REDIRECT is imo too weak to give
confidence that the mechanism will not explode with millions of packets.

> If we do want to support a "pristine data" mode, I think the least
> cumbersome way would be to add a flag that would make the kernel
> re-initialise the packet data before every program invocation. The
> reason I didn't do this was because I didn't have a use case for it. The
> traffic generator use case only rewrites a tiny bit of the packet
> header, and it's just as easy to just keep rewriting it without assuming
> a particular previous value. And there's also the possibility of just
> calling bpf_prog_run() multiple times from userspace with a lower number
> of repetitions...
>
> I'm not opposed to adding such a flag if you think it would be useful,
> though. WDYT?

reinit doesn't feel necessary.
How one would use this interface to send N different packets?
The api provides an interface for only one.
It will be copied 128 times, but the prog_run call with repeat=1
will invoke bpf prog only once, right?
So technically doing N prog_run commands with different data
and repeat=1 will achieve the result, right?
But it's not efficient, since 128 pages and 128 copies will be
performed each time.
May be there is a use case for configurable page_pool size?

Powered by blists - more mailing lists