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: add dynamically allocated buffers to icmsg #58741
ipc: add dynamically allocated buffers to icmsg #58741
Conversation
9a732f8
to
352b482
Compare
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.
Looks logically good, just grammar.
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.
Please, separate the core changes from the sample ones.
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.
Overall looks promising. I putted a few comments.
|
||
$ west build -b nrf5340dk_nrf5340_cpuapp -- \ | ||
-DDTC_OVERLAY_FILE=boards/nrf5340dk_nrf5340_cpuapp_icmsg_w_buf.overlay \ | ||
-DDTC_OVERLAY_FILE_REMOTE=boards/nrf5340dk_nrf5340_cpunet_icmsg_w_buf.overlay |
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.
Is it work? It seems to me that for child image it should be DTC_OVERLAY_FILE_remote
.
Also I am not sure if this works with pure ZEPHYR
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.
It is not done autoamtically by the build system, so I added handling of this varable in Cmake: https://github.com/zephyrproject-rtos/zephyr/pull/58741/files#diff-1810cb21049c8e65908e16a8b62cb8c03e98484bbdc00c3511c856e8b3f783acR28
I was following naming of the variable that was already there BOARD_REMOTE
*/ | ||
|
||
#include <zephyr/logging/log.h> | ||
LOG_MODULE_REGISTER(ipc_svc_icmsg_w_buf, |
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.
Please place it after all includes, it would look cleaner in my opinion
* - Release bound endpoint | ||
* | MSG_RELEASE_BOUND | block index | | ||
* This message is a response to the "Bound endpoint" message and it is used to inform | ||
* that a specific buffer is not used anymore and a specific endpoint can now receive |
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.
Could you clarify a little which buffer?
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.
It's a buffer that starts at block referenced by block index
field. I will clarify it in the comment.
|
||
/** | ||
* Send bound message on specified endpoint. | ||
* |
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.
dev_data
?
alloc_size = msg_len; | ||
r = alloc_tx_buffer(dev_data, &alloc_size, &buffer, K_FOREVER); | ||
if (r >= 0) { | ||
msg = (struct ept_bound_msg *)buffer; |
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.
Shouldn't struct ept_bound_msg
be packed or does it msg 4 byte aligned?
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.
ept_bound_msg
does not need to be packed or aligned (all fields are bytes), but still is aligned because blocks and buffers are always 4-byte aligned.
size_t i; | ||
int r = 0; | ||
|
||
k_mutex_lock(&dev_data->mutex, K_FOREVER); |
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.
Maybe configurable timeout could be an option?
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 disagree. It is only to avoid access to the same memory by more threads. It is not long running procedure, so in such case, I don't see point of using something different then K_FOREVER.
goto exit; | ||
} | ||
|
||
addr_or_msg_type = message[0]; |
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.
Could we use defines or static const variables to describe indexes here?
return 0; | ||
} | ||
|
||
/** |
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 don't sure if you should use doxygen mark inside source file
* ICMsg is used to send and receive small 2-byte messages. The first byte is an endpoint | ||
* address or a message type and the second is the block index where the relevant buffer | ||
* starts. |
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.
Consider a change:
* ICMsg is used to send and receive small 2-byte messages. The first byte is an endpoint | |
* address or a message type and the second is the block index where the relevant buffer | |
* starts. | |
* ICMsg is used to send and receive small 2-byte messages. The first byte is an endpoint | |
* address or a message type and the second is the block index where the corresponding buffer begins. |
* Calculate pointer to block from its index and channel configuration (RX or TX). | ||
* No validation is performed. | ||
*/ | ||
static struct block_header *block_from_index(const struct channel_config *ch_conf, | ||
size_t block_index) |
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.
Missing description of parameters ch_conf
, block_index
and returned value.
* @retval -EINVAL If requested size is bigger than entire allocable space | ||
* @retval -ENOSPC If timeout was K_NO_WAIT and there was not enough space | ||
* @retval -EAGAIN If timeout occurred |
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.
You usually use a full stop at the end of a sentence.
Also in other places.
* @retval -EINVAL If requested size is bigger than entire allocable space | |
* @retval -ENOSPC If timeout was K_NO_WAIT and there was not enough space | |
* @retval -EAGAIN If timeout occurred | |
* @retval -EINVAL If requested size is bigger than entire allocable space. | |
* @retval -ENOSPC If timeout was K_NO_WAIT and there was not enough space. | |
* @retval -EAGAIN If timeout occurred. |
num_blocks); | ||
return -EINVAL; | ||
} | ||
/* Update actual buffer size and number of block to release. */ |
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.
/* Update actual buffer size and number of block to release. */ | |
/* Update actual buffer size and number of blocks to release. */ |
/** | ||
* Release bound endpoint message received. | ||
*/ |
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.
Please use the doxygen style. It is used elsewhere.
I think the documentation similar to |
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.
Unfortunately, I won't review this fully anytime soon due to other more urgent tasks.
A few potential problems came to my mind while looking through this code:
- Cache management - I see it is implemented
- Keeping memory allocation metadata in the same memory space as the allocated data itself may be considered a security issue. But as long as this is just for point-to-point communication, it's probably fine. Though some security experts could have a look
- Yet another hundreds-lines dynamic memory allocation module in Zephyr could be a huge maintenance cost. I'm curious if one of the existing allocation modules like memory slabs could be used here instead.
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.
Why is new backend implemented instead of using multi endpoint variant? What's the rationale?
Edit:
Can't we use heap implementation from zephyr instead of implementing new allocator? Not sure about the requirements for the backend but i'm happy to hear.
@hubertmis, @emob-nordic, answering to your questions:
The rationale is at the beginning of the PR description. The multiendpoint backend has significant limitations that cannot be resolved without completely rewriting it. The purpose of this PR is to provide backend that has no significant limitations and can be used in almost in any use case (maybe except some restricted low footprint or very high performance cases).
No, memory allocation metadata is not placed in shared memory. One exception is the buffer length (which must be shared). I payed a lot of attention to always verify it before use to make it secure. I was implementing it as a secure (non-hackable) communication channel. I agree, security experts could have a look. Can you suggest someone?
It is not a new hundreds-lines dynamic memory allocation module. I am actually using one of the zephyr's kind of allocators: I cannot use slabs (or others) because of security reasons (see my answer above). Only alternative to zephyr's |
Fair enough. Thanks for responding to my concerns! |
5da0ba6
to
da38acb
Compare
I updated the PR with new changes:
|
da38acb
to
a2af09e
Compare
:board: nrf5340dk_nrf5340_cpuapp | ||
:goals: debug | ||
|
||
Open a serial terminal (minicom, putty, etc.) and connect the board with the |
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.
Open a serial terminal (minicom, putty, etc.) and connect the board with the | |
Open a serial terminal (for example Minicom or PuTTY) and connect the board with the |
Overview | ||
******** |
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.
Overview | |
******** |
You can leave this heading out, as there is no text under the main heading.
It uses ``icmsg_me`` backend by default and it can be configured to use | ||
``icmsg_with_buf`` backend. |
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.
It uses ``icmsg_me`` backend by default and it can be configured to use | |
``icmsg_with_buf`` backend. | |
By default, it uses the ``icmsg_me`` backend. | |
You can also configure it to use the ``icmsg_with_buf`` backend. |
Reset the board and the following message will appear on the corresponding | ||
serial port, one is host another is remote: |
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.
Reset the board and the following message will appear on the corresponding | |
serial port, one is host another is remote: | |
After resetting the board, the following message will appear on the corresponding | |
serial port: |
You can change the backend to ``icmsg_with_buf`` by changing the devicetree | ||
overlay files. |
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.
You can change the backend to ``icmsg_with_buf`` by changing the devicetree | |
overlay files. | |
To change the backend to ``icmsg_with_buf``, switch the devicetree | |
overlay files as follows: |
Configuration | ||
============= | ||
|
||
The backend is configured via Kconfig and devicetree. |
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.
The backend is configured via Kconfig and devicetree. | |
The backend is configured using Kconfig and devicetree. |
* Define two memory regions and assign them to ``tx-region`` and ``rx-region`` | ||
of an instance. Ensure that the memory regions used for data exchange are | ||
unique (not overlapping any other region) and accessible by both domains | ||
(or CPUs). |
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.
* Define two memory regions and assign them to ``tx-region`` and ``rx-region`` | |
of an instance. Ensure that the memory regions used for data exchange are | |
unique (not overlapping any other region) and accessible by both domains | |
(or CPUs). | |
* Define two memory regions and assign them to ``tx-region`` and ``rx-region`` of an instance. | |
Ensure that the memory regions used for data exchange are unique (not overlapping any other region) and accessible by both domains (or CPUs). |
* Define MBOX devices which are used to send the signal that informs the other | ||
domain (or CPU) that data has been written. Ensure that the other domain | ||
(or CPU) is able to receive the signal. |
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.
* Define MBOX devices which are used to send the signal that informs the other | |
domain (or CPU) that data has been written. Ensure that the other domain | |
(or CPU) is able to receive the signal. | |
* Define MBOX devices for sending a signal that informs the other domain (or CPU) of the written data. | |
Ensure that the other domain (or CPU) can receive the signal. |
You must provide a similar configuration for the other side of the | ||
communication (domain or CPU) but you must swap the MBOX channels, memory | ||
regions (``tx-region`` and ``rx-region``) and block count (``tx-blocks`` and | ||
``rx-blocks``). |
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.
You must provide a similar configuration for the other side of the | |
communication (domain or CPU) but you must swap the MBOX channels, memory | |
regions (``tx-region`` and ``rx-region``) and block count (``tx-blocks`` and | |
``rx-blocks``). | |
You must provide a similar configuration for the other side of the communication (domain or CPU). | |
Swap the MBOX channels, memory regions (``tx-region`` and ``rx-region``) and block count (``tx-blocks`` and | |
``rx-blocks``). |
This architecture allows for overcoming some common problems | ||
with other backends (mostly related to multithread access and zero-copy). | ||
That is why this backend provides an alternative that has no known significant | ||
limitations like the others. |
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.
This architecture allows for overcoming some common problems | |
with other backends (mostly related to multithread access and zero-copy). | |
That is why this backend provides an alternative that has no known significant | |
limitations like the others. | |
This architecture allows for overcoming some common problems with other backends (mostly related to multithread access and zero-copy). | |
This backend provides an alternative with no significant limitations. |
Samples | ||
======= | ||
|
||
- :ref:`ipc_multi_endpoint_sample` |
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.
- :ref:`ipc_multi_endpoint_sample` | |
* :ref:`ipc_multi_endpoint_sample` |
RST list should start with * character
|
||
config IPC_SERVICE_BACKEND_ICMSG_WITH_BUF_NUM_EP | ||
int "Endpoints count" | ||
range 1 251 |
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.
Why 251 is a limitation here? I saw that FF value is used to mark invalid endpoint so could it be 254?
The build assert in code checks if this value is equal or less than 0xFF
#define WAITING_BOUND_MSG_EMPTY 0xFFFF | ||
|
||
/** Alignment of a block. */ | ||
#define BLOCK_ALIGNMENT sizeof(size_t) |
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.
It might be necessary to align block size with the cache line size
#define BLOCK_HEADER_SIZE (offsetof(struct block_header, data)) | ||
|
||
/** Flag indicating that ICMsg was bounded for this instance. */ | ||
#define FLAG_ICMSG_BOUNDED (1 << 31) |
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.
#define FLAG_ICMSG_BOUNDED (1 << 31) | |
#define FLAG_ICMSG_BOUNDED BIT(31) |
#define BYTES_PER_ICMSG_MESSAGE 8 | ||
|
||
/** Maximum ICMsg overhead. It is used to calculate size of ICMsg area. */ | ||
#define ICMSG_BUFFER_OVERHEAD (2 * (sizeof(struct spsc_pbuf) + BYTES_PER_ICMSG_MESSAGE)) |
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.
This and one above define statement, could you move them to icmsg header and connect them with structure size instead of magic numbers which can be changed and values here will nor be valid any longer? Of course if it is possible to do
switch (message->msg_type) { | ||
case MSG_RELEASE_DATA: | ||
ept_addr = EPT_ADDR_INVALID; | ||
/* Fallback to next 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.
You can add here __fallthrough;
statement to avoid potential compiler warnings
bool valid_state; | ||
|
||
/* Find endpoint that matches requested name. */ | ||
buffer = block_from_index(&conf->rx, rx_block_index)->data; |
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.
It would be more readable to do this in two lines
struct backend_data *dev_data = instance->data; | ||
|
||
dev_data->conf = conf; | ||
dev_data->is_initiator = (conf->rx.blocks_ptr < conf->tx.blocks_ptr); |
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.
Maybe it would be better to make it configurable through Kconfig?
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.
Why? Doing it automatically saves a lot of trouble for user, because he don't need to make sure that he put right values for each core. Things gets even more complicated when you have more than two cores. What if you want to talk with one core as initiator and with the other not?
Detecting it in runtime makes the bounding a little bit bigger, because program contains support for both cases. I check it when I was implementing it. It was few hundreds bytes (I don't remember exact number now).
In my plans for improvements I have initiator detection in compile time, so during the preprocessing we will know if we should compile code for initiator, follower or both. I didn't implemented it to don't over-complicate first version.
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 was just wonder if this condition will work in feature when maybe different processors could have different memory mapping but I don't sure if such case is even possible. My other question is if we even need to have roles in this module. It seems to me that both sides could send bound message with local endpoint address to each others.
|
||
/* Set flag that ICMsg is bounded and now, endpoint bounding may start. */ | ||
atomic_or(&dev_data->flags, FLAG_ICMSG_BOUNDED); | ||
schedule_ept_bound_process(dev_data); |
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.
Does it make sense to trigger bound process on no initiator side on ICMsg bound since it will be triggered on every bound message reception?
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.
But the user can call bound of endpoint before ICMsg is bounded (or later), that is always scheduled, so it will be handled when both are conditions are true.
*token = ept; | ||
|
||
/* Rest of the bounding will be done in the system workqueue. */ | ||
schedule_ept_bound_process(dev_data); |
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.
Is it needed on no initiator side?
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.
Yes
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.
Could we just assume that endpoint registration is done after icmsg control channel is open. It could simplify logic because we wouldn't need to have states and bound message could be sent directly from this function.
ce5baed
to
6b07cb5
Compare
6b07cb5
to
db0579c
Compare
I pushed new changes:
|
db0579c
to
2093eb9
Compare
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.
Just some small nits.
$ west build -b nrf5340dk_nrf5340_cpuapp --sysbuild -- \ | ||
-DDTC_OVERLAY_FILE=boards/nrf5340dk_nrf5340_cpuapp_icbmsg.overlay \ | ||
-Dremote_DTC_OVERLAY_FILE=boards/nrf5340dk_nrf5340_cpunet_icbmsg.overlay |
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.
Could you please add this to sample.yaml
?
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.
Add this to sample.yaml
sample.ipc.multi_endpoint.icbmsg:
platform_allow: nrf5340dk_nrf5340_cpuapp
integration platforms:
- nrf5340dk_nrf5340_cpuapp
tags: ipc
sysbuild: true
extra_args:
DTC_OVERLAY_FILE=boards/nrf5340dk_nrf5340_cpuapp_icbmsg.overlay
remote_DTC_OVERLAY_FILE=boards/nrf5340dk_nrf5340_cpunet_icbmsg.overlay
#include <zephyr/ipc/ipc_service_backend.h> | ||
#include <zephyr/cache.h> | ||
|
||
LOG_MODULE_REGISTER(ipc_svc_icbmsg, |
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.
nit: Maybe just ipc_icbmsg
.
alloc_size = len; | ||
r = alloc_tx_buffer(dev_data, &alloc_size, &buffer, K_FOREVER); | ||
if (r < 0) { | ||
return r; |
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.
You actually reworked alloc_tx_buffer
return codes instead of just overriding the return code here, which is fine, but could be harder to support if return codes will change in IPC-service and they will.
bf81481
to
d1ca650
Compare
d1ca650
to
2d0f767
Compare
The icmsg and icmsg_me backends has limitations in context of concurrent access to single instance. Some limitations are: * sending by more thread at the same time may cause -EBUSY * allocating TX buffer will cause errors in other threads that want to allocate before first thread sent the message, * during no-copy receive, when RX buffer is on hold receiving is totally blocked. This backend resolves those limitations by adding dynamically allocated buffers on shared memory next to ICmsg circular buffer. The data is passed using those buffers, so concurrency is not a problem. The ICmsg is used only to pass short 2-byte messages containing references to those buffers. The backend also supports multiple endpoint. The ipc/icmsg_me sample was modified to support this backend. Signed-off-by: Dominik Kilian <Dominik.Kilian@nordicsemi.no>
There are more ipc_service backends that supports multiple endpoint. This sample can be used for any of those backends, so this commits makes the sample more generic. The default backend it still icmsg_me, but now, the sample has files and instructions for icmsg_with_buf backend. Signed-off-by: Dominik Kilian <Dominik.Kilian@nordicsemi.no>
2d0f767
to
f058c58
Compare
The
icmsg
andicmsg_me
backends have limitations in context of concurrent access to a single instance. Some of the limitations are:This backend resolves those limitations by adding dynamically allocated buffers on shared memory next to ICmsg circular buffer. The data is passed using those buffers, so concurrency is not a problem. The ICmsg is used only to pass short 2-byte messages containing references to those buffers. The backend also supports multiple endpoint. The ipc/icmsg_me sample was modified to support this backend.