Skip to content

drivers: eswifi: Fix memory buffer allocation in off_read_work #76483

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 2, 2024
Merged

drivers: eswifi: Fix memory buffer allocation in off_read_work #76483

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions drivers/wifi/eswifi/eswifi_socket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,33 +133,40 @@ static void eswifi_off_read_work(struct k_work *work)

__select_socket(eswifi, socket->index);

/* Verify if we can allocate a rx packet before reading data to prevent leaks */
pkt = net_pkt_rx_alloc_with_buffer(eswifi->iface, 1460,
Copy link
Member

@jukkar jukkar Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@LeoBriandFiveO LeoBriandFiveO Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !

Copy link
Contributor Author

@LeoBriandFiveO LeoBriandFiveO Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@LeoBriandFiveO LeoBriandFiveO Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

AF_UNSPEC, 0, K_NO_WAIT);
if (!pkt) {
LOG_ERR("Cannot allocate rx packet");
goto done;
}

len = __read_data(eswifi, 1460, &data); /* 1460 is max size */
if (len < 0) {
__stop_socket(eswifi, socket);

if (socket->recv_cb) {
/* send EOF (null pkt) */
net_pkt_unref(pkt);
pkt = NULL;
Copy link
Contributor Author

@LeoBriandFiveO LeoBriandFiveO Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set pkt to NULL in case of non NULL pkt before the do recv callback

goto do_recv_cb;
}
}

if (!len || !socket->recv_cb) {
net_pkt_unref(pkt);
goto done;
}

LOG_DBG("payload sz = %d", len);

pkt = net_pkt_rx_alloc_with_buffer(eswifi->iface, len,
AF_UNSPEC, 0, K_NO_WAIT);
if (!pkt) {
LOG_ERR("Cannot allocate rx packet");
goto done;
}

if (net_pkt_write(pkt, data, len) < 0) {
LOG_WRN("Incomplete buffer copy");
}

/* Resize the packet */
net_pkt_trim_buffer(pkt);
Copy link
Contributor Author

@LeoBriandFiveO LeoBriandFiveO Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add pkt_trim in order to unallocate unused buffers


net_pkt_cursor_init(pkt);

do_recv_cb:
Expand Down