On 10/11/2022 18.39, John Fastabend wrote:
Stanislav Fomichev wrote:
On Wed, Nov 9, 2022 at 5:35 PM John Fastabend <john.fastabend@xxxxxxxxx> wrote:
Stanislav Fomichev wrote:
On Wed, Nov 9, 2022 at 4:26 PM John Fastabend <john.fastabend@xxxxxxxxx> wrote:
Stanislav Fomichev wrote:
xskxceiver conveniently setups up veth pairs so it seems logical
to use veth as an example for some of the metadata handling.
We timestamp skb right when we "receive" it, store its
pointer in new veth_xdp_buff wrapper and generate BPF bytecode to
reach it from the BPF program.
This largely follows the idea of "store some queue context in
the xdp_buff/xdp_frame so the metadata can be reached out
from the BPF program".
[...]
orig_data = xdp->data;
orig_data_end = xdp->data_end;
+ vxbuf.skb = skb;
act = bpf_prog_run_xdp(xdp_prog, xdp);
@@ -942,6 +946,7 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
struct sk_buff *skb = ptr;
stats->xdp_bytes += skb->len;
+ __net_timestamp(skb);
Just getting to reviewing in depth a bit more. But we hit veth with lots of
packets in some configurations I don't think we want to add a __net_timestamp
here when vast majority of use cases will have no need for timestamp on veth
device. I didn't do a benchmark but its not free.
If there is a real use case for timestamping on veth we could do it through
a XDP program directly? Basically fallback for devices without hw timestamps.
Anyways I need the helper to support hardware without time stamping.
Not sure if this was just part of the RFC to explore BPF programs or not.
Initially I've done it mostly so I can have selftests on top of veth
driver, but I'd still prefer to keep it to have working tests.
I can't think of a use for it though so its just extra cycles. There
is a helper to read the ktime.
As I mentioned in another reply, I wanted something SW-only to test
this whole metadata story.
Yeah I see the value there. Also because this is in the veth_xdp_rcv
path we don't actually attach XDP programs to veths except for in
CI anyways. I assume though if someone actually does use this in
prod having an extra _net_timestamp there would be extra unwanted
cycles.
Sorry, but I think it is wrong to add this SW-timestamp to veth.
As John already pointed out the BPF-prog can just call ktime helper
itself. Plus, we *do* want to use this code path as a fast-path, not
just for CI testing.
I suggest you use the offloaded VLAN tag instead.
The idea was:
- veth rx sets skb->tstamp (otherwise it's 0 at this point)
- veth kfunc to access rx_timestamp returns skb->tstamp
- xsk bpf program verifies that the metadata is non-zero
- the above shows end-to-end functionality with a software driver
Yep 100% agree very handy for testing just not sure we can add code
to hotpath just for testing.
I agree, I really dislike adding code to hotpath just for testing.
Using VLAN instead would solve a practical problem, that XDP lacks
access to the offloaded VLAN tag. Which is one of the lacking features
that XDP-hints targets to add.
--Jesper
