tcp_hpts: refactor the per tcpcb call to either input/output method

[glebius]

Summary

Either input or output return unlocked on failure. Should be no
functional change.

[tuexen]

sys/netinet/tcp_hpts.c

1389–1390

Why don't you need to call INP_WUNLOCK(), if tcp_output() returns a positive error?

[rrs]

sys/netinet/tcp_hpts.c

1387

So what is going to happen if tcp_output() or tfb_do_queued_segments() return a false value indicating that the tcp connection was dropped? Its rare but an example is say you call tcp_output() aka rack_output() and that calls the timer-checks which realize the connection has reached its last timeout.. that drops the connection and I believe will unlock the lock as part of that.

That would cause some badness I think, unless you changed the sources to all non-zero return to make sure we don't get a drop/unlock...

[rrs]

sys/netinet/tcp_hpts.c

1389–1390

You may be right here, have limited access to src code until next week.. but I am pretty sure tcp_output() aka rack_output can send back a -1 indicating that the connection has been dropped... which means the unlock is not a good idea.

A postive return I would have to look at, probably means you should unlock... I can check next week when I return to warmer climates :)

[rrs]

sys/netinet/tcp_hpts.c

1389–1390

Ok having looked at this in a bit more detail. When a negative value is returned it means that the INP has been unlocked. It often means it is destroyed actually but for sure its unlocked. So if you remove this "skip-pacing" jump around the unlock you are going to be unlocking a inp that is already unlocked. If INVARIANT is on thats going to cause a crash.

Now this is rare, it occurs I think when a connection hits multiple timeouts and goes out the error path.

So overall I think this change will break things!

[glebius]

sys/netinet/tcp_hpts.c

1389–1390

Michael is right - the change misses case with positive error code. Randall, the negative is covered correctly - unlock is already done and is not needed.

[glebius]

Negate result of ctf_do_queued_segments() and compare error to >= 0.

[rrs]

Ok after a detailed analysis:

For the rack_output() call

Line 19856 will return -Val to indicate dropped connection, no unlock is needed (timer checking)
Line 19937 will return postive value (or 0) when rack_fast_output() returns said values. A negative value

falls through the rest of the output routine both fast output routines return only 0 or -1 so
this return will always be 0.

Line 21370 will return host unreachable error (postive) and the caller will need to unlock
Line 22389, 22444, 22468 will return the errno that comes from ip/ip6_output() so postive return

needs an unlock.

Line 22594 will see 0 or -1, 0 means all done with output and -1 is a failure (not a lock drop), the

failure will cause it to goto again and it only returns >0 0 so unlock is needed if it returns

Line 22623 final return which should be 0 but could be an error I suppose.. no negative return here so unlock is needed.

Now on the input call ctf_do_queued_segments()

In this function any non-zero return has the locked dropped and all packets cleaned up that were pulled before
the start of the queuing. So no matter if postive or negative you should have no unlock needed.

I believe this code is correct :)

Gleb I had missed your predict true before ...

Helps to be home where I have better access to things :)

[glebius]

Later I'd probably bring the input and output to same KBI, so that both return same signedness error for unlocked return. But this requires a sweeping change over stacks, so later.

[rrs]

In D47925#1095113, @glebius wrote:

Later I'd probably bring the input and output to same KBI, so that both return same signedness error for unlocked return. But this requires a sweeping change over stacks, so later.

Yes it would, in all stacks.. so it would be a big change. It would make sense that always negative means don't unlock and postive or zero means all ok even if there was an error...i.e.
the output path would look like the input :)

But it is a *big* change.