Page MenuHomeFreeBSD

tcp: LRO timestamps have lost their previous precision
ClosedPublic

Authored by rrs on Tue, Jun 8, 2:32 PM.

Details

Summary

Recently we had a rewrite to tcp_lro.c that was tested but one subtle change
was the move to a less precise timestamp. This causes all kinds of chaos
in tcp's that do pacing and needs to be fixed to use the more precise
time that was there before.

Test Plan

Validate that the precision is back via BB logs.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rrs requested review of this revision.Tue, Jun 8, 2:32 PM
rrs added reviewers: hselasky, gallatin, tuexen.

Hi Randall,

Is it possible to only change getsbintime() into sbintime()?

I mean sbintime() provides 1ns granularity from what I can see. Do you need more than this?

Is the problem here that conversion between different timestamp formats resulted in a negative difference?

I picked sbintime() because its cheaper to compare than bintime.

--HPS

I tried that at First but Lawrence complained since the whole timing
structure of the sbintime is also yet another abbreviation.. so it changes
the granularity of the time (at least according to Lawrence). I did poke
into the code a bit and looked and yes, he was right .. so thats when
I elected to go with bintime.

This makes it so the previous granularity that was in the LRO code is preserved.

sys/netinet/tcp_lro.c
574

Should we change all time computations to use bintime and its helper functions?

Instead of mixing with sbintime?

Are we sure bintime returning always incrementing values when switching from one CPU to another? I mean if the TCP processing threads are not CPU bound?

I hesitated to change the flushing time to use bintime, just because it was easier not
too and in reality few drivers use this, and those that do I think less precision
is required.

As to changing CPU's it should be the same, though I have never explored what kinda
fun could ensue if you go from one cpu to another... Have to think on a way to
actually do that i.e. figure out if some sort of madness ensues.. will have to
think about a good way to measure this...

sys/netinet/tcp_lro.c
574

Comparing a bintime() with a sbinuptime() is that safe? Can you convert this piece to use all bintime's?

This revision is now accepted and ready to land.Tue, Jun 8, 5:56 PM

Sure I can do that, I do think its safe, but will cause some inaccuracy.. let me go work on converting the other to a bintime too.

This addresses Hans concern about comparing a bintime to a sbtime by just
using the bintime and converting things to pure nanoseconds for comparisions.

This revision now requires review to proceed.Wed, Jun 9, 10:59 AM
This revision is now accepted and ready to land.Wed, Jun 9, 12:12 PM