From ca97cf7a0f35ed844299aef44a684c763a0136c2 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Tue, 10 Oct 2023 12:14:33 -0700 Subject: [PATCH] [ip6-mpl] simplify `Mpl::HandleRetransmissionTimer()` (#9509) This commit contains smaller changes in `HandleRetransmissionTimer()`: - Remove nested if/else blocks and use `continue`. - We clone the MPL message if more retx are needed, otherwise use the `message` directly. - In both cases, we now use the same code path for preparing and sending the `message`, avoiding repeated code. - Rename `GetTimerExpirations() to `DetermineMaxRetransmissions()`. --- src/core/net/ip6_mpl.cpp | 101 ++++++++++++++++++++------------------- src/core/net/ip6_mpl.hpp | 6 +-- 2 files changed, 55 insertions(+), 52 deletions(-) diff --git a/src/core/net/ip6_mpl.cpp b/src/core/net/ip6_mpl.cpp index c5dd2d3e5..70a37c211 100644 --- a/src/core/net/ip6_mpl.cpp +++ b/src/core/net/ip6_mpl.cpp @@ -311,9 +311,9 @@ void Mpl::HandleTimeTick(void) #if OPENTHREAD_FTD -uint8_t Mpl::GetTimerExpirations(void) const +uint8_t Mpl::DetermineMaxRetransmissions(void) const { - uint8_t timerExpirations = 0; + uint8_t maxRetx = 0; switch (Get().GetRole()) { @@ -322,16 +322,16 @@ uint8_t Mpl::GetTimerExpirations(void) const break; case Mle::kRoleChild: - timerExpirations = kChildTimerExpirations; + maxRetx = kChildRetransmissions; break; case Mle::kRoleRouter: case Mle::kRoleLeader: - timerExpirations = kRouterTimerExpirations; + maxRetx = kRouterRetransmissions; break; } - return timerExpirations; + return maxRetx; } void Mpl::AddBufferedMessage(Message &aMessage, uint16_t aSeedId, uint8_t aSequence, bool aIsOutbound) @@ -349,7 +349,7 @@ void Mpl::AddBufferedMessage(Message &aMessage, uint16_t aSeedId, uint8_t aSeque interval = kDataMessageInterval; #endif - VerifyOrExit(GetTimerExpirations() > 0); + VerifyOrExit(DetermineMaxRetransmissions() > 0); VerifyOrExit((messageCopy = aMessage.Clone()) != nullptr, error = kErrorNoBufs); if (!aIsOutbound) @@ -378,66 +378,69 @@ void Mpl::HandleRetransmissionTimer(void) { TimeMilli now = TimerMilli::GetNow(); TimeMilli nextTime = now.GetDistantFuture(); - Metadata metadata; for (Message &message : mBufferedMessageSet) { + Metadata metadata; + Message *messageCopy; + uint8_t maxRetx; + metadata.ReadFrom(message); if (now < metadata.mTransmissionTime) { nextTime = Min(nextTime, metadata.mTransmissionTime); + continue; + } + + metadata.mTransmissionCount++; + + maxRetx = DetermineMaxRetransmissions(); + + if (metadata.mTransmissionCount > maxRetx) + { + // If the number of tx already exceeds the limit, remove + // the message. This situation can potentially happen on + // a device role change, which then updates the max MPL + // retx. + + mBufferedMessageSet.DequeueAndFree(message); + continue; + } + + if (metadata.mTransmissionCount < maxRetx) + { + metadata.GenerateNextTransmissionTime(now, kDataMessageInterval); + metadata.UpdateIn(message); + + nextTime = Min(nextTime, metadata.mTransmissionTime); + + messageCopy = message.Clone(); } else { - uint8_t timerExpirations = GetTimerExpirations(); + // This is the last retx of message, we can use the + // `message` directly. - // Update the number of transmission timer expirations. - metadata.mTransmissionCount++; + mBufferedMessageSet.Dequeue(message); + messageCopy = &message; + } - if (metadata.mTransmissionCount < timerExpirations) + if (messageCopy != nullptr) + { + if (metadata.mTransmissionCount > 1) { - Message *messageCopy = message.Clone(message.GetLength() - sizeof(Metadata)); + // Mark all transmissions after the first one as "MPL + // retx". This is used to decide whether to send this + // message to the device's sleepy children. - if (messageCopy != nullptr) - { - if (metadata.mTransmissionCount > 1) - { - messageCopy->SetSubType(Message::kSubTypeMplRetransmission); - } - - messageCopy->SetLoopbackToHostAllowed(true); - messageCopy->SetOrigin(Message::kOriginHostTrusted); - Get().EnqueueDatagram(*messageCopy); - } - - metadata.GenerateNextTransmissionTime(now, kDataMessageInterval); - metadata.UpdateIn(message); - - nextTime = Min(nextTime, metadata.mTransmissionTime); + messageCopy->SetSubType(Message::kSubTypeMplRetransmission); } - else - { - mBufferedMessageSet.Dequeue(message); - if (metadata.mTransmissionCount == timerExpirations) - { - if (metadata.mTransmissionCount > 1) - { - message.SetSubType(Message::kSubTypeMplRetransmission); - } - - metadata.RemoveFrom(message); - message.SetLoopbackToHostAllowed(true); - message.SetOrigin(Message::kOriginHostTrusted); - Get().EnqueueDatagram(message); - } - else - { - // Stop retransmitting if the number of timer expirations is already exceeded. - message.Free(); - } - } + metadata.RemoveFrom(*messageCopy); + messageCopy->SetLoopbackToHostAllowed(true); + messageCopy->SetOrigin(Message::kOriginHostTrusted); + Get().EnqueueDatagram(*messageCopy); } } diff --git a/src/core/net/ip6_mpl.hpp b/src/core/net/ip6_mpl.hpp index 65f5be40d..8ad9ed453 100644 --- a/src/core/net/ip6_mpl.hpp +++ b/src/core/net/ip6_mpl.hpp @@ -234,8 +234,8 @@ private: uint8_t mSequence; #if OPENTHREAD_FTD - static constexpr uint8_t kChildTimerExpirations = 0; // MPL retransmissions for Children. - static constexpr uint8_t kRouterTimerExpirations = 2; // MPL retransmissions for Routers. + static constexpr uint8_t kChildRetransmissions = 0; // MPL retransmissions for Children. + static constexpr uint8_t kRouterRetransmissions = 2; // MPL retransmissions for Routers. struct Metadata { @@ -252,7 +252,7 @@ private: uint8_t mIntervalOffset; }; - uint8_t GetTimerExpirations(void) const; + uint8_t DetermineMaxRetransmissions(void) const; void HandleRetransmissionTimer(void); void AddBufferedMessage(Message &aMessage, uint16_t aSeedId, uint8_t aSequence, bool aIsOutbound);