From b6af86151b5692a946ed7de4d9c6b0d77226f297 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Tue, 23 Aug 2022 22:29:41 -0700 Subject: [PATCH 1/4] Don't delay the release of Lwip UDPEndPoint if there is no pending LwIPReceiveUDPMessage --- src/inet/UDPEndPointImplLwIP.cpp | 43 ++++++++++++++++++++++++++------ src/inet/UDPEndPointImplLwIP.h | 1 + 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/src/inet/UDPEndPointImplLwIP.cpp b/src/inet/UDPEndPointImplLwIP.cpp index 2d54324e606fd7..b28896f87ba23b 100644 --- a/src/inet/UDPEndPointImplLwIP.cpp +++ b/src/inet/UDPEndPointImplLwIP.cpp @@ -252,17 +252,22 @@ void UDPEndPointImplLwIP::CloseImpl() mUDP = nullptr; mLwIPEndPointType = LwIPEndPointType::Unknown; - // In case that there is a UDPEndPointImplLwIP::LwIPReceiveUDPMessage + // If there is a UDPEndPointImplLwIP::LwIPReceiveUDPMessage // event pending in the event queue (SystemLayer::ScheduleLambda), we // schedule a release call to the end of the queue, to ensure that the // queued pointer to UDPEndPointImplLwIP is not dangling. - Retain(); - CHIP_ERROR err = GetSystemLayer().ScheduleLambda([this] { Release(); }); - if (err != CHIP_NO_ERROR) + if (mDelayRelease == true) { - ChipLogError(Inet, "Unable scedule lambda: %" CHIP_ERROR_FORMAT, err.Format()); - // There is nothing we can do here, accept the chance of racing - Release(); + mDelayRelease = false; + + Retain(); + CHIP_ERROR err = GetSystemLayer().ScheduleLambda([this] { Release(); }); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Inet, "Unable scedule lambda: %" CHIP_ERROR_FORMAT, err.Format()); + // There is nothing we can do here, accept the chance of racing + Release(); + } } } @@ -278,6 +283,9 @@ void UDPEndPointImplLwIP::Free() void UDPEndPointImplLwIP::HandleDataReceived(System::PacketBufferHandle && msg, IPPacketInfo * pktInfo) { + // Clear mDelayRelease flag so that we don't need to delay release of current UDPEndPoint. + mDelayRelease = false; + if ((mState == State::kListening) && (OnMessageReceived != nullptr)) { if (pktInfo != nullptr) @@ -390,17 +398,38 @@ void UDPEndPointImplLwIP::LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb pktInfo->SrcPort = port; pktInfo->DestPort = pcb->local_port; +<<<<<<< HEAD CHIP_ERROR err = ep->GetSystemLayer().ScheduleLambda( [ep, p = System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(buf), pktInfo = pktInfo.get()] { ep->HandleDataReceived(System::PacketBufferHandle::Adopt(p), pktInfo); }); +======= + // Set mDelayRelease flag to delay release of this UDP EndPoint until HandleDataReceived is + // called on this UDP EndPoint. + ep->mDelayRelease = true; + + CHIP_ERROR err = ep->GetSystemLayer().ScheduleLambda([ep, p = System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(buf), pktInfo] { + ep->HandleDataReceived(System::PacketBufferHandle::Adopt(p), pktInfo); + }); +>>>>>>> 5c4b712f6 (Don't delay the release of Lwip UDPEndPoint if there is no pending LwIPReceiveUDPMessage) if (err == CHIP_NO_ERROR) { // If ScheduleLambda() succeeded, it has ownership of the buffer, so we need to release it (without freeing it). static_cast(std::move(buf).UnsafeRelease()); +<<<<<<< HEAD // Similarly, ScheduleLambda now has ownership of pktInfo. pktInfo.release(); +======= + } + else + { + ep->mDelayRelease = false; + + // If ScheduleLambda() succeeded, `pktInfo` will be deleted in `HandleDataReceived`. + // Otherwise we delete it here. + Platform::Delete(pktInfo); +>>>>>>> 5c4b712f6 (Don't delay the release of Lwip UDPEndPoint if there is no pending LwIPReceiveUDPMessage) } } diff --git a/src/inet/UDPEndPointImplLwIP.h b/src/inet/UDPEndPointImplLwIP.h index 7bc523c913ff3a..21213c77e976e9 100644 --- a/src/inet/UDPEndPointImplLwIP.h +++ b/src/inet/UDPEndPointImplLwIP.h @@ -61,6 +61,7 @@ class UDPEndPointImplLwIP : public UDPEndPoint, public EndPointStateLwIP static void LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb, struct pbuf * p, const ip_addr_t * addr, u16_t port); udp_pcb * mUDP; // LwIP User datagram protocol (UDP) control block. + bool mDelayRelease = false; }; using UDPEndPointImpl = UDPEndPointImplLwIP; From 22fb20f9b239f810c6caed0cd70191b7a04cd9b3 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Wed, 24 Aug 2022 07:20:11 -0700 Subject: [PATCH 2/4] Use std::atomic_int instead of bool --- src/inet/UDPEndPointImplLwIP.cpp | 27 ++++++++------------------- src/inet/UDPEndPointImplLwIP.h | 2 +- 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/src/inet/UDPEndPointImplLwIP.cpp b/src/inet/UDPEndPointImplLwIP.cpp index b28896f87ba23b..4bfa088423f758 100644 --- a/src/inet/UDPEndPointImplLwIP.cpp +++ b/src/inet/UDPEndPointImplLwIP.cpp @@ -256,10 +256,8 @@ void UDPEndPointImplLwIP::CloseImpl() // event pending in the event queue (SystemLayer::ScheduleLambda), we // schedule a release call to the end of the queue, to ensure that the // queued pointer to UDPEndPointImplLwIP is not dangling. - if (mDelayRelease == true) + if (mDelayReleaseCount == 0) { - mDelayRelease = false; - Retain(); CHIP_ERROR err = GetSystemLayer().ScheduleLambda([this] { Release(); }); if (err != CHIP_NO_ERROR) @@ -283,8 +281,8 @@ void UDPEndPointImplLwIP::Free() void UDPEndPointImplLwIP::HandleDataReceived(System::PacketBufferHandle && msg, IPPacketInfo * pktInfo) { - // Clear mDelayRelease flag so that we don't need to delay release of current UDPEndPoint. - mDelayRelease = false; + // Decrease mDelayReleaseCount so that we don't need to delay release of current UDPEndPoint if it reaches 0. + mDelayReleaseCount--; if ((mState == State::kListening) && (OnMessageReceived != nullptr)) { @@ -398,38 +396,29 @@ void UDPEndPointImplLwIP::LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb pktInfo->SrcPort = port; pktInfo->DestPort = pcb->local_port; -<<<<<<< HEAD + // Increase mDelayReleaseCount to delay release of this UDP EndPoint while the HandleDataReceived call is + // pending on it. + ep->mDelayReleaseCount++; + CHIP_ERROR err = ep->GetSystemLayer().ScheduleLambda( [ep, p = System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(buf), pktInfo = pktInfo.get()] { ep->HandleDataReceived(System::PacketBufferHandle::Adopt(p), pktInfo); }); -======= - // Set mDelayRelease flag to delay release of this UDP EndPoint until HandleDataReceived is - // called on this UDP EndPoint. - ep->mDelayRelease = true; - - CHIP_ERROR err = ep->GetSystemLayer().ScheduleLambda([ep, p = System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(buf), pktInfo] { - ep->HandleDataReceived(System::PacketBufferHandle::Adopt(p), pktInfo); - }); ->>>>>>> 5c4b712f6 (Don't delay the release of Lwip UDPEndPoint if there is no pending LwIPReceiveUDPMessage) if (err == CHIP_NO_ERROR) { // If ScheduleLambda() succeeded, it has ownership of the buffer, so we need to release it (without freeing it). static_cast(std::move(buf).UnsafeRelease()); -<<<<<<< HEAD // Similarly, ScheduleLambda now has ownership of pktInfo. pktInfo.release(); -======= } else { - ep->mDelayRelease = false; + ep->mDelayReleaseCount--; // If ScheduleLambda() succeeded, `pktInfo` will be deleted in `HandleDataReceived`. // Otherwise we delete it here. Platform::Delete(pktInfo); ->>>>>>> 5c4b712f6 (Don't delay the release of Lwip UDPEndPoint if there is no pending LwIPReceiveUDPMessage) } } diff --git a/src/inet/UDPEndPointImplLwIP.h b/src/inet/UDPEndPointImplLwIP.h index 21213c77e976e9..3d8aebe1021f52 100644 --- a/src/inet/UDPEndPointImplLwIP.h +++ b/src/inet/UDPEndPointImplLwIP.h @@ -61,7 +61,7 @@ class UDPEndPointImplLwIP : public UDPEndPoint, public EndPointStateLwIP static void LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb, struct pbuf * p, const ip_addr_t * addr, u16_t port); udp_pcb * mUDP; // LwIP User datagram protocol (UDP) control block. - bool mDelayRelease = false; + std::atomic_int mDelayReleaseCount{ 0 }; }; using UDPEndPointImpl = UDPEndPointImplLwIP; From 07ddb8317333ffdde1c97637392c1ff498a90f4d Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Thu, 25 Aug 2022 12:43:19 -0700 Subject: [PATCH 3/4] Update src/inet/UDPEndPointImplLwIP.cpp Co-authored-by: Zang MingJie --- src/inet/UDPEndPointImplLwIP.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/inet/UDPEndPointImplLwIP.cpp b/src/inet/UDPEndPointImplLwIP.cpp index 4bfa088423f758..26e4bed626ff56 100644 --- a/src/inet/UDPEndPointImplLwIP.cpp +++ b/src/inet/UDPEndPointImplLwIP.cpp @@ -281,9 +281,6 @@ void UDPEndPointImplLwIP::Free() void UDPEndPointImplLwIP::HandleDataReceived(System::PacketBufferHandle && msg, IPPacketInfo * pktInfo) { - // Decrease mDelayReleaseCount so that we don't need to delay release of current UDPEndPoint if it reaches 0. - mDelayReleaseCount--; - if ((mState == State::kListening) && (OnMessageReceived != nullptr)) { if (pktInfo != nullptr) From 5c09abb9b8f64db696cb7138f05d86745798a23a Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Mon, 29 Aug 2022 10:24:35 -0700 Subject: [PATCH 4/4] Update src/inet/UDPEndPointImplLwIP.cpp Co-authored-by: Boris Zbarsky --- src/inet/UDPEndPointImplLwIP.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/inet/UDPEndPointImplLwIP.cpp b/src/inet/UDPEndPointImplLwIP.cpp index 26e4bed626ff56..7b9cc6f46415ee 100644 --- a/src/inet/UDPEndPointImplLwIP.cpp +++ b/src/inet/UDPEndPointImplLwIP.cpp @@ -256,13 +256,13 @@ void UDPEndPointImplLwIP::CloseImpl() // event pending in the event queue (SystemLayer::ScheduleLambda), we // schedule a release call to the end of the queue, to ensure that the // queued pointer to UDPEndPointImplLwIP is not dangling. - if (mDelayReleaseCount == 0) + if (mDelayReleaseCount != 0) { Retain(); CHIP_ERROR err = GetSystemLayer().ScheduleLambda([this] { Release(); }); if (err != CHIP_NO_ERROR) { - ChipLogError(Inet, "Unable scedule lambda: %" CHIP_ERROR_FORMAT, err.Format()); + ChipLogError(Inet, "Unable to schedule lambda: %" CHIP_ERROR_FORMAT, err.Format()); // There is nothing we can do here, accept the chance of racing Release(); } @@ -399,6 +399,7 @@ void UDPEndPointImplLwIP::LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb CHIP_ERROR err = ep->GetSystemLayer().ScheduleLambda( [ep, p = System::LwIPPacketBufferView::UnsafeGetLwIPpbuf(buf), pktInfo = pktInfo.get()] { + ep->mDelayReleaseCount--; ep->HandleDataReceived(System::PacketBufferHandle::Adopt(p), pktInfo); }); @@ -412,10 +413,6 @@ void UDPEndPointImplLwIP::LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb else { ep->mDelayReleaseCount--; - - // If ScheduleLambda() succeeded, `pktInfo` will be deleted in `HandleDataReceived`. - // Otherwise we delete it here. - Platform::Delete(pktInfo); } }