I have been wondering about xmit_more myself. I don’t think it changes anything for software timestamps, but it may with hardware timestamps.
It arguably makes the software timestamp too early if taken on the first segment, as the NIC is only informed of all the new descriptors when the last segment is written and the doorbell is rung.
Totally makes sense. Possibly this can be improved software TX timestamps by delaying until just before ring buffer is advanced. It would have to be updated in each driver. I may have a look at this once I am complete this patch. Hopefully that one will be a bit smoother.
How do you see that? The skb_tstamp_tx call currently is already the last action before ringing the doorbell, after setting up the descriptor. It cannot be set later.
The only issue specific to GSO is that xmit_more can forego this doorbell until the last segment. We want to complicate this logic with a special case based on tx_flags. A process that cares should either not use GSO, or the timestamp should be associated with the last segment as I've been arguing so far.
Can you elaborate on this suspected memory leak?
A user program cannot free a zerocopy buffer until it is reported as free. If zerocopy events are not reported, that could be a memory leak.
I may have a fix. I have added a -P option when I am running an audit. It doesn’t appear to affect performance, and since implementing it I have received all error messages expected for both timestamp and zerocopy.
I am still testing.
I see, a userspace leak from lack of completion notification.
If the issue is a few missing notifications at the end of the run, then perhaps cfg_waittime_ms is too short.
I’ll get back to you when I have tested this more thoroughly. Early results suggest that adding the -P poll() option has fixed it without any appreciable performance hit. I’ll share raw results with you, and we can make a final decision together.
In the main loop? It still is peculiar that notifications appear to go missing unless the process blocks waiting for them. Nothing in sock_zerocopy_callback or the queueing onto the error queue should cause drops, as far as I know.
Should the test have failed at this point? I did return an error(), but the script kept running.
This should normally be cause for test failure, I think yes. Though it's fine to send the code for review and possibly even merge, so that I can take a look.
Sounds like udpgso_bench.sh needs a ’set -e’ to ensure it stops on first error.
Indeed. Ideally even run all tests, but return error if any failed, like this recent patch
selftests/bpf: fail test_tunnel.sh if subtests fail https://patchwork.ozlabs.org/patch/1105221/
but that may be a lot of code churn and better left to a separate patch.