From 54fa0f1836fa8f5e27b8a11cba42be8f9587acda Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Fri, 13 Oct 2017 09:29:18 -0700 Subject: [PATCH] [ncp] enhance handling of outbound IP message over spinel (to host) (#2259) This commit changes how the outbound datagram IPv6 messages (going from NCP to host over spinel) are handled. If NCP spinel buffer is full, new outbound messages are saved in a message queue and are transfered later (as soon as buffer space becomes available). This ensures that outbound IPv6 messages are never dropped due to spinel NCP buffer being full. To realize this, the following changes are made in this commit: (a) The behavior of methods `NcpFrameBuffer::InFrameFeedMessage()` and `SpinelEncoder::WriteMessage()` is changed so that the passed-in message ownership changes to NCP buffer ONLY when the entire spinel frame is written and the frame is successfully ended/finished. If the spinel frame gets discarded (e.g. no buffer space) the caller continues to own the message instance and should either free or save it. (b) The unit test `test_ncp_buffer` is changed to address and check for this change. (c) The `NcpBase::HandleDatagramFromStack()` is modified to implement/use a message queue to save outbound messages and try again to send the queued messages from `HandleFrameRemovedFromNcpBuffer()` callback (when spinel buffer becomes available). --- src/ncp/ncp_base.cpp | 5 ++ src/ncp/ncp_base.hpp | 5 ++ src/ncp/ncp_base_ftd.cpp | 13 +++-- src/ncp/ncp_base_mtd.cpp | 87 ++++++++++++++++++++-------------- src/ncp/ncp_buffer.cpp | 11 +++-- src/ncp/ncp_buffer.hpp | 7 ++- src/ncp/spinel_encoder.hpp | 7 ++- tests/unit/test_ncp_buffer.cpp | 4 ++ 8 files changed, 90 insertions(+), 49 deletions(-) diff --git a/src/ncp/ncp_base.cpp b/src/ncp/ncp_base.cpp index 768dc82c8..482dff3bf 100644 --- a/src/ncp/ncp_base.cpp +++ b/src/ncp/ncp_base.cpp @@ -554,6 +554,7 @@ NcpBase::NcpBase(otInstance *aInstance): mTxFrameBuffer.SetFrameRemovedCallback(&NcpBase::HandleFrameRemovedFromNcpBuffer, this); #if OPENTHREAD_MTD || OPENTHREAD_FTD + otMessageQueueInit(&mMessageQueue); otSetStateChangedCallback(mInstance, &NcpBase::HandleNetifStateChanged, this); otIp6SetReceiveCallback(mInstance, &NcpBase::HandleDatagramFromStack, this); otIp6SetReceiveFilterEnabled(mInstance, true); @@ -711,6 +712,10 @@ void NcpBase::HandleFrameRemovedFromNcpBuffer(NcpFrameBuffer::FrameTag aFrameTag } } +#if OPENTHREAD_MTD || OPENTHREAD_FTD + SuccessOrExit(SendQueuedDatagramMessages()); +#endif + UpdateChangedProps(); exit: diff --git a/src/ncp/ncp_base.hpp b/src/ncp/ncp_base.hpp index 4c55b7935..7ac5948c9 100644 --- a/src/ncp/ncp_base.hpp +++ b/src/ncp/ncp_base.hpp @@ -219,6 +219,9 @@ private: static void HandleDatagramFromStack(otMessage *aMessage, void *aContext); void HandleDatagramFromStack(otMessage *aMessage); + otError SendQueuedDatagramMessages(void); + otError SendDatagramMessage(otMessage *aMessage); + static void HandleActiveScanResult_Jump(otActiveScanResult *aResult, void *aContext); void HandleActiveScanResult(otActiveScanResult *aResult); @@ -649,6 +652,8 @@ private: #endif // OPENTHREAD_ENABLE_RAW_LINK_API #if OPENTHREAD_MTD || OPENTHREAD_FTD + otMessageQueue mMessageQueue; + uint32_t mInboundSecureIpFrameCounter; // Number of secure inbound data/IP frames. uint32_t mInboundInsecureIpFrameCounter; // Number of insecure inbound data/IP frames. uint32_t mOutboundSecureIpFrameCounter; // Number of secure outbound data/IP frames. diff --git a/src/ncp/ncp_base_ftd.cpp b/src/ncp/ncp_base_ftd.cpp index 83c7efe0a..eb0a6f748 100644 --- a/src/ncp/ncp_base_ftd.cpp +++ b/src/ncp/ncp_base_ftd.cpp @@ -470,17 +470,16 @@ void NcpBase::HandleTmfProxyStream(otMessage *aMessage, uint16_t aLocator, uint1 SuccessOrExit(error = mEncoder.WriteUint16(length)); SuccessOrExit(error = mEncoder.WriteMessage(aMessage)); - // Set the `aMessage` pointer to NULL to indicate that it does - // not need to be freed at the exit. The `aMessage` is now owned - // by the outbound frame and will be freed when the frame is either - // successfully sent and then removed, or if the frame gets - // discarded. - aMessage = NULL; - SuccessOrExit(error = mEncoder.WriteUint16(aLocator)); SuccessOrExit(error = mEncoder.WriteUint16(aPort)); SuccessOrExit(error = mEncoder.EndFrame()); + // The `aMessage` is owned by the outbound frame and NCP buffer + // after frame was finished/ended successfully. It will be freed + // when the frame is successfully sent and removed. + + aMessage = NULL; + exit: if (aMessage != NULL) diff --git a/src/ncp/ncp_base_mtd.cpp b/src/ncp/ncp_base_mtd.cpp index 1fef0240b..2188441c5 100644 --- a/src/ncp/ncp_base_mtd.cpp +++ b/src/ncp/ncp_base_mtd.cpp @@ -2549,58 +2549,76 @@ void NcpBase::HandleDatagramFromStack(otMessage *aMessage, void *aContext) } void NcpBase::HandleDatagramFromStack(otMessage *aMessage) +{ + VerifyOrExit(aMessage != NULL); + + SuccessOrExit(otMessageQueueEnqueue(&mMessageQueue, aMessage)); + SuccessOrExit(SendQueuedDatagramMessages()); + +exit: + // If the queued message can not be sent now (out of buffer), + // it will be sent once spinel buffer becomes available from + // `HandleFrameRemovedFromNcpBuffer()` callback. + + return; +} + +otError NcpBase::SendDatagramMessage(otMessage *aMessage) { otError error = OT_ERROR_NONE; uint8_t header = SPINEL_HEADER_FLAG | SPINEL_HEADER_IID_0; bool isSecure = otMessageIsLinkSecurityEnabled(aMessage); - uint16_t length = otMessageGetLength(aMessage); + spinel_prop_key_t propKey = isSecure ? SPINEL_PROP_STREAM_NET : SPINEL_PROP_STREAM_NET_INSECURE; - SuccessOrExit(error = mEncoder.BeginFrame( - header, - SPINEL_CMD_PROP_VALUE_IS, - isSecure ? SPINEL_PROP_STREAM_NET : SPINEL_PROP_STREAM_NET_INSECURE - )); - - SuccessOrExit(error = mEncoder.WriteUint16(length)); + SuccessOrExit(error = mEncoder.BeginFrame(header, SPINEL_CMD_PROP_VALUE_IS, propKey)); + SuccessOrExit(error = mEncoder.WriteUint16(otMessageGetLength(aMessage))); SuccessOrExit(error = mEncoder.WriteMessage(aMessage)); - // Set the `aMessage` pointer to NULL to indicate that it does - // not need to be freed at the exit. The `aMessage` is now owned - // by the OutboundFrame and will be freed when the frame is either - // successfully sent and then removed, or if the frame gets - // discarded. - aMessage = NULL; - // Append any metadata (rssi, lqi, channel, etc) here! SuccessOrExit(error = mEncoder.EndFrame()); -exit: - - if (aMessage != NULL) + if (isSecure) { - otMessageFree(aMessage); - } - - if (error != OT_ERROR_NONE) - { - mChangedPropsSet.AddLastStatus(SPINEL_STATUS_DROPPED); - mUpdateChangedPropsTask.Post(); - mDroppedOutboundIpFrameCounter++; + mOutboundSecureIpFrameCounter++; } else { - if (isSecure) - { - mOutboundSecureIpFrameCounter++; - } - else - { - mOutboundInsecureIpFrameCounter++; - } + mOutboundInsecureIpFrameCounter++; } + +exit: + return error; } +otError NcpBase::SendQueuedDatagramMessages(void) +{ + otError error = OT_ERROR_NONE; + otMessage *message; + + while ((message = otMessageQueueGetHead(&mMessageQueue)) != NULL) + { + // Since an `otMessage` instance can be in one queue at a time, + // it is first dequeued from `mMessageQueue` before attempting + // to include it in a spinel frame by calling `SendDatagramMessage()` + // If forming of the spinel frame fails, the message is enqueued + // back at the front of `mMessageQueue`. + + otMessageQueueDequeue(&mMessageQueue, message); + + error = SendDatagramMessage(message); + + if (error != OT_ERROR_NONE) + { + otMessageQueueEnqueueAtHead(&mMessageQueue, message); + } + + SuccessOrExit(error); + } + +exit: + return error; +} // ---------------------------------------------------------------------------- // MARK: Property/Status Changed @@ -2702,7 +2720,6 @@ exit: return; } - } // namespace Ncp } // namespace ot diff --git a/src/ncp/ncp_buffer.cpp b/src/ncp/ncp_buffer.cpp index 49ef17578..69d6d2340 100644 --- a/src/ncp/ncp_buffer.cpp +++ b/src/ncp/ncp_buffer.cpp @@ -90,7 +90,10 @@ void NcpFrameBuffer::Clear(void) while ((message = otMessageQueueGetHead(&mWriteFrameMessageQueue)) != NULL) { otMessageQueueDequeue(&mWriteFrameMessageQueue, message); - otMessageFree(message); + + // Note that messages associated with current (unfinished) input frame + // are not yet owned by the `NcpFrameBuffer` and therefore should not + // be freed. } for (uint8_t priority = 0; priority < kNumPrios; priority++) @@ -304,11 +307,13 @@ void NcpFrameBuffer::InFrameDiscard(void) // Move the write segment head and tail pointers back to frame start. mWriteSegmentHead = mWriteSegmentTail = mWriteFrameStart[mWriteDirection]; - // Free any messages associated with current frame. while ((message = otMessageQueueGetHead(&mWriteFrameMessageQueue)) != NULL) { otMessageQueueDequeue(&mWriteFrameMessageQueue, message); - otMessageFree(message); + + // Note that messages associated with current (unfinished) input frame + // being discarded, are not yet owned by the `NcpFrameBuffer` and + // therefore should not be freed. } mWriteDirection = kUnknown; diff --git a/src/ncp/ncp_buffer.hpp b/src/ncp/ncp_buffer.hpp index 2fdacf4bb..78a42e1a6 100644 --- a/src/ncp/ncp_buffer.hpp +++ b/src/ncp/ncp_buffer.hpp @@ -209,8 +209,11 @@ public: * If no buffer space is available, this method will discard and clear the frame and return error status * `OT_ERROR_NO_BUFS`. * - * In case of success, the passed-in message @p aMessage will be owned by the frame buffer instance and will be - * freed when either the frame is removed or discarded. In case of failure @p aMessage remains unchanged. + * The ownership of the passed-in message @p aMessage changes to `NcpFrameBuffer` ONLY when the entire frame is + * successfully finished (i.e., with a successful call to `InFrameEnd()` for the current input frame), and in this + * case the `otMessage` instance will be freed once the frame is removed (using `OutFrameRemove()`) from NCP buffer. + * However, if the input frame gets discarded before it is finished (e.g., running out of buffer space), the + * `otMessage` instance remains unchanged. * * @param[in] aMessage A message to be added to current frame. * diff --git a/src/ncp/spinel_encoder.hpp b/src/ncp/spinel_encoder.hpp index fe40f80e1..cf1def941 100644 --- a/src/ncp/spinel_encoder.hpp +++ b/src/ncp/spinel_encoder.hpp @@ -508,8 +508,11 @@ public: * If no buffer space is available, this method will discard and clear the frame and return error status * `OT_ERROR_NO_BUFS`. * - * In case of success, the passed-in message @p aMessage will be owned by the frame buffer instance and will be - * freed when either the the frame is removed or discarded. In case of failure @p aMessage remains unchanged. + * The ownership of the passed-in message @p aMessage changes to underlying `NcpFrameBuffer` ONLY when the entire + * frame is successfully finished (i.e., with a successful call to `EndFrame()` for the current frame being written), + * and in this case the `otMessage` instance will be freed once the frame is removed from the `NcpFrameBuffer`. + * However, if the frame gets discarded before it is finished (e.g., running out of buffer space), the `otMessage` + * instance remains unchanged. * * @param[in] aMessage A message to be added to current frame. * diff --git a/tests/unit/test_ncp_buffer.cpp b/tests/unit/test_ncp_buffer.cpp index 092f5be8d..b3ab4eb95 100644 --- a/tests/unit/test_ncp_buffer.cpp +++ b/tests/unit/test_ncp_buffer.cpp @@ -574,6 +574,10 @@ void TestNcpFrameBuffer(void) WriteTestFrame1(ncpBuffer, frame1IsHighPriority ? NcpFrameBuffer::kPriorityHigh : NcpFrameBuffer::kPriorityLow); + // Note that message will not be freed by the NCP buffer since the frame associated with it was discarded and + // not yet finished/ended. + otMessageFree(message); + VerifyAndRemoveFrame3(ncpBuffer); // Start reading few bytes from the frame