-
Notifications
You must be signed in to change notification settings - Fork 7.7k
kernel: msgq: adding support for k_msgq_put_front
and sample code
#92865
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
base: main
Are you sure you want to change the base?
kernel: msgq: adding support for k_msgq_put_front
and sample code
#92865
Conversation
k_msgq_put_urgent
and sample code
d4fac33
to
85938e2
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.
A design nitpick around the cut/paste feel of the change.
Also: please isolate changes to the sample to a separate commit from the semantic change in the kernel.
And while a sample can be used as a test case, it's really best to put an actual test case of this functionality into tests/kernel/msgq/msgq_api
kernel/msg_q.c
Outdated
|
||
/* give message to waiting thread */ | ||
(void)memcpy(pending_thread->base.swap_data, data, | ||
msgq->msg_size); |
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.
Not loving the duplication of the implementation code. Ideally all this should be shared. In particular maintenance-quality-issues are at their most important in code surrounding memory copies into/out-of caller pointers (which can cross security boundaries if CONFIG_USERSPACE=y!). Might take some refactoring to make that happen, obviously.
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 understand and agree, but at first I really didn't want to touch in the already existing k_msgq_put
function. So should I do something like this, then?
int _copy_to_queue ( ... , bool should_copy_to_back) {
// shared implementation goes here
}
int z_impl_k_msgq_put( ... ) {
return _copy_to_queue( ... , true);
}
int z_impl_k_msgq_put_urgent( ... ) {
return _copy_to_queue( ..., , false);
}
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.
FWIW I have done just that to avoid code duplication.
samples/basic/msg_queue/src/main.c
Outdated
} | ||
|
||
received[BUF_SIZE - 1] = '\0'; | ||
/* we expect to see CBA012345... */ |
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.
While this is a good note for understanding, it would be even better for this to be a test that actually checked messages were received in the expected order.
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 agree. For now I have refactored the sample code to have a regex to check the sample output (with the message sequence) on the tests. I intend to do the proper ZTEST thing as you suggested as well, but I'd like to settle on the implementation before that.
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 add reference: #46339 to PR description.
Also this feature is good to be optional and it need more test.
kernel/msg_q.c
Outdated
/* message queue isn't full */ | ||
pending_thread = z_unpend_first_thread(&msgq->wait_q); | ||
if (unlikely(pending_thread != NULL)) { | ||
SYS_PORT_TRACING_OBJ_FUNC_EXIT(k_msgq, put, msgq, timeout, 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.
Add an additional trace log to facilitate easier debugging.
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 like this idea, however I'm not certain on how to refactor this part. would this be just changing put
by put_urgent
(or whatever naming convention we end up deciding)? Like this:
SYS_PORT_TRACING_OBJ_FUNC_EXIT(k_msgq, put_urgent, msgq, timeout, 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.
You can refer #71195
I think I finished most implements at that time.
34fffd8
to
66d7bfd
Compare
done! not sure what you mean by "good to be optional", though. Proper tests will be done soon - I wanted some feedback on this contribution before taking the effort (if for some reason this was immediately discarded by reviewers, then it would be a waste of energy). |
66d7bfd
to
347fdde
Compare
In my mind, the feature is not required for every developer so we can be optional and make the user to decide enabling it or not, this can reduce code size. |
3ca45a2
to
6ccbe34
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.
Aside from needing a proper test case, this looks to be on the right track--just some minor suggestions to tidy up a couple of corners.
* | ||
* This routine sends a message to the beginning (head) of message queue @a q. | ||
* Messages sent with this method will be retrieved before any pre-existing | ||
* messages in the queue. |
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 think that it is worthwhile to point out that this urgent behavior only applies only if there is available space in the message queue's buffer. If that buffer is full, then all urgency is ignored and it behaves the same as k_msgq_put().
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 have ditched the _urgent
suffix due to the suggestion made by @TaiJuWu. It's _front
for now, so perhaps this is less of a concern. I have included a note anyway:
@note if there is no space in the message queue, this function will behave the same as k_msgq_put.
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.
Thanks for adding the note. I still think that it is important to have in there.
kernel/msg_q.c
Outdated
/* decrement the read pointer */ | ||
msgq->read_ptr -= msgq->msg_size; | ||
if (msgq->read_ptr < msgq->buffer_start) { | ||
msgq->read_ptr = msgq->buffer_end - msgq->msg_size; | ||
} |
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 would suggest checking for the start of the buffer first.
if (msgq->read_ptr == msgq->buffer_start) {
msgq->read_ptr = msgq->buffer_end;
}
msgq->read_ptr -= msgq->msg_size;
That should guard against the extremely unlikely scenario of an underflow should the start of the buffer ever reside at address 0x0.
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.
Clever one! I hadn't thought about that, thanks for highlighting the issue. Changes have been made.
kernel/msg_q.c
Outdated
} | ||
|
||
/* match read and write pointers */ | ||
msgq->write_ptr = msgq->read_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.
I think I understand the desire to use msgq->write_ptr -- it maintains a sort of consitency with k_msgq_put(). However, I think it would be okay to just use a temporary variable so that we don't have to restore msgq_write_ptr
afterwards.
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.
While writing about this contribution I came to a similar idea as yours, however I went a bit further in the search for optimization: since we have msgq->read_ptr
already where we want to place the incoming message, it makes sense to just write directly to it instead of needing to manipulate write_ptr
or, as you suggested, creating a temporary variable just for it.
Full explanation I left in the code:
/*
* writing a message to the head of the queue is
* less obvious, but it can be achieved by simply
* following these steps:
*
* 1. decrementing the read pointer. This effectively
* opens space for the incoming message in the head of
* the queue.
*
* 2. temporarily matching the write pointer with the
* read pointer. This way the message will be written
* in the recently opened space.
*
* 3. reverting the write pointer after the write to its
* original value. The read pointer, by its turn, should
* be always at the head, so it doesn't need reverting
* because we just wrote to the head.
*
* ...but this is inefficient because we need to go back
* and forth with the value of write_ptr; note that it
* becomes equal to read_ptr just to be restored afterwards.
* A much more efficient solution is to simply write the
* message directly to read_ptr's address after step 1,
* which avoids needing to perform steps 2 and 3 altogether.
*/
if (msgq->read_ptr == msgq->buffer_start) {
msgq->read_ptr = msgq->buffer_end;
}
msgq->read_ptr -= msgq->msg_size;
(void)memcpy(msgq->read_ptr, (char *)data, msgq->msg_size);
6ccbe34
to
5bcb4d8
Compare
k_msgq_put_urgent
and sample codek_msgq_put_front
and sample code
I'm not sure about that simply because all of the message queue API is already fully included by default. Leaving just one single function (which is not that big anyway) as opt-in to enable or not doesn't look right to me. If others share your concern, I'll be happy to make the change though. |
thank you for your time and comments, peter! I have applied most of your suggestions. |
{ | ||
__ASSERT(!arch_is_in_isr() || K_TIMEOUT_EQ(timeout, K_NO_WAIT), ""); |
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 remove ! arch_is_in_isr
, the function will make thread go to sleep (line 208)?
And you need to addition bit to point out the reason of sleep.
If Im wrong, please correct me.
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.
sorry, that removal was by accident. I have restored it to the original state. The SYS_PORT_TRACING
macros will be adjusted later.
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.
There is condition for K_TIMEOUT_EQ(timeout, K_NO_WAIT)
at line 202 already so I guess we don't need this condition or we can remove it .
By the way, the secondary comment is not resolve, it is we need to addition bit to point out the reason of sleep.
If a thread go to sleep, we need additional bit to point out the reason to sleep.
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.
Both checks are needed as blocking calls are not allowed at ISR level.
!arch_is_in_isr() : If true, then we are at at thread level, which is OK. Bail out of the assert.
Otherwise, it's false, (operating ISR level) and we must test for a blocking timeout. If K_NO_WAIT, it is non-blocking and this is OK from an ISR. Otherwise, it is a blocking timeout and the assert is triggered.
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
SYS_PORT_TRACING
macros will be adjusted later.
You can cherry-pick from my PR, just add me to co-authoer, I will also appreciate it.
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.
@TaiJuWu I will check that with my advisor, but as far as I know I unfortunately can't add you as co-author because this contribution is related to my thesis work, and thus it is required of me to be the only author.
I have added an extra commit exclusively for the tracing following your contribution. Cherry-picking wasn't possible due to the suffix difference.
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 am fine for every decision you make ;)
It's purely my own bias, to be honest.
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.
Both checks are needed as blocking calls are not allowed at ISR level.
!arch_is_in_isr() : If true, then we are at at thread level, which is OK. Bail out of the assert. Otherwise, it's false, (operating ISR level) and we must test for a blocking timeout. If K_NO_WAIT, it is non-blocking and this is OK from an ISR. Otherwise, it is a blocking timeout and the assert is triggered.
Thanks for your explanation!
Should we change the logic to arch_is_in_isr && !K_TIMEOUT_EQ(timeout, K_NO_WAIT)
This will improve readability.
5bcb4d8
to
7944b64
Compare
@TaiJuWu - I may be misunderstanding the intent behind your desire to make it optional for as I see it, it would in a sense automatically be so. That is, Zephyr uses garbage collection at build time to remove both unused functions and unused variables. Consequently, if no one uses the new urgent/front routine, it does not find its way into the final image. |
7944b64
to
36618db
Compare
kernel/msg_q.c
Outdated
* writing a message to the head of the queue is | ||
* less obvious, but it can be achieved by simply | ||
* following these steps: | ||
* | ||
* 1. decrementing the read pointer. This effectively | ||
* opens space for the incoming message in the head of | ||
* the queue. | ||
* | ||
* 2. temporarily matching the write pointer with the | ||
* read pointer. This way the message will be written | ||
* in the recently opened space. | ||
* | ||
* 3. reverting the write pointer after the write to its | ||
* original value. The read pointer, by its turn, should | ||
* be always at the head, so it doesn't need reverting | ||
* because we just wrote to the head. | ||
* | ||
* ...but this is inefficient because we need to go back | ||
* and forth with the value of write_ptr; note that it | ||
* becomes equal to read_ptr just to be restored afterwards. | ||
* A much more efficient solution is to simply write the | ||
* message directly to read_ptr's address after step 1, | ||
* which avoids needing to perform steps 2 and 3 altogether. |
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.
Let's simplify this comment. Perhaps something like ...
* writing a message to the front of the queue is
* simple and effective: create space for the message
* by decrementing read_ptr, then copy the message
* to the newly created space
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.
alright, I have reduced the message. I still think the full rationale is valuable, though.
K_OOPS(K_SYSCALL_MEMORY_READ(data, msgq->msg_size)); | ||
|
||
return z_impl_k_msgq_put_front(msgq, data, timeout); | ||
} |
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 think we are missing ...
#include <zephyr/syscalls/k_msgq_put_front_mrsh.c>
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.
really? ok, but what's that for?
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 part of the system call infrastructure--for marshalling function.
* | ||
* This routine sends a message to the beginning (head) of message queue @a q. | ||
* Messages sent with this method will be retrieved before any pre-existing | ||
* messages in the queue. |
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.
Thanks for adding the note. I still think that it is important to have in there.
@@ -4785,7 +4785,7 @@ __syscall int k_msgq_alloc_init(struct k_msgq *msgq, size_t msg_size, | |||
int k_msgq_cleanup(struct k_msgq *msgq); | |||
|
|||
/** | |||
* @brief Send a message to a message queue. | |||
* @brief Send a message to the end of a message queue. |
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 commit message still has a referent to k_msgq_put_urgent instead of k_msgq_put_front.
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.
thanks for the notice, changed it already.
Thanks for your explanation, I have not known this knowledge, I agree with you. |
This commit introduces the k_msgq_put_front API for sending messages to a queue in a LIFO scheme. Signed-off-by: Alexander Paschoaletto <axelpinheiro@gmail.com>
this commit adds a sample code to illustrate the base usage of message queues. a producer and a consumer thread work together, exchanging messages in a FIFO (for normal payloads) and LIFO (for higher priority payloads) schemes. Signed-off-by: Alexander Paschoaletto <axelpinheiro@gmail.com>
36618db
to
fcd6407
Compare
1dc7e6e
to
3b88b4a
Compare
|
||
[producer] sending: 0 | ||
[producer] sending: 1 | ||
[producer] sending: A (urgent) |
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.
urgent or front?
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.
in the context of the sample, the publisher thread sends normal and urgent messages. It's not related to the API naming.
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.
Got it, thanks.
Once the minor compliance check is fixed, I expect I will be giving this a +1. |
3b88b4a
to
68bfba2
Compare
This commit adds the tracing macros and functions related specifically to the k_msgq_put_front API. Signed-off-by: Alexander Paschoaletto <axelpinheiro@gmail.com>
68bfba2
to
8223338
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.
Mostly nits
******** | ||
|
||
A simple sample to demonstrate the basic usage of Zephyr message queues. | ||
a producer thread sends both normal and urgent messages. |
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.
a producer thread sends both normal and urgent messages. | |
A producer thread sends both normal and urgent messages. |
:goals: run | ||
:compact: | ||
|
||
To build for another board, change "qemu_x86" above to that board's name. |
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.
To build for another board, change "qemu_x86" above to that board's name. | |
To build for another board target, replace "qemu_x86" above with it. |
@@ -0,0 +1,2 @@ | |||
# nothing here |
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.
# nothing here |
@@ -0,0 +1,17 @@ | |||
sample: | |||
description: A message queue usage sample | |||
name: message queue 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.
name: message queue sample | |
name: message queue |
|
||
/* | ||
* sends messages every 100 msec, in repeating | ||
* sequence normal, normal, urgent, ... |
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.
* sequence normal, normal, urgent, ... | |
* sequence: normal, normal, urgent, ... |
Overview | ||
******** | ||
|
||
A simple sample to demonstrate the basic usage of Zephyr message queues. |
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.
A simple sample to demonstrate the basic usage of Zephyr message queues. | |
A sample demonstrating the basic usage of Zephyr message queues. |
queue, and the first message to go is the first to be delivered. Every "urgent" | ||
message, at its turn, implies sending the message to the beginning of the queue. | ||
|
||
In this sample, one producer thread sends 1 urgent message for each 2 regular |
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.
In this sample, one producer thread sends 1 urgent message for each 2 regular | |
In this sample, one producer thread sends 1 urgent message for each 2 normal |
Every normal message sending implies sending the message to the end of the | ||
queue, and the first message to go is the first to be delivered. Every "urgent" | ||
message, at its turn, implies sending the message to the beginning of the queue. |
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.
Every normal message sending implies sending the message to the end of the | |
queue, and the first message to go is the first to be delivered. Every "urgent" | |
message, at its turn, implies sending the message to the beginning of the queue. | |
Every normal message is put at the end of the queue, and they are delivered as FIFO. | |
Every "urgent" message is put at the beginning of the queue, | |
and it is delivered first as long as no other "urgent" message comes in after it. |
@@ -0,0 +1,17 @@ | |||
sample: | |||
description: A message queue usage 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.
description: A message queue usage sample | |
description: Message queue demo sample |
tags: | ||
- message_queue |
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.
tags: | |
- message_queue | |
tags: | |
- message_queue |
already above
it is a test failure, is it excepted? The reason is described in #92865 (comment)
|
Following the examples of FreeRTOS and RTEMS, this PR introduces a
k_msgq_put_front
API for sending messages to the head of the queue, in a LIFO scheme. It also adds a simple sample code that demonstrates how it works.Due to the fact that both versions of
k_msgq_put
share most of the code, this contribution creates an internalput_msg_in_queue
function which inserts the message at the front or back of the queue depending on the flagput_at_back
passed to it. Now the implementation looks like this:note: some work concerning mainly docs and ci/cd still needs updating, but the bulk of the contribution is here. I'd like to have some opinions on this contribution before taking the time to do everyhing properly.
Closes #46339