Handle out-of-buf cases gracefully in net

[jukkar]

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.

[codecov-io]

Codecov Report

Merging #9118 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            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

Impacted Files	Coverage Δ
drivers/ethernet/eth_native_posix_adapt.c	20.4% <0%> (-19.22%)	⬇️
lib/posix/pthread_key.c	75.92% <0%> (-5.56%)	⬇️
subsys/net/ip/icmpv6.c	30.31% <0%> (-4.24%)	⬇️
kernel/mempool.c	84.61% <0%> (-3.85%)	⬇️
kernel/alert.c	96.42% <0%> (-3.58%)	⬇️
include/misc/slist.h	86.66% <0%> (-3.34%)	⬇️
subsys/net/ip/icmpv4.c	35.03% <0%> (-3.31%)	⬇️
drivers/ethernet/eth_native_posix.c	20.86% <0%> (-3.14%)	⬇️
kernel/stack.c	92.3% <0%> (-1.93%)	⬇️
kernel/mem_slab.c	98.07% <0%> (-1.93%)	⬇️
... and 35 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b9d980...f236d18. Read the comment docs.

[jukkar]

New version with L2 and network lib directories handled

[jukkar]

recheck

[pfl]

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?

[jukkar]

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.

[pfalcon]

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()".

[jukkar]

New version that fixes merge conflict.

[pfalcon]

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)".

[jukkar]

Fixed compile error and clarified the function documentation in ipv6.h

[pfalcon]

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).

[jukkar]

rebased to latest master

[pfalcon]

I tested this on master 3712fd3 against issues described in #7831 and #3132, it doesn't help with it - there's a still imminent request processing delay after processing some (~100) number of requests.

[pfalcon]

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.

[pfalcon]

@pfl: Please review discussion of the issues you raised in #9118 (comment) , and see if you like any suggestions there.

[rveerama1]

Just a side note, we still need to modify net_pkt_write_xx() helper functions. Those are still using K_FOREVER.

[jukkar]

Changed net_pkt_write_xx() and net_pkt_insert_xx() to use the timeout.

[jukkar]

Fixed compile error in rpl.c

[jukkar]

@pfl ping, can you re-visit this one?

[jukkar]

Resolved merge conflict.