Make k_delayed_work_cancel cancel pending work #777
Conversation
| @@ -57,12 +57,14 @@ void k_queue_init(struct k_queue *queue) | |||
| SYS_TRACING_OBJ_INIT(k_queue, queue); | |||
| } | |||
|
|
|||
| #if !defined(CONFIG_POLL) | |||
There was a problem hiding this comment.
Shouldn't we make CONFIG_POLL mandatory now as if this is not set, then we would not be able to cancel the pending work, which would be very confusing for the end user as some config would allow cancelling always and some not.
There was a problem hiding this comment.
This is simpler to start with, if we do choose to go with mandatory CONFIG_POLL there is no reason to keep the logic around wait_q but many objects that do have wait_q don't have POLL_EVENT yet so this will serve as test bed how to convert the objects so we can take advantage of k_poll if that became built-in. I actually asked about this making k_poll part of the always enabled set of APIs in the mailing list but nobody responded.
Vudentz
force-pushed
the
master
branch
2 times, most recently
from
fc9415f to
83fff72
Compare
|
While this seems to work reliably now, there is one thing we might have to discuss: We could in theory have a sys_dlist_t instead of just one poller, in which case this is exactly like wait_q, though the existing logic insert items in priority order. This perhaps would lead to a behavior definition on waiters vs pollers, right now the waiters are always check first so pollers are left with the remaining regardless if the poller thread priority is higher than the waiters (perhaps this should be considered a bug). |
tests/kernel/workq/work_queue/:w
Outdated
| @@ -0,0 +1,3973 @@ | |||
| /* | |||
There was a problem hiding this comment.
I may imagine this file named ":w" was added to the PR by mistake?
|
As someone who faced the issue this patch fixes in #672, I can say that the issue of not being able to cancel pending work after it was thread-scheduled turned out to be not so serious. Still, one needs to work it around, so I appreciate @Vudentz' work on this. This however makes pretty big changes to kernel and indeed makes a step towards dependency on CONFIG_POLL feature, and that may make Zephyr core less modular and bigger. So, I'm informally +0.5 on this, but it would be interesting to know if this helps Bluetooth folks, and how kernel maintainers like it (@jhedberg , @andrewboie are in reviewers). |
|
@pfalcon Note that _POLL_EVENT is just a pointer, _wait_q_t aka sys_dlist_t is actually 2 and even if we do multiple pollers it can be done with the use of sys_dlist_t so it would be actually the same memory usage per object. It the k_poll_event that may be more heavy weight but that is per user of k_poll, which I guess it is a small price comparing to be able to use k_poll in any object that nowadays supports waiters. |
tests/kernel/queue/testcase.yaml
Outdated
| @@ -1,3 +1,6 @@ | |||
| tests: | |||
| - test: | |||
| tags: kernel | |||
| - test_pool: | |||
There was a problem hiding this comment.
Apparently typo, should be "test_poll".
There was a problem hiding this comment.
Nice catch, I will fix it.
include/kernel.h
Outdated
| * @brief Remove an element from a queue. | ||
| * | ||
| * This routine removes data item from @a queue. The first 32 bits of the | ||
| * data item are reserved for the kernel's use. |
There was a problem hiding this comment.
Probably worth documenting that this is an O(N) list removal. The slist API is called "find and remove" for a reason.
There was a problem hiding this comment.
Or we changed it to use sys_dlist_t as the wait_q, though this can be done separately if we find that would be a problem.
k_queue_remove can be used to remove an element from any position in the queue. Signed-off-by: Luiz Augusto von Dentz <[email protected]>
Add another list of elements which is removed before k_queue_get is called. Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This makes use of POLL_EVENT in case k_poll is enabled which is preferable over wait_q as that allows objects to be removed for the data_q at any time. Signed-off-by: Luiz Augusto von Dentz <[email protected]>
Make use of k_queue directly since it has a more flexible API. Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This has been a limitation caused by k_fifo which could only remove items from the beggining, but with the change to use k_queue in k_work_q it is now possible to remove items from any position with use of k_queue_remove. Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This adds a test that attempts to submit a work with 0 timeout thus causing it to immediatelly be submitted to the queue so it is pending execution which is then cancelled with k_delayed_work_cancel. Note this can only be done with coop threads with the same or higher priority otherwise the work_q thread is wakeup before k_delayed_work_cancel takes place, thus why test_delayed_cancel uses K_HIGHEST_THREAD_PRIO. Signed-off-by: Luiz Augusto von Dentz <[email protected]>
…s#777) This allow the readonly properties to be modifiable in native so objects can update the property or delete them if needed. Also adding a new helper method that can delete a property. Signed-off-by: Jimmy Huang <[email protected]>
Second try of #682, now using k_poll when CONFIG_POLL is enabled.