In D17910#383510, @vmaffione wrote:

I'm sorry, but as a design decision for netmap we never automatically change interface features/offloading automatically and restore them.
We require the application or the system administrator to do so explicitely.
In any case this must be discussed with the upstream project https://github.com/luigirizzo/netmap

The same feature was added with r357606 revision: We can close this review.

This change is not needed anymore as it has been fixed with r324945

Address @np comment

In D17802#381512, @np wrote:

Can you provide exact steps to reproduce the problem that you described? I get an EAGAIN (as expected) and not a panic if I try to enable netmap when the link is down.

Address @np comment

In D17843#381446, @np wrote:

What version of FreeBSD is this? I think this is already fixed in 12.0.

In D15686#333677, @mmacy wrote:

@jch sounds simple enough to replicate, nonetheless can you share any benchmark source?

In D15686#333661, @mmacy wrote:

In D15686#333660, @rwatson wrote:

[snip]
I think you slightly misread my comment. It was not so much that I am concerned that there is a pathological case in theory (clearly those exist all over the place), it's that in practical use this could shift the watermark on allocated inpcbs substantially in some real-world workloads, and that it would be good to seek some testing from who work with such workloads, which differ substantially from the target workload for this work (as I understand it). In the past, work on who connections/sec support has been primarily driven by the folk at Verisign doing top-level domain hosting, and who have a good test setup for that. I wonder if they'd be willing to give this a spin and in particular look for evidence of impact on connections/sec to make sure that this doesn't lead to a substantive decrease (and ideally instead leads to an increase) in connection throughput.

It seems unlikely that a 2ms backlog of inpcbs could alter the connection rate, vs new connections not blocking on callouts and all the various points that acquire an info read lock. Verisign has not been responsive of late, but if they're willing to test that would be an interesting data point.

Fixed and MFC'ed with rS307551: Fix a double-free when an inp transitions to INP_TIMEWAIT state.

gnn accepted this revision.

Still need someone from -transport to approve this review though. :)

In D8211#171925, @slw_zxy.spb.ru wrote:

I was testing this patch in 3 days.
No TCP related problems.

Review from Slawa:

• log(LOG_ERR) and workaround a difficult to debug case when INVARIANT is not defined
• Add a tcp_twstart KASSERT

Thanks for your time on this review, will push this change as soon as D8211: Fix a double-free when an inp transitions to INP_TIMEWAIT state after having been dropped. is complete.

In D8211#170331, @slw_zxy.spb.ru wrote:

I think imp also have reproduced this issue (https://lists.freebsd.org/pipermail/freebsd-stable/2016-September/085518.html)
Also report Urmas Lett <urmas.lett@eenet.ee> (email to jch)

Fix a comment typo

Currently only @slw_zxy.spb.ru and @girgen have reproduced this issue thus are able to validate this change, I have not been able to reproduce it but our TCP QA is all good with this change.

In D7042#147487, @glebius wrote:

I would not agree that there is API change for callout_stop(). Let's look into the manual page:
...

The problem is that the whole paradigm of callouts is that it has three consequent states: not scheduled -> scheduled -> running -> not scheduled. The API and the manual page assume that, some comments in the code assume that, and looks like some contributors to the code also did. But this paradigm isn't true. A callout can be scheduled and running at the same time. And the mishandling of this state was the source of our TCP panics. Now let's get back to the documentation and see what should callout_stop() return if callout is scheduled and running at the same time. It should return 0 because callout is currently being serviced and can't be stopped. At the same time it should return 1, since it has successfully unscheduled a callout. So it must be 0 and 1 at the same time! :) Yep, which is impossible. The API is limited by wrong paradigm and we must select the most safe value between these two. Now, I assert that returning 1 is much less safe in this case. Because any properly designed user of the API will be ready to free resources that are now in use by running callout. This is exactly what happened with TCP. Any other user, use it async version or not, is also prone to panic if we return 1. I assert that returning 0 will make behaviour more safe, and won't break any users, since this is what a sane programmer would expect from API. To finalize, I would notice that this state of running+scheduled in practice is extremely rare, if we noticed it only now at above 50 Gbit/s. So, I'm strongly against limiting the behavior only to async version.

Looks good to me, let me launch our TCP QA on it.

In D7042#147452, @hselasky wrote:

callout_stop() should return 0 when the callout is currently being serviced and indeed unstoppable.

I think this approach will add extra complications to the code.

I suggest to use:

static void
dummy_drain(void *arg)
{
}

ret = callout_async_drain(co, &dummy_drain, NULL);

Which is basically what you want and allow callout_async_drain() to update the callback function if called multiple times.

Instead of:

ret = callout_stop()

In this case. It returns the correct value for the client.

If callout_stop() always return 0 in the MPSAFE cc_exec_curr == c, it breaks the callout API possibly with out-of-the-tree code which we don't control.

I agree with this change spirit:

Tested on top of -CURRENT r298539 and all good. Performance are the same with and without this patch (this is expected). Green light from me.

I starting testing this callout_async_drain() usage and so far so good. Let me upgrade our lab to a more recent revision of -CURRENT and I will report my findings here (should be by the end of this week).

In D5924#126467, @rrs wrote:

jch: Thanks for running your torture tests.. I did not take out the flags you added though I
am not sure we need them anymore.. what do you think?

Nice to see callout_async_drain() in action, TCP timer code being (much) cleaner. Will review this change, and run our usual TCP torture tests on it.

I approve this server-side TFO implementation, it is a solid first implementation we can build on.

Good example of using callout_drain_async().

I approve this change for MPSAFE callouts. As I lack of specific tests (and knowledge) for non-MPSAFE callouts, I would like another reviewer to look at it (@rrs?).

I have used and tested callout_async_drain() for TCP timer callouts and all good, everything works as expected. I would suggest you to create a review that depends on this one, to show how to use callout_async_drain() as a real example. What I did for my testing:

I am indeed for adding callout_drain_async() as this is exactly what we are doing with TCP timer callouts in HEAD, stable/10 and releng/10.2, but it is currently done using callout_stop()/callout_reset() in a non simple way instead of using directly callout_drain_async().

Let me try to try callout_drain_async() in top of our last TCP changes. I will put the results here.

I do approve the addition of this function, it would have made my life much easier for these two tasks:

Change put back with rS287304: Put r284245 back in place: If at first this fix was seen as a temporary, as it turns out this change is not a workaround for a callout_stop() bug, but the right to use callout_stop()/callout_reset() in mpsafe mode. Should be the last change in the task to remove TCP timers race conditions started with D2079: Fix TCP timers use-after-free old race conditions.

Change reverted in rS287305: Revert r286880: If at first this change made sense, it turns out, thanks for your time sharing the subtlety of callout_stop() in mpsafe mode.

Hi guys, after emails exchanges with @helasky and @kib I believe that if this change makes sense and improves some callout usages (i.e. in TCP timers) it is not worth it overall. In details:

Updated with hselaski inputs