From 6d84735e155c3a38d9aa2d4b0a0da89b6e8ab2a1 Mon Sep 17 00:00:00 2001 From: david gauchard Date: Wed, 4 Mar 2020 00:40:45 +0100 Subject: [PATCH 1/2] udp: fix again pbuf management --- .../ESP8266WiFi/src/include/UdpContext.h | 67 ++++++++++++++----- 1 file changed, 49 insertions(+), 18 deletions(-) diff --git a/libraries/ESP8266WiFi/src/include/UdpContext.h b/libraries/ESP8266WiFi/src/include/UdpContext.h index 2bd1782e52..c35c1cd33e 100644 --- a/libraries/ESP8266WiFi/src/include/UdpContext.h +++ b/libraries/ESP8266WiFi/src/include/UdpContext.h @@ -202,6 +202,30 @@ class UdpContext _on_rx = handler; } +#ifdef DEBUG_ESP_CORE + // this helper is ready to be used when debugging UDP + void printChain (const pbuf* pb, const char* msg) const + { + // printf the pb pbuf chain, bufferred and all at once + char buf[128]; + int l = snprintf(buf, sizeof(buf), "UDP: %s: ", msg); + while (pb) + { + l += snprintf(&buf[l], sizeof(buf) -l, "%p(H=%d,%d<=%d)-", + pb, pb->flags == PBUF_HELPER_FLAG, pb->len, pb->tot_len); + pb = pb->next; + } + l += snprintf(&buf[l], sizeof(buf) - l, "(end)"); + DEBUGV("%s\n", buf); + } +#else + void printChain (const pbuf* pb, const char* msg) const + { + (void)pb; + (void)msg; + } +#endif + size_t getSize() const { if (!_rx_buf) @@ -262,38 +286,44 @@ class UdpContext return true; } + // We have interleaved informations on addresses within received pbuf chain: + // (before ipv6 code we had: (data-pbuf) -> (data-pbuf) -> (data-pbuf) -> ... in the receiving order) + // Now: (address-info-pbuf -> chained-data-pbuf [-> chained-data-pbuf...]) -> + // (chained-address-info-pbuf -> chained-data-pbuf [-> chained...]) -> ... + // _rx_buf is currently adressing a data pbuf, + // in this function it is going to be discarded. + auto deleteme = _rx_buf; - while(_rx_buf->len != _rx_buf->tot_len) + // forward in the chain until next address-info pbuf or end of chain + while(_rx_buf && _rx_buf->flags != PBUF_HELPER_FLAG) _rx_buf = _rx_buf->next; - _rx_buf = _rx_buf->next; - if (_rx_buf) { - if (_rx_buf->flags == PBUF_HELPER_FLAG) - { - // we have interleaved informations on addresses within reception pbuf chain: - // before: (data-pbuf) -> (data-pbuf) -> (data-pbuf) -> ... in the receiving order - // now: (address-info-pbuf -> data-pbuf) -> (address-info-pbuf -> data-pbuf) -> ... + assert(_rx_buf->flags == PBUF_HELPER_FLAG); - // so the first rx_buf contains an address helper, - // copy it to "current address" - auto helper = (AddrHelper*)PBUF_ALIGNER(_rx_buf->payload); - _currentAddr = *helper; + // copy address helper to "current address" + auto helper = (AddrHelper*)PBUF_ALIGNER(_rx_buf->payload); + _currentAddr = *helper; - // destroy the helper in the about-to-be-released pbuf - helper->~AddrHelper(); + // destroy the helper in the about-to-be-released pbuf + helper->~AddrHelper(); - // forward in rx_buf list, next one is effective data - // current (not ref'ed) one will be pbuf_free'd with deleteme - _rx_buf = _rx_buf->next; - } + // forward in rx_buf list, next one is effective data + // current (not ref'ed) one will be pbuf_free'd + // with the 'deleteme' pointer above + _rx_buf = _rx_buf->next; // this rx_buf is not nullptr by construction, + assert(_rx_buf); // ref'ing it to prevent release from the below pbuf_free(deleteme) + // (ref counter prevents release and will be decreased by pbuf_free) pbuf_ref(_rx_buf); } + + // release in chain previous data, and if any: + // current helper, but not start of current data pbuf_free(deleteme); _rx_buf_offset = 0; @@ -488,6 +518,7 @@ class UdpContext return; } } + #if LWIP_VERSION_MAJOR == 1 #define TEMPDSTADDR (¤t_iphdr_dest) #define TEMPINPUTNETIF (current_netif) From 869a13a5655f5cdd06abfcddee96dac7b03e0043 Mon Sep 17 00:00:00 2001 From: David Gauchard Date: Wed, 4 Mar 2020 11:24:04 +0100 Subject: [PATCH 2/2] Process rx buffer size, cache it, because _rx_buf->tot_len is *not* the total size --- .../ESP8266WiFi/src/include/UdpContext.h | 36 +++++++++++++------ 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/libraries/ESP8266WiFi/src/include/UdpContext.h b/libraries/ESP8266WiFi/src/include/UdpContext.h index c35c1cd33e..8534c61f11 100644 --- a/libraries/ESP8266WiFi/src/include/UdpContext.h +++ b/libraries/ESP8266WiFi/src/include/UdpContext.h @@ -47,6 +47,7 @@ class UdpContext , _rx_buf(0) , _first_buf_taken(false) , _rx_buf_offset(0) + , _rx_buf_size(0) , _refcnt(0) , _tx_buf_head(0) , _tx_buf_cur(0) @@ -74,6 +75,7 @@ class UdpContext pbuf_free(_rx_buf); _rx_buf = 0; _rx_buf_offset = 0; + _rx_buf_size = 0; } } @@ -204,11 +206,11 @@ class UdpContext #ifdef DEBUG_ESP_CORE // this helper is ready to be used when debugging UDP - void printChain (const pbuf* pb, const char* msg) const + void printChain (const pbuf* pb, const char* msg, size_t n) const { // printf the pb pbuf chain, bufferred and all at once char buf[128]; - int l = snprintf(buf, sizeof(buf), "UDP: %s: ", msg); + int l = snprintf(buf, sizeof(buf), "UDP: %s %u: ", msg, n); while (pb) { l += snprintf(&buf[l], sizeof(buf) -l, "%p(H=%d,%d<=%d)-", @@ -231,7 +233,7 @@ class UdpContext if (!_rx_buf) return 0; - return _rx_buf->tot_len - _rx_buf_offset; + return _rx_buf_size - _rx_buf_offset; } size_t tell() const @@ -246,7 +248,7 @@ class UdpContext } bool isValidOffset(const size_t pos) const { - return (pos <= _rx_buf->tot_len); + return (pos <= _rx_buf_size); } netif* getInputNetif() const @@ -327,12 +329,13 @@ class UdpContext pbuf_free(deleteme); _rx_buf_offset = 0; + _rx_buf_size = _processSize(_rx_buf); return _rx_buf != nullptr; } int read() { - if (!_rx_buf || _rx_buf_offset >= _rx_buf->tot_len) + if (!_rx_buf || _rx_buf_offset >= _rx_buf_size) return -1; char c = pbuf_get_at(_rx_buf, _rx_buf_offset); @@ -345,9 +348,9 @@ class UdpContext if (!_rx_buf) return 0; - size_t max_size = _rx_buf->tot_len - _rx_buf_offset; + size_t max_size = _rx_buf_size - _rx_buf_offset; size = (size < max_size) ? size : max_size; - DEBUGV(":urd %d, %d, %d\r\n", size, _rx_buf->tot_len, _rx_buf_offset); + DEBUGV(":urd %d, %d, %d\r\n", size, _rx_buf_size, _rx_buf_offset); void* buf = pbuf_get_contiguous(_rx_buf, dst, size, size, _rx_buf_offset); if(!buf) @@ -363,7 +366,7 @@ class UdpContext int peek() const { - if (!_rx_buf || _rx_buf_offset == _rx_buf->tot_len) + if (!_rx_buf || _rx_buf_offset == _rx_buf_size) return -1; return pbuf_get_at(_rx_buf, _rx_buf_offset); @@ -375,7 +378,7 @@ class UdpContext if (!_rx_buf) return; - _consume(_rx_buf->tot_len - _rx_buf_offset); + _consume(_rx_buf_size - _rx_buf_offset); } size_t append(const char* data, size_t size) @@ -459,6 +462,14 @@ class UdpContext private: + size_t _processSize (const pbuf* pb) + { + size_t ret = 0; + for (; pb && pb->flags != PBUF_HELPER_FLAG; pb = pb->next) + ret += pb->len; + return ret; + } + void _reserve(size_t size) { const size_t pbuf_unit_size = 128; @@ -496,8 +507,8 @@ class UdpContext void _consume(size_t size) { _rx_buf_offset += size; - if (_rx_buf_offset > _rx_buf->tot_len) { - _rx_buf_offset = _rx_buf->tot_len; + if (_rx_buf_offset > _rx_buf_size) { + _rx_buf_offset = _rx_buf_size; } } @@ -506,6 +517,7 @@ class UdpContext { (void) upcb; // check receive pbuf chain depth + // optimization path: cache the pbuf chain length { pbuf* p; int count = 0; @@ -572,6 +584,7 @@ class UdpContext _first_buf_taken = false; _rx_buf = pb; _rx_buf_offset = 0; + _rx_buf_size = pb->tot_len; } if (_on_rx) { @@ -679,6 +692,7 @@ class UdpContext pbuf* _rx_buf; bool _first_buf_taken; size_t _rx_buf_offset; + size_t _rx_buf_size; int _refcnt; pbuf* _tx_buf_head; pbuf* _tx_buf_cur;