net: Set net_buf alloc timeout for IPv4 ARP and IPv6 ND #6208
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6208 +/- ##
==========================================
+ Coverage 53.16% 53.16% +<.01%
==========================================
Files 412 412
Lines 40117 40131 +14
Branches 7726 7729 +3
==========================================
+ Hits 21327 21335 +8
- Misses 15653 15662 +9
+ Partials 3137 3134 -3
Continue to review full report at Codecov.
|
subsys/net/ip/ipv6.c
Outdated
There was a problem hiding this comment.
Perhaps this should have a less generic name, but e.g. namespaced one, e.g. ND_NET_BUF_TIMEOUT ?
There was a problem hiding this comment.
Depending on the bearer, use-case, runtime context, it could be wise to let the actual user to set what he needs through Kconfig.
There was a problem hiding this comment.
Having some config option would be nice but we could end up having multiple config options for various timeouts. I would like to avoid that as that would be very confusing to end user.
For the actual timeout value, I think almost every value is equally good. I mean that if the system is short on buffers (which typically means heavily loaded system), then any timeout value would be equally good. On the other hand, if there is a buffer leak, then any timeout value would not change anything.
So I suggest we go with one hard coded value atm and change this if we really need to.
There was a problem hiding this comment.
So I suggest we go with one hard coded value atm and change this if we really need to.
I'm +1 for that. However, I'd still recommend a more distinctive name for such a define.
|
This definitely looks like a step in the right direction, a questions is: how do we handle these -ENOMEM's? |
At the moment, the constructed message is just discarded. This might affect the functionality of the device of course. |
But does the caller at least prints a log message for this (preferrably, ERR-level). I hope, we don't have a silent discarding here. |
pfalcon
left a comment
pfalcon
left a comment
There was a problem hiding this comment.
Thanks for the answers. I'd still recommend checking that -ENOMEM has a good chance to be reported back to user (at least via logging). Otherwise, this is step in the right direction, so let me +1 it.
jukkar
force-pushed
the
bug-5484-arp+nd
branch
from
c25761e to
3f7f3f2
Compare
|
Uploaded new version:
|
Instead of having K_FOREVER when allocating a packet in IPv4 ARP and IPv6 ND, set a timeout so that we do not have a case where we would wait net_buf forever. Fixes zephyrproject-rtos#5484 Signed-off-by: Jukka Rissanen <[email protected]>
jukkar
force-pushed
the
bug-5484-arp+nd
branch
from
3f7f3f2 to
6f0458d
Compare
|
Uploaded version that fixes sanity error. |
pfalcon
left a comment
pfalcon
left a comment
There was a problem hiding this comment.
One thing which can be improved, otherwise, looks good, thanks for the added logging!
| #include <net/arp.h> | ||
| #include "net_private.h" | ||
|
|
||
| #define NET_BUF_TIMEOUT MSEC(100) |
There was a problem hiding this comment.
Ideally, this would be ARP_NET_BUF_TIMEOUT then ;-).
4 participants
Instead of having K_FOREVER when allocating a packet in IPv4 ARP
and IPv6 ND, set a timeout so that we do not have a case where we
would wait net_buf forever.
Fixes #5484
Signed-off-by: Jukka Rissanen [email protected]