Do not use pvconn argument to get the TCP connection pointer because pvconn is
normally NULL for some events like NETDEV_DOWN. Instead, the TCP connection pointer
can be reliably obtained from the corresponding TCP socket.
Both the snd_ackcb and snd_datacb callbacks were created and destroyed right after sending every packet.
Whenever TCP_REXMIT event occurred due to TCP send timeout, TCP_REXMIT was ignored because
snd_ackcb callback had been destroyed by the time.
The issue is fixed as follows:
- both the snd_ackcb and snd_datacb callbacks are combined into one snd_cb callback
(the same way as in tcp_send_unbuffered.c).
- the snd_cb callback lives until all requested data (via sendfile) is sent,
including all ACKs and possible retransmissions.
As a positive side effect of the code optimization / fix, sendfile TCP payload throughput is increased.
tcp_sendfile() reads data directly from a file and does not use NET_TCP_WRITE_BUFFERS data flow
even if CONFIG_NET_TCP_WRITE_BUFFERS option is enabled.
Despite this, tcp_sendfile relied on NET_TCP_WRITE_BUFFERS specific flow control variables that
were idle during sendfile operation. Thus it was a total inconsistency.
E.g. because of the issue, TCP socket used by sendfile() operation never issued
FIN packet on close() command, and the TCP connection hung up.
As a result of the fix, simultaneously enabled CONFIG_NET_TCP_WRITE_BUFFERS and
CONFIG_NET_SENDFILE options can coexist.
Wrong unackseq calculation locked conn->tx_unacked at non-zero values
even if all ACKs were received. Thus unbuffered psock_tcp_send() never completed.
If the remote TCP receiver advertised TCP window size greater than 64 KB
and TCP ACK packets returned to the NuttX TCP sender with a significant delay,
tx_unacked variable overflowed and further TCP send stalled forever
(until TCP re-connection).
While it's a neat idea, it doesn't work well in reality.
* Many of modern tcp stacks don't obey the "ack every other packet"
rule these days. (Linux, macOS, ...)
* Even if a traditional TCP implementation is assumed, we can't
predict/control which packets are acked reliably. For example,
window updates can easily mess up our strategy.
tcp_timer: eliminated false decrements of conn->timer in case of multiple network adapters.
The false timer decrements sometimes provoked TCP spurious retransmissions due to premature timeouts.
In case of enabled packet forwarding mode, packets were forwarded in a reverse order
because of LIFO behavior of the connection event list.
The issue exposed only during high network traffic. Thus the event list started to grow
that resulted in changing the order of packets inside of groups of several packets
like the following: 3, 2, 1, 6, 5, 4, 8, 7 etc.
Remarks concerning the connection event list implementation:
* Now the queue (list) is FIFO as it should be.
* The list is singly linked.
* The list has a head pointer (inside of outer net_driver_s structure),
and a tail pointer is added into outer net_driver_s structure.
* The list item is devif_callback_s structure.
It still has two pointers to two different list chains (*nxtconn and *nxtdev).
* As before the first argument (*dev) of the list functions can be NULL,
while the other argument (*list) is effective (not NULL).
* An extra (*tail) argument is added to devif_callback_alloc()
and devif_conn_callback_free() functions.
* devif_callback_alloc() time complexity is O(1) (i.e. O(n) to fill the whole list).
* devif_callback_free() time complexity is O(n) (i.e. O(n^2) to empty the whole list).
* devif_conn_event() time complexity is O(n).
Gregory Nutt has submitted the SGA
UVC Ingenieure has submitted the SGA
Max Holtzberg has submitted the ICLA
as a result we can migrate the licenses to Apache.
Signed-off-by: Alin Jerpelea <alin.jerpelea@sony.com>
* Do not accept the window in old segments.
Implement SND.WL1/WL2 things in the RFC.
* Do not accept the window in the segment w/o ACK bit set.
The window is an offset from the ack seq.
(maybe it's simpler to just drop segments w/o ACK though)
* Subtract snd_wnd by the amount of the ack advancement.
Fix a wrong assertion in:
```
commit 98ec46d726
Author: YAMAMOTO Takashi <yamamoto@midokura.com>
Date: Tue Jul 20 09:10:43 2021 +0900
tcp_send_buffered.c: fix iob allocation deadlock
Ensure to put the wrb back onto the write_q when blocking
on iob allocation. Otherwise, it can deadlock with other
threads doing the same thing.
```
I forget to submit this with https://github.com/apache/incubator-nuttx/pull/4257
With an applictation using mbedtls, I observed retransmitted segments
with corrupted user data, detected by the peer tls during mac processing.
Looking at the packet dump, I suspect that a wrb which has been put back
onto the write_q for retransmission was partially sent but fully acked.
Note: it's normal for a retransmission to be acked before sent.
In that case, the bug fixed in this commit would cause the wrb have
a wrong sequence number, possibly the same as the next wrb. It matches
what I saw in the packet dump. That is, the broken segments contain the
payload identical to one of the previous segment.
Consider a bi-directional TCP connection:
1. we use all IOBs for tx queue
2. we advertize zero recv window because we have no free IOBs
3. if the peer tcp does the same thing,
both sides advertize zero window and can not drain the tx queue.
For a similar stall to happen, the peer doesn't need to be
a naive tcp implementation like nuttx. A naive application blocking
on send() without draining its read buffer is enough.
(Probably such an application should be fixed to drain rx even
when tx is full. However, it's another story.)
This commit avoids the situation by prevent tx from grabbing
the all IOBs in the first place. (assuming CONFIG_IOB_THROTTLE > 0)
Since we do not have the Nagle's algorithm,
the TCP_NODELAY socket option is enabled by default.
Change-Id: I0c8619bb06cf418f7eded5bd72ac512b349cacc5
Signed-off-by: chao.an <anchao@xiaomi.com>
Since a SOL option IP_TTL exist, we should rename this IP_TTL
in netconfig.h to avoid confusion.
Signed-off-by: Huang Qi <huangqi3@xiaomi.com>
Change-Id: Ib04c36553f23bce8d362e97294a8b83eaa050cf3
quote from https://man7.org/linux/man-pages/man2/sendfile.2.html:
If offset is not NULL, then it points to a variable holding the
file offset from which sendfile() will start reading data from
in_fd. When sendfile() returns, this variable will be set to the
offset of the byte following the last byte that was read. If
offset is not NULL, then sendfile() does not modify the file
offset of in_fd; otherwise the file offset is adjusted to reflect
the number of bytes read from in_fd.
If offset is NULL, then data will be read from in_fd starting at
the file offset, and the file offset will be updated by the call.
The change also align with the implementation at:
libs/libc/misc/lib_sendfile.c
Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Change-Id: I607944f40b04f76731af7b205dcd319b0637fa04
1. change all window relative value type to uint32_t
2. move window range validity check(UINT16_MAX) before assembling TCP header
Signed-off-by: chao.an <anchao@xiaomi.com>
tcp_close disposes the connection immediately if it's called in
TCP_LAST_ACK. If it happens, we will end up with responding the
last ACK with a RST.
This commit fixes it by making tcp_close wait for the completion
of the passive close.
This fixes connection closing issues with CONFIG_NET_TCP_WRITE_BUFFERS.
Because TCP_CLOSE is used for both of input and output for tcp_callback,
the close callback and the send callback confuses each other as
the following. As it effectively disposes the connection immediately,
we end up with responding to the consequent ACK and FIN/ACK from the peer
with RSTs.
tcp_timer
-> tcp_close_eventhandler
returns TCP_CLOSE (meaning an active close)
-> psock_send_eventhandler
called with TCP_CLOSE from tcp_close_eventhandler, misinterpet as
a passive close.
-> tcp_lost_connection
-> tcp_shutdown_monitor
-> tcp_callback
-> tcp_close_eventhandler
misinterpret TCP_CLOSE from itself as
a passive close
The current code just leave the window value from the segment
from the peer. It doesn't make sense.
Instead, always use 0.
This matches what NetBSD and Linux do.
(As far as I read their code correctly.)
* It doesn't make sense to have this conditional on our own
SO_KEEPALIVE support. (CONFIG_NET_TCP_KEEPALIVE)
Actually we don't have a control on the peer tcp stack,
who decides to send us keep-alive probes.
* We should respond them for non ESTABLISHED states. eg. FIN_WAIT_2
See also:
https://github.com/apache/incubator-nuttx/pull/3919#issuecomment-868248576
Do not bother to preserve segment boundaries in the tcp
readahead queues.
* Avoid wasting the tail IOB space for each segments.
Instead, pack the newly received data into the tail space
of the last IOB. Also, advertise the tail space as
a part of the window.
* Use IOB chain directly. Eliminate IOB queue overhead.
* Allow to accept only a part of a segment.
* This change improves the memory efficiency.
And probably more importantly, allows less-confusing
recv window advertisement behavior.
Previously, even when we advertise N bytes window,
we often couldn't actually accept N bytes. Depending on
the segment sizes and IOB configurations, it was causing
segment drops.
Also, the previous code was moving the right edge of the
window back and forth too often, even when nothing in
the system was competing on the IOBs. Shrinking the
window that way is a kinda well known recipe to confuse
the peer stack.
