Jakub Kicinski <kuba@xxxxxxxxxx> writes:
> On Mon, 20 Sep 2021 23:01:48 +0200 Toke Høiland-Jørgensen wrote:
>> > 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...
> 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?
> 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).
-Toke
