• Zero t_precisions[which] in tcp_timer_enter(), by mav@

• Provide precision calculation in tcp_timer_next() per Alexander's suggestion.

• Save precisions from callout_when() and use them when scheduling.

In D37321#855299, @mav wrote:

In D37321#855280, @glebius wrote:

Is this returned precision going to be the same for any ticks argument or not? For example I call callout_when for TT_DELACK and store the result, but then I see that TT_PERSIST is going to be run earlier. Can I use precision returned by callout_when() for TT_DELACK to schedule TT_PERSIST?

callout_when() returns precision value as the biggest of two: percent of relative time (either system-wide or passed explicitly with C_PREL() via flags), or explicitly passed to it absolute value. Once with callout_when() you switch from relative to absolute times, you have no relative time and so you can only use absolute precision returned by it as-is. What I think you should do to properly aggregate the events with each other and with other system, is to make tcp_timer_next() return absolute time of the first event (t1) as time and min(time+precision) of all events minus t1 as precision.

In D37321#855280, @glebius wrote:

Oh, thanks! I got another question then. Is this returned precision going to be the same for any ticks argument or not? For example I call callout_when for TT_DELACK and store the result, but then I see that TT_PERSIST is going to be run earlier. Can I use precision returned by callout_when() for TT_DELACK to schedule TT_PERSIST?

In D37321#855262, @mav wrote:

In D37321#855260, @glebius wrote:

In D37321#855218, @mav wrote:

Even old callout_reset_on() is not that strict by using system-wide precision of 5%.

I don't see that. In sys/callout.h:

See tc_precexp in callout_when(). You've lost it when you dropped precision argument returned by callout_when().

In D37321#855218, @mav wrote:

Even old callout_reset_on() is not that strict by using system-wide precision of 5%.

In D37321#855218, @mav wrote:

In D37321#855118, @glebius wrote:

Do I understand it correct that the "precision" argument to callout_reset_sbt_on() becomes pretty useless if we specify C_HARDLOCK?

It is not useless if specific CPU is idle when the time of the event comes. The current patch passing zero precision would wake up CPU exactly on that hardclock tick, whenever it is needed for anything else or not. May be it could aggregate with some other hardclock callouts there, but more likely not. Even old callout_reset_on() is not that strict by using system-wide precision of 5%. If TCP can allow lower precision, then you should specify it. It allows the callout to be handled at some later time when CPU finally wake up. For that to work you should probably store the absolute precision values in addition to absolute times and make tcp_timer_next() return both time and precision, if different TCP timers may have too different precisions that may conflict (formally later timer may have smaller precision and so can not wait for long precision of the formally first timer, that is normally resolved by callouts subsystem).

I am not a big fan of C_HARDCLOCK any more. With fast TSC clocks everywhere C_HARDCLOCK's ability to save on sbinuptime() gives much less benefits. It is still OK to use it, if the area is really tied to hardclock in some way, but I would prefer the precision mechanism to be used when possible instead or in addition to reach events aggregation.

• Use callout_when() to make sure that we are rounded down
• Don't use local variable named "ticks"

I see. The current code uses callout_reset_on() which has C_HARDCLOCK in the macro, that does this rounding down. Do I understand it correct that the "precision" argument to callout_reset_sbt_on() becomes pretty useless if we specify C_HARDLOCK?

I plan to push this change next week.

I got some profiling data when running without WITNESS and with patch that counts the loops. The machines were serving 10 - 100 Gbit/s, but as they are using RACK, they use TCP timers not as often as the default stack does. Some machines didn't detect any looping but most experienced very little looping:

Profiling data for longer period. Note that the kernel is with WITNESS, I believe this explains very long spin times. In a day or two I will have larger profiling data for a group of machines and without WITNESS.

# sysctl net.inet.tcp.timer_stop_loops
net.inet.tcp.timer_stop_loops: 0 5 4 5 2 3 5 7 3 3 1 0 2 0 0 0 0 1 2 1 0 0 0 0 0 0 0 0 0 0 0 0
# uptime
 5:12PM  up 3 days,  7:58, 2 users, load averages: 9.67, 9.64, 9.86

• Rename the flag.
• Check absence of flag before setting.

Some profiling on how often and how long the looping happens. With this patch:

Add a handler for callout_stop() failure and a comment explaining what's going on.

Just a fractional micropercent optimization suggestion. First set the VNET context, then enter the epoch. Same at the end: first exit the epoch, then restore VNET pointer. The VNET context set on a thread for extra time doesn't have any negative impact. The epoch prolonged for extra time does.

Has been committed 9eb0e8326d0fe73ae947959c1df327238d3b2d53

Yes, you are right. It is impossible to create raceless self-freeing callout with two callouts embedded into the same structure. This revision superseded by D37321.

Note: this version will fire KASSERT at certain conditions, as it is incorrect. However, even with correct assert this version has potential use after free, that is hidden by the use of SMR memory for tcpcbs. Right now this revision is not targeted to cgit, it is posted just to share code.

I'm also putting that on tomorrows TCP talk agenda with tuexen, rscheff and others.

May I ask for more time? I just found out that the only reason the TIME_WAIT persists correctly with the current code is that callout_drain_async() doesn't really behave as it is documented. Even when it is able to stop callout immediately, it will leave it for a drain at the scheduled time. So, when I was doing the 9c3507f91987b7c8ce3b895a5ee46a1de6bc42f2 change, I was puzzled why I can safely remove all this TIME_WAIT handling from here, as it seems right, but is never executing. Now I realized why it happened so. I'm working towards standard callout KPI for the TCP timers and this will very likely almost revert 9c3507f91987b7c8ce3b895a5ee46a1de6bc42f2 back to original Robert's code. It could be that the TOE problem will go away or it could manifest itself in a different manner.

Looking at 267334. IMHO, we should return the error for invalid message as close as possible to the userland. Don't let the invalid data travel this down.

Can we return unsigned value from ng_get_composite_len() instead?

@jhb can you please share the actual panic message and trace with TOE? Also the suggested change is actually two different changes preventing call tp tcp_usr_disconnect() in TIME-WAIT. Is there two different panic paths?

May I ask for time until Wednesday to think more about this? If it critical for further work on TOE it is fine to just commit as is.

I would prefer to understand and eliminate the scenario when we are calling disconnect on an already disconnected socket, instead of reintroducing this bandaid. Is there any way for me to reproduce it without Chelsio hardware?

This is how it gets through! I should also restore the KASSERT()s in rack/bbr, then.

Was abandoned in favor of D23518

This legacy revision has some metadata legacy from subversion times. The arc tool fails on it. Will reopen this revision with git-arc.

More discussion on the suppression back when it was added: https://lists.freebsd.org/pipermail/freebsd-net/2004-December/006088.html