kernel: Introduce k_triggered_work #14041

pizi-nordic · 2019-03-04T15:43:46Z

This PR introduces triggered work. The triggered work is submitted to workqueue when one of watched events happen. Such mechanism allows to eliminate costly dedicated threads to execute something when data is available or some resource become ready.

The work is driven mostly by device power management (details: #13382 (comment)) and @pdunaj needs.

Fixes: #10748

zephyrbot · 2019-03-04T15:50:16Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

andyross

Maybe it would be a little more general to have a callback-based API where a kernel function can be registered on a pollable object that gets called on event state changes, then implement k_triggered_work() in terms of that? That strikes me as useful too, and that way we won't have to implement it as two separate subsystems.

pabigot · 2019-03-04T22:20:19Z

@BFrog and @microbuilder might be able to provide feedback as to whether this feature would help solve sensor notification problems where #13718 (comment) suggested that callbacks are problematic but we still want to get rid of coupling sensor drivers to a specific thread hand-off solution.

I don't fully understand @andyross's suggestion but if its intent is to support immediate execution of specific actions ("kernel function") on event notifications, where one of the available actions is submitting work to a queue, I could see that being more general. Examples of other useful triggers would help motivate the generalization.

pdunaj · 2019-03-05T09:15:41Z

@andyross

a callback-based API where a kernel function can be registered on a pollable object that gets called on event state changes

I am not sure this would work well. If state change happens from atomic context (e.g. interrupt) some API (e.g. mutex) would be unusable. Triggering work with pollable objects makes code executed from workq thread which has fewer limitations. Triggered work should work in userspace.

As an alternative I was also thinking about creating callback manager that would have its own thread and poll on behalf of others. This is however more expressive from the RAM perspective as you have to create yet another thread and yet another stack.

pizi-nordic · 2019-03-05T13:32:06Z

I have updated the PR introducing a callback into _poller structure. I think that this is the thing suggested by @andyross as it allows for easy extension in the future.

I have kept the callback internal to the polling code, as exporting it to the user might create a lot of trouble: It is called with interupts disabled from various contexts.

andyross · 2019-03-06T16:39:11Z

To be clear: my thinking here is targetted more at how to evolve poll instead of bolting things onto it. IMHO poll (not really the subject of this patch per se) is implemented at the wrong level of abstraction. Threads that wait for something, in Zephyr, are always waiting on a wait_q. So the wait_q should be 1:1 with our "signalable event" thing. But that's not the way poll works, so everything needs to be special cased ("Oh, you're waiting on a semaphore, that's different than being pended on a fifo and I'm going to handle them with a big switch statement").

I'd like to get those switches and special cases out of poll, and make "signal interest in multiple wait_q's" a single lower level operation. In that world, having special cases on what to do after the signal (i.e. ready the thread vs. add a work item) has all the same problems and I'd like to have the low level operation be as decoupled from the API as possible.

pizi-nordic · 2019-03-07T11:28:34Z

@andyross: What you propose will not work with the k_triggered_work interface introduced here. I simply do not have a thread which I could put in the wait_q.

tbursztyka · 2019-03-13T09:57:18Z

kernel/poll.c

+
+int _impl_k_poll(struct k_poll_event *events, int num_events, s32_t timeout)
+{
+	__ASSERT(!_is_in_isr(), "");


asserts after the variable declarations

kernel/poll.c

pizi-nordic · 2019-09-25T15:37:45Z

@pabigot: I added basic tests for the new functionality.

pabigot

This seems ok. There's enough that I'm not entirely happy about that I don't want to go ahead and approve it. There's nothing here that I would, at this moment, insist be changed, but I don't think there's been enough eyes on it from a functional perspective to just merge it.

I'd still like to see it named k_work_poll since it's essentially a threadless k_poll.

Also the timeout addition should be squashed into the commit that adds the new API.

When a work item timeout occurs I believe it would be better to delay disabling the poller until the wrapper work handler clears the event registry. That would require separate tracking of whether the timeout fired and whether events were discovered; e.g. add to struct k_work_triggered a signaled flag that gets set in k_work_triggered_poller_cb just as expired is set in triggered_work_expiration_handler. Then the consumer can optimally handle any events that occur between the timeout and the point where the work handler gets invoked rather than assuming there's nothing to do except timeout activity.

@andyross or somebody from the kernel team should determine whether the addition of CONFIG_POLL=y to tests/kernel/workq/work_queue is appropriate, since that eliminates the former use of that test when CONFIG_POLL is not selected.

pabigot · 2019-09-26T19:52:54Z

tests/kernel/workq/work_queue/prj.conf

@@ -1,3 +1,4 @@
 CONFIG_ZTEST=y
 CONFIG_QEMU_TICKLESS_WORKAROUND=y
+CONFIG_POLL=y


With this addition please remove the section of testcase.yaml that runs this test with CONFIG_POLL=y.

Or it may be that the new API needs to be in a separate test so the behavior of workq without CONFIG_POLL continues to be tested. For that we need input from a kernel maintainer.

pabigot · 2019-09-26T19:56:15Z

kernel/poll.c

+	k_work_submit_to_queue(work_q, &twork->work);
+}
+
+static int k_work_triggered_poller_cb(struct k_poll_event *event, u32_t status)


For consistency with other internal functions this should be triggered_work_poller_cb. The k_ prefix makes it look like public API.

pabigot · 2019-09-26T19:57:08Z

kernel/poll.c

+	struct _poller *poller = event->poller;
+
+	if (poller->is_polling && poller->thread) {
+		struct k_work_triggered *work =


In at least one other function (the expiration handler above) this variable is twork to distinguish it from the contained struct k_work member. Consistent naming that indicates the type of the pointed-to value would be nice.

pizi-nordic · 2019-09-27T13:07:24Z

When a work item timeout occurs I believe it would be better to delay disabling the poller until the wrapper work handler clears the event registry. That would require separate tracking of whether the timeout fired and whether events were discovered; e.g. add to struct k_work_triggered a signaled flag that gets set in k_work_triggered_poller_cb just as expired is set in triggered_work_expiration_handler. Then the consumer can optimally handle any events that occur between the timeout and the point where the work handler gets invoked rather than assuming there's nothing to do except timeout activity.

I think that it is possible now:

If expired flag is false, the event happened before timeout.
If expired flag true, the timeout happened before any event. However the events are still monitored after timeout up to clean-up phase. As result the application could scan the events to check if something changed state after timeout and before execution.

pabigot · 2019-09-27T14:04:51Z

As result the application could scan the events to check if something changed state after timeout and before execution.

Yes, but it's more accurate to say that the application must scan the events, as once the timeout occurs nothing outside the event array records the fact of a subsequent signal. In k_poll there's a binary choice between 0 and -EAGAIN, which is fine there because there is no significant interval between the timeout and clearing the event registration. For the work item there's an arbitrary delay between those two steps which increases the likelihood of an event being signalled but not announced.

I had proposed:

for non-error situations [k_work_poll] will store the return code that k_poll would have returned (indicating either timeout or event) into a field in struct k_work_poll, so the work item can handle both timeout and event triggers.

Switching from bool expired to int poll_result would seem to make sense as it's compatible with existing API. Unfortunately that API doesn't support representing that both timeout and an event signal have occurred, though we could do a post-timeout change of the result from -EAGAIN to 0 when an event comes in, which would be pretty natural. We could also handle -EINTR too.

Again I'm not requesting that you do this; I still want other eyes on it before approving. As is it's a nice feature (though with surprisingly little interest); it could just have better compatibility with existing API, IMO.

pabigot · 2019-10-01T13:52:24Z

requested by @pizi-nordic for API meeting: provide input on #14041 (comment) i.e. whether API should follow k_poll in name and return value, or remain distinct as k_work_triggered.

Prototype in last commits of https://github.com/pabigot/zephyr/commits/pr/14041

attn @carlescufi

pabigot · 2019-10-01T17:27:02Z

Added to dev-review since #14041 (comment) didn't get time in API telecon. @pizi-nordic would really like to get the API settled so this can be merged.

galak · 2019-10-03T15:24:04Z

dev-review: meeting consensus was to go with @pabigot proposal and rename to k_work_poll as the prefix

This commit separates k_poll() infrastructure from k_poll() API implementation, allowing other (future) API calls to use the same framework. Signed-off-by: Piotr Zięcik <piotr.ziecik@nordicsemi.no>

pabigot

Two small documentation changes (fix link, mention submit on timeout).

pabigot · 2019-10-04T11:34:16Z

doc/reference/kernel/threads/workqueue.rst

+The :cpp:func:`k_work_poll()` interface schedules a triggered work
+item in response to a **poll event** (see :ref:`polling_v2`), that will
+call a user-defined function when the monitored resource becomes available
+or a poll signal is raised. In contrast to :cpp:func:`k_poll()`,


" ~~the~~ a monitored resource becomes available or a poll signal is raised, or a timeout occurs."

pabigot · 2019-10-04T11:43:37Z

doc/reference/kernel/threads/workqueue.rst

+Triggered Work
+**************
+
+The :cpp:func:`k_work_poll()` interface schedules a triggered work


This needs to be k_work_poll_submit() for the documentation link to work.

pabigot · 2019-10-04T11:48:32Z

kernel/poll.c

+				  .thread     = _current,
+				  .cb         = k_poll_poller_cb };
+
+	__ASSERT(!z_is_in_isr(), "");


Sorry, didn't run it first: this needs to be z_arch_is_in_isr() (upstream rename).

This commit adds new k_work_poll interface. It allows to submit given work to a workqueue automatically when one of the watched pollable objects changes its state. Signed-off-by: Piotr Zięcik <piotr.ziecik@nordicsemi.no> Signed-off-by: Peter A. Bigot <pab@pabigot.com>

pizi-nordic requested review from nashif, ioannisg, pdunaj, andrewboie, carlescufi and ramakrishnapallala March 4, 2019 15:43

pizi-nordic requested a review from andyross as a code owner March 4, 2019 15:43

pizi-nordic requested a review from pabigot March 4, 2019 15:44

ioannisg requested review from galak and agross-oss March 4, 2019 15:47

pizi-nordic force-pushed the k_triggered_work branch from 685cad3 to c312e15 Compare March 4, 2019 15:54

andyross reviewed Mar 4, 2019

View reviewed changes

pizi-nordic force-pushed the k_triggered_work branch from c312e15 to bd752ce Compare March 5, 2019 13:22

pizi-nordic force-pushed the k_triggered_work branch 4 times, most recently from 1403599 to f7b2c12 Compare March 6, 2019 15:57

pizi-nordic force-pushed the k_triggered_work branch from f7b2c12 to a3a4c34 Compare March 11, 2019 13:47

pizi-nordic changed the title ~~[RFC] kernel: Introduce k_triggered_work~~ kernel: Introduce k_triggered_work Mar 11, 2019

pizi-nordic force-pushed the k_triggered_work branch 2 times, most recently from ae27adb to ff8809c Compare March 11, 2019 15:20

tbursztyka reviewed Mar 13, 2019

View reviewed changes

kernel/poll.c Show resolved Hide resolved

pizi-nordic force-pushed the k_triggered_work branch from ff8809c to 2eeeeb3 Compare March 18, 2019 10:37

pizi-nordic force-pushed the k_triggered_work branch from 299f11e to cb606a0 Compare September 25, 2019 15:40

pabigot self-requested a review September 26, 2019 14:34

pizi-nordic force-pushed the k_triggered_work branch 2 times, most recently from dfe2d56 to 34ae8c3 Compare September 26, 2019 14:46

pabigot reviewed Sep 26, 2019

View reviewed changes

pizi-nordic force-pushed the k_triggered_work branch 3 times, most recently from 82d3518 to b1cadc1 Compare September 27, 2019 11:09

pabigot added the dev-review To be discussed in dev-review meeting label Oct 1, 2019

galak removed the dev-review To be discussed in dev-review meeting label Oct 3, 2019

kernel: Separate k_poll() infrastructure and implementation

604fa99

This commit separates k_poll() infrastructure from k_poll() API implementation, allowing other (future) API calls to use the same framework. Signed-off-by: Piotr Zięcik <piotr.ziecik@nordicsemi.no>

pizi-nordic force-pushed the k_triggered_work branch 3 times, most recently from 76ed74c to fb81402 Compare October 4, 2019 10:12

pabigot requested changes Oct 4, 2019

View reviewed changes

pabigot reviewed Oct 4, 2019

View reviewed changes

pizi-nordic force-pushed the k_triggered_work branch from fb81402 to 687f290 Compare October 4, 2019 11:50

pabigot approved these changes Oct 4, 2019

View reviewed changes

carlescufi approved these changes Oct 4, 2019

View reviewed changes

carlescufi merged commit 19d8349 into zephyrproject-rtos:master Oct 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kernel: Introduce k_triggered_work #14041

kernel: Introduce k_triggered_work #14041

pizi-nordic commented Mar 4, 2019 •

edited

Loading

zephyrbot commented Mar 4, 2019 •

edited

Loading

andyross left a comment

pabigot commented Mar 4, 2019

pdunaj commented Mar 5, 2019

pizi-nordic commented Mar 5, 2019

andyross commented Mar 6, 2019

pizi-nordic commented Mar 7, 2019

tbursztyka Mar 13, 2019

pizi-nordic Mar 18, 2019

pizi-nordic commented Sep 25, 2019 •

edited

Loading

pabigot left a comment

pabigot Sep 26, 2019

pizi-nordic Sep 27, 2019

pabigot Sep 26, 2019

pizi-nordic Sep 27, 2019

pabigot Sep 26, 2019

pizi-nordic Sep 27, 2019

pizi-nordic commented Sep 27, 2019

pabigot commented Sep 27, 2019

pabigot commented Oct 1, 2019

pabigot commented Oct 1, 2019

galak commented Oct 3, 2019

pabigot left a comment

pabigot Oct 4, 2019

pizi-nordic Oct 4, 2019

pabigot Oct 4, 2019

pizi-nordic Oct 4, 2019

pabigot Oct 4, 2019

pizi-nordic Oct 4, 2019

kernel: Introduce k_triggered_work #14041

kernel: Introduce k_triggered_work #14041

Conversation

pizi-nordic commented Mar 4, 2019 • edited Loading

zephyrbot commented Mar 4, 2019 • edited Loading

andyross left a comment

Choose a reason for hiding this comment

pabigot commented Mar 4, 2019

pdunaj commented Mar 5, 2019

pizi-nordic commented Mar 5, 2019

andyross commented Mar 6, 2019

pizi-nordic commented Mar 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pizi-nordic commented Sep 25, 2019 • edited Loading

pabigot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pizi-nordic commented Sep 27, 2019

pabigot commented Sep 27, 2019

pabigot commented Oct 1, 2019

pabigot commented Oct 1, 2019

galak commented Oct 3, 2019

pabigot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pizi-nordic commented Mar 4, 2019 •

edited

Loading

zephyrbot commented Mar 4, 2019 •

edited

Loading

pizi-nordic commented Sep 25, 2019 •

edited

Loading