-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Handle out-of-buf cases gracefully in net #9118
Handle out-of-buf cases gracefully in net #9118
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9118 +/- ##
==========================================
- Coverage 52.53% 52.52% -0.01%
==========================================
Files 212 201 -11
Lines 25821 25359 -462
Branches 5428 5299 -129
==========================================
- Hits 13565 13320 -245
+ Misses 10071 9944 -127
+ Partials 2185 2095 -90
Continue to review full report at Codecov.
|
a7f8a0e
to
7e0a978
Compare
New version with L2 and network lib directories handled |
7e0a978
to
c3c0a7c
Compare
recheck |
if (!net_pkt_append_##type##_timeout(pkt, value, \ | ||
BUF_ALLOC_TIMEOUT)) { \ | ||
ret = -ENOMEM; \ | ||
goto drop; \ |
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.
That goto is a bit of a trap as it is encoded in the #define. As the code is specific to this file, instead of a goto drop cleanup could be performed. Or the caller could free all allocations on error. In this code it is simple, all drops just return the error, so a return -ENOMEM can be safely returned here?
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.
I tried to make the define to do "the right thing" meaning that all the functions would have error return at the end of the function. The defines are there so that the code looks a bit nicer and we do not have so many exposed error checks in the middle of the functions. I am not really sure I follow how would you like to see the code to look like in this case.
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.
I'm on @jukkar's side here, in the sense that I find such code acceptable and "attempting-to-do-better". It's not just a single case (like dns/llmnr_responder.c to which @pfl originally commented), but a pattern reused in different files. It's effectively a poorman's exception handling handling, implemented in C.
Well, I can see @pfl's point too, that for someone not familiar with the code (or to someone who didn't look at the code for a couple of months ;-)), it may be confusing - after all, C is the language without exception handling, and it may be not immediately obvious that some functions/macros have a "side effect" of "throwing" an exception (goto'ing drop in this case). A compromise may be naming this macros e.g. as "append_or_drop()".
c3c0a7c
to
d7b5b97
Compare
New version that fixes merge conflict. |
subsys/net/ip/ipv6.h
Outdated
@@ -134,7 +134,8 @@ int net_ipv6_send_na(struct net_if *iface, const struct in6_addr *src, | |||
* @param iface Network interface | |||
* @param next_header_proto Protocol type of the next header after IPv6 header. | |||
* | |||
* @return Return network packet that contains the IPv6 packet. | |||
* @return Return network packet that contains the IPv6 packet or NULL if |
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.
If you edit this description, maybe makes sense to make it less cryptic at the same time. If this function returns its input "pkt" param, makes sense to say so, and then in parens add "(which now contains IPv6 header)".
d7b5b97
to
b9d8c2a
Compare
Fixed compile error and clarified the function documentation in ipv6.h |
This patch definitely moves in the right direction, though I wonder if it may have not immediately obvious outcomes (i.e. further refactors/changes may be required as follow-ups). Before +1'ing this, I'd like to test this with my usual smoke tests (which may take some time, as I'm just back from vacation and going thru backlog of things happened during this time). |
b9d8c2a
to
3c8b9f5
Compare
rebased to latest master |
I then proceeded to test with my 2nd standard testcase - samples/net/sockets/big_http_download. With both dumb_http_server and big_http_download samples, I didn't see any objective regressions. And that's the whole point. I didn't see any improvements either on the issues I tested for, and that only proves that we have many different issues. I think we should merge this (because it's clearly a paradigmatic improvement), and move on to fight next issues. |
@pfl: Please review discussion of the issues you raised in #9118 (comment) , and see if you like any suggestions there. |
Just a side note, we still need to modify net_pkt_write_xx() helper functions. Those are still using K_FOREVER. |
3c8b9f5
to
c5e1295
Compare
Changed net_pkt_write_xx() and net_pkt_insert_xx() to use the timeout. |
c5e1295
to
26abd42
Compare
Fixed compile error in rpl.c |
@pfl ping, can you re-visit this one? |
Instead of waiting forever for a network buffer, have a timeout when allocating net_buf. This way we cannot left hanging for a long time waiting for a buffer and possibly deadlock the system. This commit only adds checks to core IP stack in subsys/net/ip Fixes zephyrproject-rtos#7571 Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
Instead of waiting forever for a network buffer, have a timeout when allocating net_buf. This way we cannot left hanging for a long time waiting for a buffer and possibly deadlock the system. This commit adds checks to L2 and network support libraries. Fixes zephyrproject-rtos#7571 Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
Do not wait forever for network buffers when receiving data. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
Do not wait forever for network buffers when receiving data. Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
26abd42
to
f236d18
Compare
Resolved merge conflict. |
This removes K_FOREVER calls from network buffer allocations so it is not possible to "hang" IP stack because of no network buffers available. Tested with echo-server running in frdm-k64f and flooding it from Linux using net-tools echo-client program. The system did not manage to receive much packets but it did not hang.