net: context: Call send_cb for TCP after ACK is received

[jukkar]

Change the TCP send_cb call to be done after we have received
ACK to sent packet. It did not make much sense to call the
callback immediately (synchronously) after we sent the net_pkt.

Signed-off-by: Jukka Rissanen [email protected]

[andyross]

No complaints on the code, but you might want to make this configurable. This adds significant code size, won't naturally compile itself out if unused, and is only going to be useful to a small handful of apps. I mean, it's undeniably cool, but Berkeley sockets don't actually provide a way to know when data is acked at all, and the world has survived.

An alternative way to achieve the same feature, FWIW, would be a "bytes_still_bufferd()" API or equivalent that would tell the user how far behind the current write point the latest ACK is. Might be smaller.

[jukkar]

Indeed the BSD sockets do not provide this kind of feature and world has survived. I have a real use case for this feature and it is related to how zephyr network stack works currently.

• The HTTP server library implementation at HTTP server library API #140 closes the TCP connection after serving the request. So after sending the response, the http code in http_server.c unrefs the connection which will send RST to client. This is just fine except that the seq/ack numbers of the sent RST are bogus because we did not wait for the ACK. The connection is terminated but the client will re-send many messages back to zephyr because of this. The motivation for this PR was to fix this packet re-send. Current code in HTTP server library API #140 is not working properly yet in this respect as the response timeout is still 0 and code in this PR net: context: Call send_cb for TCP after ACK is received #172 is not really run by that PR yet.
• Another issue with this PR that I noticed and which will require still more work, is that the received SEQ/ACK values are not yet "saved" when we call the send_cb. Any TCP messages that are sent by send_cb will have their sequence numbers wrong because of this.

[jukkar]

Let's abandon this patch for time being. I was testing this with http-server which disconnects the client after serving the request. This connection tear-down is very fast and causes some packet resends by the client. This is probably acceptable trade off and we avoid bringing extra code that is only handling a rare use case.

[jukkar]

The original code, if we need it later, can be found at jukkar#4

[pfalcon]

I didn't have a chance to look at this before, and so far just quickly skimmed thru the link posted above. I however can share following thought. There can be 2 policies for sending:

1. "High throughput policy": Each sending task (e.g.) thread sends data as fast and as large as possible - i.e., having 100K it allocates a send buffer, writes fitting data into it (not possible without net: net_pkt_append: Take into account MSS/MTU when adding data to a packet #119), then allocates another buffer and repeats, the only thing which stops it (blocks) is unavailability of threads.
2. "Good citizen policy": Each sending task allocates just one buffer, fills it, sends, and waits for "sent" callback to refill and repeat.

Approach no.1 doesn't require "sent" callback at all, but a thread working like that effectively DoSes all other threads (indeed, with few threads working like that we'll likely get a deadlock in the current Zephyr). Approach no.2 requires reliably working "sent" callback, i.e. it indeed should be called once data are truly acked (or fully undeliverable), not after 1st attempt to send data out.

Consequently, I'd argue that this feature is required, though approach to implementation may vary indeed.

[pfalcon]

And - forgot to add that comment - IMHO it doesn't help to close such patches prematurely. I understand there's a concern that such may clog the queue, but some issues can't be resolved from first attempt, and IMHO it's better to keep them around (at least for few weeks) so people can think about them. We seem to have "in progress" label to keep PRs organized (or maybe can add explicit "rfc" or "wip" labels).

[jukkar]

The PR is not lost yet and I can resubmit it after 1.8 so we can ponder this case a bit more.