net: tcp: Properly queue FIN packets for retransmission

[pfalcon]

net: tcp: Properly queue FIN packets for retransmission

In TCP protocol, any packet is subject to retransmission if not
ACKed in expected time. Thus, any packet, including FIN (and SYN
for that matter) should be added to the retransmission queue.

In our case, despite its name, queue_fin() function didn't add
FIN packet to rexmit queue, so do that. Then, in
net_tcp_ack_received() which handles ACKs, make sure that we can
handle FIN packets: calculate its sequence number properly, don't
make adhoc adjustments to retransmission logic (it's handled
centrally in restart_timer() already), etc.

Fixes: #8188

Signed-off-by: Paul Sokolovsky <[email protected]>

[codecov-io]

Codecov Report

Merging #9592 into master will increase coverage by 0.16%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #9592      +/-   ##
==========================================
+ Coverage   52.17%   52.33%   +0.16%     
==========================================
  Files         212      212              
  Lines       25914    25922       +8     
  Branches     5583     5585       +2     
==========================================
+ Hits        13520    13567      +47     
+ Misses      10145    10111      -34     
+ Partials     2249     2244       -5

Impacted Files	Coverage Δ
subsys/net/ip/tcp.c	61.46% <81.81%> (+3.22%)	⬆️
subsys/net/ip/net_if.c	61.99% <0%> (+0.27%)	⬆️
subsys/net/ip/net_pkt.c	63.55% <0%> (+0.49%)	⬆️
subsys/net/ip/ipv4.c	73.43% <0%> (+3.12%)	⬆️
drivers/net/loopback.c	84% <0%> (+12%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8d5ec3...ade160c. Read the comment docs.

[jukkar]

Please protect sent_list using a mutex here and in other places in this file.

[pfalcon]

Please protect sent_list using a mutex here

Yes, but why? As I told in #8674 , we should consider various strategies of protecting against concurrent access, and choose the best one (and that may be different best strategies for protecting various data).

Here, as the code comment says, net_tcp_queue_pkt() is the sole point of where something gets added to sent_list, and that's the only operation which can happen based on user API request, i.e. which may happen from the main thread, which is preemptive thread. All other accesses happen only from delayed work handlers, which run in cooperative thread, so can't be preempted themselves. So, we need to protect only this addition to list, all other access to sent_list are already serialized among each other.

Next point is that sys_slist_append() is very lightweight, maybe 3-5 cycles. So, if we just disable IRQs with irq_lock(), that's the max interrupt jitter we introduce. Mutex lock/unlock operations for sure include irq_lock()/irq_unlock() too, so they for sure introduce latency too. In other words, with mutexes, we don't win on latency, but surely lose on overall complexity and performance.

and in other places in this file.

Well, that's certainly out of scope of this patch. It's also not needed, if the above analysis holds. If you don't buy my arguments right away, then I'd rather remove any attempts at protection of sent_list from this patch, and resubmit as a separate patch afterwards, so we can have focused discussion on it.

[jukkar]

Lets remove the locking then from this PR and think this more in another PR. There is also #8674 that deals with net_context locking and is somewhat related to this one.

[pfalcon]

Lets remove the locking then from this PR

Ok, doing.

[jukkar]

Please "open" these added lines a bit and add empty lines, now it looks very crowded.

[pfalcon]

Done

[pfalcon]

Removed locking based on the discussion (the commit message was also updated).

[jukkar]

LGTM