Skip to content
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

ipc_service: nocopy tx buffer allocation works unexpectedly with RPMSG backend #45934

Closed
KAGA164 opened this issue May 24, 2022 · 3 comments · Fixed by #46242
Closed

ipc_service: nocopy tx buffer allocation works unexpectedly with RPMSG backend #45934

KAGA164 opened this issue May 24, 2022 · 3 comments · Fixed by #46242
Assignees
Labels
area: IPC Inter-Process Communication Feature Request A request for a new feature

Comments

@KAGA164
Copy link
Collaborator

KAGA164 commented May 24, 2022

Describe the bug
Target platform: nrf5340dk_nrf5340
Application: Internal tests that uses ipc_service with RPMSG backend.

int ipc_service_get_tx_buffer(struct ipc_ept *ept, void **data, uint32_t *size, k_timeout_t wait) can return after 15 seconds with NULL pointer when timeout is set to K_FOREVER when there is not possibility to allocates buffer due to shared memory being full. I would expect that this function should wait forever for free place for buffer if shared memory is full and function is called with K_FOREVER parameter.

The second issue is that the ipc_service_get_tx_buffer can only allocates buffer with fixed size of RPMSG_BUFFER_SIZE - sizeof(struct rpmsg_hdr) and currently there is not option other than adding build time definition to set RPMSG_BUFFER_SIZE to size different than 512 bytes. It would be good to have this parameter configurable at least from Kconfig during build time but the perfect solution would be to add possibility to allocate buffer which could contain a few continuous block of memory to fit data bigger than RPMSG_BUFFER_SIZE because sometimes we need to allocate buffer just for few bytes but in other case it can be over 1Kb.

To Reproduce
Run any application that uses IPC with RPMSG backend and try to pass more data that can fit into a shared memory. For example buffers can be hold on the Rx remote side without being fired. When the shared memory will be full then allocation of the next Tx buffer will exit after 15 seconds setting the data pointer to the NULL.

Expected behavior

  1. int ipc_service_get_tx_buffer(struct ipc_ept *ept, void **data, uint32_t *size, k_timeout_t wait) should wait forever for free place in shared memory when wait parameter is set to K_FOREVER.
  2. Maximum requested Tx buffer size should be configurable at least from the Kconfig. The perfect solutions would be buffer allocation where the place for data could be composed of several contiguous memory blocks if data doesn't fit into single block.

Impact
Implementation that use int ipc_service_get_tx_buffer(struct ipc_ept *ept, void **data, uint32_t *size, k_timeout_t wait) with wait time set to forever may failed unexpectedly if this function returns with NULL pointer due to shared memory being full.

It is not possible to sent more data than RPMSG_BUFFER_SIZE - sizeof(struct rpmsg_hdr) through IPC service with RPMSG when nocopy mechanism is used.

@KAGA164 KAGA164 added bug The issue is a bug, or the PR is fixing a bug area: IPC Inter-Process Communication labels May 24, 2022
@carlocaione carlocaione added Feature Request A request for a new feature and removed bug The issue is a bug, or the PR is fixing a bug labels May 24, 2022
@carlocaione
Copy link
Collaborator

int ipc_service_get_tx_buffer(struct ipc_ept *ept, void **data, uint32_t *size, k_timeout_t wait) can return after 15 seconds with NULL pointer when timeout is set to K_FOREVER when there is not possibility to allocates buffer due to shared memory being full. I would expect that this function should wait forever for free place for buffer if shared memory is full and function is called with K_FOREVER parameter.

This is actually due to OpenAMP implementation, not really depending on Zephyr:
https://github.com/OpenAMP/open-amp/blob/9e30ada7e97e64ab8d322270fd68808b821e996c/lib/rpmsg/rpmsg_virtio.c#L22-L23

So you probably want to open an issue there for this.

The second issue is that the ipc_service_get_tx_buffer can only allocates buffer with fixed size of RPMSG_BUFFER_SIZE - sizeof(struct rpmsg_hdr) and currently there is not option other than adding build time definition to set RPMSG_BUFFER_SIZE to size different than 512 bytes. It would be good to have this parameter configurable at least from Kconfig during build time

Yes, this is something we can do leveraging OpenAMP/open-amp@6cb75b8 buth this is manly a feature request and not a bug.

@KAGA164
Copy link
Collaborator Author

KAGA164 commented May 24, 2022

In case of returning with K_FOREVER. On the remote side the ipc_service_get_tx_buffer can even return without waiting any time here:


if the shared memory is full the get_tx_buffer_size returns 0 as a buffer size. So I would consider it as a bug since I would expect to wait for available buffer forever but instead of this on remote side I return immediately with ENOMEM error and theipc_service_get_tx_buffer return 0 as a max buffer size but on the master side it waits maximum 15 seconds for a free buffer.

I can agree that second issue can be considered as a feature.

@carlocaione
Copy link
Collaborator

In case of returning with K_FOREVER. On the remote side the ipc_service_get_tx_buffer can even return without waiting any time here:

if the shared memory is full the get_tx_buffer_size returns 0 as a buffer size. So I would consider it as a bug since I would expect to wait for available buffer forever but instead of this on remote side I return immediately with ENOMEM error and theipc_service_get_tx_buffer return 0 as a max buffer size but on the master side it waits maximum 15 seconds for a free buffer.
I can agree that second issue can be considered as a feature.

I see that there is an asymmetry between remote and host about this. I'll try to see if we can fix this without involving OpenAMP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: IPC Inter-Process Communication Feature Request A request for a new feature
Projects
None yet
2 participants