af_xdp: move rx timestamp read to support WODA #251
Conversation
wdebruij
force-pushed
the
master
branch
2 times, most recently
from
623ae39 to
d77f5c3
Compare
There was a problem hiding this comment.
While this change looks good to me, I still would like @sianj-xilinx to approve too.
|
Ping |
There was a problem hiding this comment.
Looks like a good change, thanks!
I've briefly tested this with some internal woda_latency_{server,client} apps and they seem to be happy to run over AF_XDP with this change, for internal reviewers, you can find the test log at ~jfeather/rhome/dev/public-pr-251/woda_latency_server.log.
I would like to check what testing @ivatet-amd has done and why specifically he wanted @sianj-xilinx's review before merging this, but he is currently off, so I'll aim to come back to this PR in the coming weeks.
src/lib/ciul/efxdp_vi.c
Outdated
| ts_out->tv_sec = meta->tstamp / ns_to_sec; | ||
| ts_out->tv_nsec = meta->tstamp % ns_to_sec; | ||
| ts_out->tv_nsec_frac = 0; | ||
| ts_out->tv_flags = EF_VI_SYNC_FLAG_CLOCK_SET | EF_VI_SYNC_FLAG_CLOCK_IN_SYNC; |
There was a problem hiding this comment.
Rather than fabricate traceability claims, would it be sufficient for WODA to claim only the CLOCK_SET flag and leave the IN_SYNC flag clear with EF_TIMESTAMPING_REPORTING kept at 0?
There was a problem hiding this comment.
Good point.
As a matter of fact, after reviewing the code and running src/tests/onload/wire_order yesterday, I do not see a requirement of having either flag set for WODA at all.
They are used, of course, in SO_TIMESTAMPING to decide whether to fill in the (deprecated in Linux) SOF_TIMESTAMPING_SYS_HARDWARE field.
But for WODA, the critical part is only that ef_vi_receive_get_precise_timestamp returns 0, so that record_rx_timestamp is called.
I've been staring at this for a while, as it would mean that I was mistaken in October when I wrote that paragraph. Unfortunately, the comment does not explain why at the time I believed that "WODA also needs the EF_VI_SYNC flags passed".
From both testing and code inspection today, I believe it does not. WODA can work on the raw hardware timestamps, regardless of whether they are synced with the host clock or with PTP. It can also operate on trailer timestamps, after all, for which this information is not communicated explicitly either.
So agreed that EF_VI_SYNC_FLAG_CLOCK_IN_SYNC can be removed if EF_TIMESTAMPING_REPORTING is kept zero. But probably even better to leave both zero.
If you agree, I'll do that.
Thanks for the review!
There was a problem hiding this comment.
I have updated the commit as in my previous comment.
Move AF_XDP rx timestamp read into vi->ops.receive_get_timestamp. The original implementation added a branch in device independent get_rx_timestamp. This is not needed, and it breaks WODA, because record_rx_timestamp is skipped. Fix this, and as a side-effect better isolate the AF_XDP from the device independent code. With this change, src/tests/onload/wire_order succeeds. Signed-off-by: Willem de Bruijn <[email protected]>
Move AF_XDP rx timestamp read into vi->ops.receive_get_timestamp.
The original implementation added a branch in device independent get_rx_timestamp. This is not needed, and it breaks WODA, because record_rx_timestamp is skipped.
Fix this, and as a side-effect better isolate the AF_XDP from the device independent code.
WODA also needs the EF_VI_SYNC flags passed. XDP offers no way to query the device for these currently. Similar to ef10_mcdi_event, assume clock is always in sync.
With this change, src/tests/onload/wire_order succeeds.