On Tue, Sep 21, 2021 at 12:04 PM Eelco Chaudron [off-list ref] wrote:

On 21 Sep 2021, at 0:44, Toke Høiland-Jørgensen wrote:

quoted

Jakub Kicinski [off-list ref] writes:

quoted

On Mon, 20 Sep 2021 23:01:48 +0200 Toke Høiland-Jørgensen wrote:

quoted

quoted

In fact I don't think there is anything infra can do better for
flushing than the prog itself:

   bool mod = false;

   ptr = bpf_header_pointer(...);
   ...
   if (some_cond(...)) {
           change_packet(...);
           mod = true;
   }
   ...
   if (mod)

to have an additional check like:

if (mod && ptr == stack)

(or something to that effect). No?

Good point. Do you think we should have the kernel add/inline this
optimization or have the user do it explicitly.

Hmm, good question. On the one hand it seems like an easy optimisation
to add, but on the other hand maybe the caller has other logic that can
better know how/when to omit the check.

Hmm, but the helper needs to check it anyway, doesn't it? At least it
can't just blindly memcpy() if the source and destination would be the
same...

quoted

The draft API was:

void *xdp_mb_pointer_flush(struct xdp_buff *xdp_md, u32 flags,
                           u32 offset, u32 len, void *stack_buf)

Which does not take the ptr returned by header_pointer(), but that's
easy to add (well, easy other than the fact it'd be the 6th arg).

I guess we could play some trickery with stuffing offset/len/flags into
one or two u64s to save an argument or two?

quoted

BTW I drafted the API this way to cater to the case where flush()
is called without a prior call to header_pointer(). For when packet
trailer or header is populated directly from a map value. Dunno if
that's actually useful, either.

Ah, didn't think of that; so then it really becomes a generic
xdp_store_bytes()-type helper? Might be useful, I suppose. Adding
headers is certainly a fairly common occurrence, but dunno to what
extent they'd be copied wholesale from a map (hadn't thought about doing
that before either).

Sorry for commenting late but I was busy and had to catch up on emails...

I like the idea, as these APIs are exactly what I proposed in April, https://lore.kernel.org/bpf/FD3E6E08-DE78-4FBA-96F6-646C93E88631@redhat.com/ (local)

I did not call it flush, as it can be used as a general function to copy data to a specific location.

Here is some performance data (throughput) for this patch set on i40e
(40 Gbit/s NIC). All using the xdp_rxq_info sample and NO multi-buffer
packets.

With v14 only:

XDP_DROP: +4%
XDP_TX: +1%
XDP_PASS: -1%

With v14 plus multi-buffer support implemented in i40e courtesy of Tirtha:

XDP_DROP: +3%
XDP_TX: -1%
XDP_PASS: -2%

/Magnus

//Eelco