-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[message-pool] evict message from coap pending request queue if send queue is empty #9984
base: main
Are you sure you want to change the base?
Conversation
Size Report of OpenThread
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks @sarveshkumarv3.
Some suggestions below:
1c1ee8f
to
e3beca9
Compare
Thinking more this PR and #9949, some aspects to consider: TMF Eviction Impact: This PR allows any message (regardless of priority) to possibly evict a TMF agent request. Since TMF messages are generally high priority themselves, should we add checks for such evictions (e.g., based on priority)? Investigating Address Query/Notification Overload: From #9949, this change aims to handle situations where a device receives many address queries, leading to frequent TMF address notifications. It may help to delve deeper into this and understand it better (address the root cause):
|
4250361
to
63ce30c
Compare
thanks for your inputs@abtink! The eviction ordering is to first evict low priority messages in send queue, high priority messages in send queue and only if that is empty to evict TMF agent request right? Also we evict from the head of the request queue, which means TMF agent messages are consuming lot of messages and one at the head is already in the buffers for a long time (In the log, I see around 26s to go through all the retransmission attempts)? Please let me know if anything else can be prioritized ahead of TMF agent messages? The other thought is MLE queue or MPL queues. The reassembly queues anyway have a limit of 2s after which it would be discarded?
Yes, I agree, from the given logs, it is not clear why we see so frequent address queries from the accessory (with no visibility here), but I suspect there is some underlying matter stack related issue, causing multiple accessories to go into a crash-loop (running out of buffers) and hence do not handle the AddressNotify. The changes here would still help to mitigate the Border Router being able to receive from other accessories (protecting its RX buffers) in case we encounter scenario of misbehaving accessories? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sarveshkumarv3 for submitting the PR and all the updates LGTM.
Couple of smaller suggestions below.
On this:
Also we evict from the head of the request queue, which means TMF agent messages are consuming lot of messages and one at the head is already in the buffers for a long time (In the log, I see around 26s to go through all the retransmission attempts)?
I cannot think of any good way to use the priority when deciding to evict from TMF agent queue. So the approach in the PR sounds reaonable.
…queue is empty Co-authored-by: Abtin Keshavarzian <abtink@google.com>
52b9b1c
to
5710ebb
Compare
thanks for the inputs @abtink, incorporated the changes now |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9984 +/- ##
===========================================
+ Coverage 68.31% 82.32% +14.01%
===========================================
Files 494 549 +55
Lines 57933 68748 +10815
===========================================
+ Hits 39577 56600 +17023
+ Misses 18356 12148 -6208
|
If the send queue is empty, currently the messages are not evicted and this could cause received messages to be dropped when too many pending address query notifications. The changes made are to evict the message from head of the coap pending request queue if send queue is empty and new buffer cannot be allocated