drivers: eswifi: Fix memory buffer allocation in off_read_work #76483
Conversation
|
Hello @LeoBRIANDSmile, and thank you very much for your first pull request to the Zephyr project! |
drivers/wifi/eswifi/eswifi_socket.c
Outdated
There was a problem hiding this comment.
So this means that we always try to allocate the "full" buffer regardless of how much data there is, this could be wasting lot of net_buf's.
Is it possible to figure out how much data there is to read (without actually reading it), and then allocate just that amount?
There was a problem hiding this comment.
Absolutely, I’ll check tomorrow if there is an API to check the size of the pending packet or if we can resize the buffer we have allocated. Thanks !
There was a problem hiding this comment.
Hi, I pushed a new commit to fix the issue of allocation. My strategy is to allocate the buffer with MTU, then read and then resize with the API net_pkt_update_length() so that we allocate the returned length of read function.
There was a problem hiding this comment.
Ok, I think I did a mistake, net_pkt_update_length() only modify length attribute of net_pkt, I'll try to resize the pkt by creating a new pkt to the exact size thanks to read return and then copy the old pkt into the new pkt and then unref the old pkt
|
Can you also double check or confirm that if we discard packet reading (e.g forced allocation failure), we are actually able to retrieve the packet in next/subsequent 'read_work' procedure (and don't fuzz the chip state). |
drivers/wifi/eswifi/eswifi_socket.c
Outdated
There was a problem hiding this comment.
Instead of this, you can call net_pkt_compact that should remove empty net_buf from net_pkt. This should be called after net_pkt_write.
There was a problem hiding this comment.
Is it not better to use net_pkt_trim_buffer() after net_pkt write ? It checks for unused buffers and deallocate them relevantly.
There was a problem hiding this comment.
Trim is ok too, I just forgot it existed
drivers/wifi/eswifi/eswifi_socket.c
Outdated
There was a problem hiding this comment.
I set pkt to NULL in case of non NULL pkt before the do recv callback
drivers/wifi/eswifi/eswifi_socket.c
Outdated
There was a problem hiding this comment.
add pkt_trim in order to unallocate unused buffers
|
Test failed on : tests/net/lib/http_server/tls/net.http.server.tls but locally it works. PR #76528 got the same issue with the same address page fault, is that weird ? |
Currently if we have an allocation failure we are going to the label "done", so we never reach __read_data function in that situation. I assume that it doesn't imply packet loss in next read_work. |
jukkar
left a comment
jukkar
left a comment
There was a problem hiding this comment.
Looks ok, just squash the commits together for the final review.
I think this is fixed by #76580 |
When receiving data over the eswifi module, we currently read the data first, then allocate a buffer, and finally write the data into the buffer. The issue is that if we can't allocate the buffer, the data that was read is lost. To fix this, we should first attempt to allocate the buffer before reading any data. If we can't allocate the buffer, we should not proceed with reading the data. By allocating a buffer with the MTU size, we can read the packet, write it into the allocated buffer and then resize by removing unused allocated buffer with net_pkt_trim_buffer(). Signed-off-by: Léo BRIAND <[email protected]>
LeoBriandFiveO
force-pushed
the
fix_eswifi
branch
from
c5c2657 to
019f78b
Compare
|
Hi @LeoBRIANDSmile! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
|
Hi davidalo, logs show us that we can’t allocate buffer but actually it is
not a failure because we will try to allocate a new buffer the next time we
run off_read_work function (take a look at my commit). So you are true to
trace back this point because it should not be an error message but an
informative message I guess.
…On Thursday, August 15, 2024, davidalo ***@***.***> wrote:
Since this change I'm getting this error:
[00:00:45.112,000] <err> net_pkt: Data buffer (1460) allocation failed.
[00:00:45.112,000] <err> wifi_eswifi: Cannot allocate rx packet
Any clue?
—
Reply to this email directly, view it on GitHub
<#76483 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BHRY7TFNKX6AWY2OD6EOB7LZRTATTAVCNFSM6AAAAABLWLUMEOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJRGQ4TQMBSGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hello @LeoBRIANDSmile . Thanks for the information. I resolved the issue by increasing the net buffers for the allocation, after doing it everything worked as expected. |
6 participants
When receiving data over the eswifi module, we currently read the data first, then allocate a buffer, and finally write the data into the buffer. The issue is that if we can't allocate the buffer, the data that was read is lost. To fix this, we should first attempt to allocate the buffer before reading any data. If we can't allocate the buffer, we should not proceed with reading the data.