[mesh-forwarder] refine direct tx finalization and message removal (#13679)

This commit updates `FinalizeMessageDirectTx()` (now renamed to
`FinalizeDirectTxAndRemoveMessageIfNoPendingTx()`) to clear
`mSendMessage` and `mMessageNextOffset` when finalizing a message's
direct transmission, and then call `RemoveMessageIfNoPendingTx()`.

Previously, `mSendMessage` was only cleared inside
`RemoveMessageIfNoPendingTx()` when the message had neither direct nor
indirect transmissions remaining. If `mSendMessage` was scheduled for
both direct and indirect transmission, finalizing its direct
transmission alone (e.g., due to queue limit eviction) left
`mSendMessage` pointing to the message even though its direct
transmission flag was cleared. When the in-flight MAC transmission
subsequently completed, `UpdateSendMessage()` would hit
`OT_ASSERT(mSendMessage->IsDirectTransmission())`.

This commit also updates `ApplyDirectTxQueueLimit()` to return `Error`
and call `FinalizeDirectTxAndRemoveMessageIfNoPendingTx()` when
dropping a newly queued message, allowing `SendMessage()` to exit
immediately without accessing a potentially freed message.
This commit is contained in:
Abtin Keshavarzian
2026-10-07 08:45:20 -07:00
committed by Jonathan Hui
parent c2645ec507
commit 33667b284a
3 changed files with 50 additions and 61 deletions
+43 -51
View File
@@ -153,7 +153,7 @@ void MeshForwarder::SendMessage(OwnedPtr<Message> aMessagePtr)
}
#if (OPENTHREAD_CONFIG_MAX_FRAMES_IN_DIRECT_TX_QUEUE > 0)
ApplyDirectTxQueueLimit(message);
SuccessOrExit(ApplyDirectTxQueueLimit(message));
#endif
if (message.IsDirectTransmission())
@@ -325,8 +325,7 @@ exit:
mTxQueueStats.UpdateFor(aMessage);
#endif
LogMessage(kMessageQueueMgmtDrop, aMessage);
FinalizeMessageDirectTx(aMessage, kErrorDrop);
RemoveMessageIfNoPendingTx(aMessage);
FinalizeDirectTxAndRemoveMessageIfNoPendingTx(aMessage, kErrorDrop);
}
return error;
@@ -425,43 +424,41 @@ bool MeshForwarder::IsDirectTxQueueOverMaxFrameThreshold(void) const
return (frameCount > OPENTHREAD_CONFIG_MAX_FRAMES_IN_DIRECT_TX_QUEUE);
}
void MeshForwarder::ApplyDirectTxQueueLimit(Message &aMessage)
Error MeshForwarder::ApplyDirectTxQueueLimit(Message &aMessage)
{
Error error;
Error error = kErrorNone;
bool originalEvictFlag;
VerifyOrExit(aMessage.IsDirectTransmission());
VerifyOrExit(IsDirectTxQueueOverMaxFrameThreshold());
// We mark the "do not evict" flag on the new `aMessage` so
// that it will not be removed in `EvictMessage()` call.
// This protects against the unlikely case where the newly
// queued `aMessage` may already be aged due to execution
// being interrupted for a long time between the queuing of
// the message and the `ApplyDirectTxQueueLimit()` call. We
// do not want the message to be potentially removed and
// freed twice.
originalEvictFlag = aMessage.GetDoNotEvict();
aMessage.SetDoNotEvict(true);
do
while (IsDirectTxQueueOverMaxFrameThreshold())
{
// We mark the "do not evict" flag on the new `aMessage` so
// that it will not be removed from `RemoveAgedMessages()`.
// This protects against the unlikely case where the newly
// queued `aMessage` may already be aged due to execution
// being interrupted for a long time between the queuing of
// the message and the `ApplyDirectTxQueueLimit()` call. We
// do not want the message to be potentially removed and
// freed twice.
aMessage.SetDoNotEvict(true);
error = EvictMessage(aMessage.GetPriority(), kEvictReasonDirectTxQueueAtLimit);
aMessage.SetDoNotEvict(originalEvictFlag);
if (error == kErrorNone)
if (EvictMessage(aMessage.GetPriority(), kEvictReasonDirectTxQueueAtLimit) != kErrorNone)
{
VerifyOrExit(IsDirectTxQueueOverMaxFrameThreshold());
aMessage.SetDoNotEvict(originalEvictFlag);
error = kErrorDrop;
LogMessage(kMessageFullQueueDrop, aMessage);
FinalizeDirectTxAndRemoveMessageIfNoPendingTx(aMessage, error);
ExitNow();
}
}
} while (error == kErrorNone);
LogMessage(kMessageFullQueueDrop, aMessage);
FinalizeMessageDirectTx(aMessage, kErrorDrop);
aMessage.SetDoNotEvict(originalEvictFlag);
exit:
return;
return error;
}
#endif // (OPENTHREAD_CONFIG_MAX_FRAMES_IN_DIRECT_TX_QUEUE > 0)
@@ -590,8 +587,7 @@ Message *MeshForwarder::PrepareNextDirectTransmission(void)
mTxQueueStats.UpdateFor(*curMessage);
#endif
LogMessage(kMessageDrop, *curMessage, error);
FinalizeMessageDirectTx(*curMessage, error);
RemoveMessageIfNoPendingTx(*curMessage);
FinalizeDirectTxAndRemoveMessageIfNoPendingTx(*curMessage, error);
continue;
}
}
@@ -971,20 +967,20 @@ void MeshForwarder::UpdateSendMessage(Error aFrameTxError, Mac::Address &aMacDes
#endif
LogMessage(kMessageTransmit, *mSendMessage, txError, &aMacDest);
FinalizeMessageDirectTx(*mSendMessage, txError);
RemoveMessageIfNoPendingTx(*mSendMessage);
FinalizeDirectTxAndRemoveMessageIfNoPendingTx(*mSendMessage, txError);
exit:
mScheduleTransmissionTask.Post();
}
void MeshForwarder::FinalizeMessageDirectTx(Message &aMessage, Error aError)
void MeshForwarder::FinalizeDirectTxAndRemoveMessageIfNoPendingTx(Message &aMessage, Error aError)
{
// Finalizes the direct transmission of `aMessage`. This can be
// triggered by successful delivery (all fragments reaching the
// destination), failure of any fragment, queue management
// dropping the message, or eviction of message to accommodate
// higher priority messages.
// Finalizes the direct transmission of `aMessage` and removes it
// from the send queue if it has no remaining pending transmissions
// (direct or indirect). This can be triggered by successful
// delivery (all fragments reaching the destination), failure of
// any fragment, queue management dropping the message, or eviction
// of the message to accommodate higher priority messages.
VerifyOrExit(aMessage.IsDirectTransmission());
@@ -998,10 +994,16 @@ void MeshForwarder::FinalizeMessageDirectTx(Message &aMessage, Error aError)
mCounters.UpdateOnTxDone(aMessage, aMessage.GetTxSuccess());
if (mSendMessage == &aMessage)
{
mSendMessage = nullptr;
mMessageNextOffset = 0;
}
aMessage.InvokeTxCallback(aError);
exit:
return;
RemoveMessageIfNoPendingTx(aMessage);
}
void MeshForwarder::FinalizeAndRemoveMessage(Message &aMessage, Error aError, MessageAction aAction)
@@ -1012,31 +1014,21 @@ void MeshForwarder::FinalizeAndRemoveMessage(Message &aMessage, Error aError, Me
FinalizeMessageIndirectTxs(aMessage);
#endif
FinalizeMessageDirectTx(aMessage, aError);
RemoveMessageIfNoPendingTx(aMessage);
FinalizeDirectTxAndRemoveMessageIfNoPendingTx(aMessage, aError);
}
bool MeshForwarder::RemoveMessageIfNoPendingTx(Message &aMessage)
void MeshForwarder::RemoveMessageIfNoPendingTx(Message &aMessage)
{
bool didRemove = false;
#if OPENTHREAD_FTD
VerifyOrExit(!aMessage.IsDirectTransmission() && !aMessage.IsIndirectTransmission());
#else
VerifyOrExit(!aMessage.IsDirectTransmission());
#endif
if (mSendMessage == &aMessage)
{
mSendMessage = nullptr;
mMessageNextOffset = 0;
}
mSendQueue.DequeueAndFree(aMessage);
didRemove = true;
exit:
return didRemove;
return;
}
Error MeshForwarder::RxInfo::ParseIp6Headers(void)
+4 -4
View File
@@ -445,8 +445,8 @@ private:
Error RemoveAgedMessages(void);
#endif
#if (OPENTHREAD_CONFIG_MAX_FRAMES_IN_DIRECT_TX_QUEUE > 0)
bool IsDirectTxQueueOverMaxFrameThreshold(void) const;
void ApplyDirectTxQueueLimit(Message &aMessage);
bool IsDirectTxQueueOverMaxFrameThreshold(void) const;
Error ApplyDirectTxQueueLimit(Message &aMessage);
#endif
void SendDestinationUnreachable(uint16_t aMeshSource, const Ip6::Headers &aIp6Headers);
Error UpdateIp6Route(Message &aMessage);
@@ -471,9 +471,9 @@ private:
void UpdateNeighborLinkFailures(Neighbor &aNeighbor, Error aError, bool aAllowNeighborRemove, uint8_t aFailLimit);
void HandleFrameTxDone(Mac::TxFrame::ParseInfo &aFrameInfo, Error aError);
void UpdateSendMessage(Error aFrameTxError, Mac::Address &aMacDest, Neighbor *aNeighbor);
void FinalizeMessageDirectTx(Message &aMessage, Error aError);
void FinalizeDirectTxAndRemoveMessageIfNoPendingTx(Message &aMessage, Error aError);
void FinalizeAndRemoveMessage(Message &aMessage, Error aError, MessageAction aAction);
bool RemoveMessageIfNoPendingTx(Message &aMessage);
void RemoveMessageIfNoPendingTx(Message &aMessage);
void HandleTimeTick(void);
void ScheduleTransmissionTask(void);
+3 -6
View File
@@ -137,8 +137,7 @@ void MeshForwarder::HandleResolved(const Ip6::Address &aEid, Error aError)
if (aError != kErrorNone)
{
LogMessage(kMessageDrop, message, kErrorAddressQuery);
FinalizeMessageDirectTx(message, kErrorAddressQuery);
RemoveMessageIfNoPendingTx(message);
FinalizeDirectTxAndRemoveMessageIfNoPendingTx(message, kErrorAddressQuery);
continue;
}
@@ -181,8 +180,7 @@ Error MeshForwarder::EvictMessage(Message::Priority aPriority, EvictReason aEvic
{
case kEvictReasonDirectTxQueueAtLimit:
LogMessage(kMessageFullQueueEvict, *evict, kErrorDrop);
FinalizeMessageDirectTx(*evict, kErrorDrop);
RemoveMessageIfNoPendingTx(*evict);
FinalizeDirectTxAndRemoveMessageIfNoPendingTx(*evict, kErrorDrop);
break;
case kEvictReasonNoMessageBuffer:
@@ -270,8 +268,7 @@ void MeshForwarder::RemoveMessagesForChild(Child &aChild, Message::Checker aChec
if (matches)
{
FinalizeMessageDirectTx(message, kErrorDrop);
RemoveMessageIfNoPendingTx(message);
FinalizeDirectTxAndRemoveMessageIfNoPendingTx(message, kErrorDrop);
}
}
}