rtio: Introduce OP_DELAY as a valid SQE operation

[ubieda]

Description

This PR introduces OP_DELAY operation, which allows SQE items to take a specified amount of time (asynchronously) before completing. This allows to serve as an asynchronous delay in between SQE items (e.g: A sensor measurement requested, which requires 50-ms before having the result available).

Fixes #77100

Testing

A test-case has been added under the rtio_api testsuite. I ran this test on native_sim, and passes all checks.

[bjarki-andreasen]

Have you considered using a struct _timeout and storing it with the sqe? the kernel already does the scheduling, and the struct is only 16-20 bytes depending on if 64 bit timeout is used. This would save us from reinventing the kernel's timeout.c with rtio_sched.c We can even squeze a full k_timeout in there like this:

		/** OP_DELAY */
		struct {
                        union {
                                k_timeout_t timeout; /**<< Copied when adding the timeout */
                                struct _timeout to; /**<< Timeout structure */
		        };
                } delay;

with this, rtio_sched_alarm() (renamed to rtio_delay()) becomes something like:

static void rtio_delay_to_handler(struct _timeout *to)
{
	struct rtio_iodev_sqe *iodev_sqe = CONTAINER_OF(to, struct rtio_iodev_sqe, delay.to);

        rtio_iodev_sqe_ok(iodev_sqe, 0);
}

void rtio_delay(struct rtio_iodev_sqe *iodev_sqe)
{
        /* copy since struct _timeout occupies the same memory as the timeout */
        k_timeout_t timeout = rtio_iodev_sqe->delay.timeout;

        z_add_timeout(&rtio_iodev_sqe->delay.to, timeout);
}

I really don't think we should reimplement this when the kernel already provides it for "free".

[ubieda]

@bjarki-andreasen Thanks for your feedback. TBH - I had not considered using the z_*_timeout APIs, only the k_timer APIs. I agree tapping into the Kernel timeout scheduling simplifies things and better and in all. I've refactored this PR to incorporate this suggestion.

However, seems like the timeout_q APIs are not currently meant to be used outside the kernel primitives. I had to cheat with an include (#include <../kernel/include/timeout_q.h>) in order to make it work but it is likely we'll need to expose this through public APIs before settling this PR.

WRT renaming, I'd prefer to stick with rtio_sched.h as this very same functionality could also be applied to other purposes (e.g: #76970)

[bjarki-andreasen]

@bjarki-andreasen Thanks for your feedback. TBH - I had not considered using the z_*_timeout APIs, only the k_timer APIs. I agree tapping into the Kernel timeout scheduling simplifies things and better and in all. I've refactored this PR to incorporate this suggestion.

However, seems like the timeout_q APIs are not currently meant to be used outside the kernel primitives. I had to cheat with an include (#include <../kernel/include/timeout_q.h>) in order to make it work but it is likely we'll need to expose this through public APIs before settling this PR.

#include <timeout_q.h> will work :) I do plan to suggest using it more widely, k_timer is overkill for pretty much all kernel code including subsystems...

WRT renaming, I'd prefer to stick with rtio_sched.h as this very same functionality could also be applied to other purposes (e.g: #76970)

Sure thing :)

[bjarki-andreasen]

removing the timeout if SQE is cancelled should be added somewhere :)

[ubieda]

I looked into this, it seems we don't need special handling for canceled OP_DELAY SQEs:

• If it was canceled before the SQE is processed, it won't get to this point.
• If it's canceled after the Timeout is scheduled, the timer expiration will not generate a CQE due to the item simply being canceled already.

I had added an additional test-case but it already passes with no code changes added:

ZTEST(rtio_api, test_rtio_delay_canceled)
{
	int res;
	struct rtio *r = &r_delay;
	struct rtio_sqe *sqe;
	struct rtio_cqe *cqe;

	sqe = rtio_sqe_acquire(r);
	zassert_not_null(sqe, "Expected a valid sqe");

	rtio_sqe_prep_delay(sqe, K_SECONDS(10), NULL);

	res = rtio_submit(r, 0);
	zassert_ok(res, "Should return ok from rtio_execute");

	rtio_sqe_cancel(sqe);
	k_sleep(K_SECONDS(10));

	cqe = rtio_cqe_consume(r);
	zassert_null(cqe, "Canceled SQEs should not generate a CQE");
	rtio_cqe_release(r, cqe);
}

[bjarki-andreasen]

no need to pass the timeout as a param, just read the data directly

[ubieda]

#include <timeout_q.h> will work :)

I just gave it a try and it's unable to find it. See the output below (including diff and build output):

(.venv) ubieda@ubieda-pc:~/zephyrproject/zephyr$ git diff
diff --git a/include/zephyr/rtio/rtio.h b/include/zephyr/rtio/rtio.h
index c0277b1ab23..f1169d7bb04 100644
--- a/include/zephyr/rtio/rtio.h
+++ b/include/zephyr/rtio/rtio.h
@@ -38,7 +38,7 @@
 #include <zephyr/sys/iterable_sections.h>
 #include <zephyr/sys/mpsc_lockfree.h>
 
-#include <../kernel/include/timeout_q.h>
+#include <timeout_q.h>
 
 #ifdef __cplusplus
 extern "C" {

(.venv) ubieda@ubieda-pc:~/zephyrproject/zephyr$ west build -b native_sim -t run tests/subsys/rtio/rtio_api/
-- west build: running target run
[1/8] Building C object zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_executor.c.obj
FAILED: zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_executor.c.obj 
ccache /usr/bin/gcc -DKERNEL -DK_HEAP_MEM_POOL_SIZE=0 -D__ZEPHYR__=1 -I/home/ubieda/zephyrproject/zephyr/build/zephyr/include/generated/zephyr -I/home/ubieda/zephyrproject/zephyr/include -I/home/ubieda/zephyrproject/zephyr/build/zephyr/include/generated -I/home/ubieda/zephyrproject/zephyr/soc/native/inf_clock -I/home/ubieda/zephyrproject/zephyr/boards/native/native_sim -I/home/ubieda/zephyrproject/zephyr/scripts/native_simulator/common/src/include -I/home/ubieda/zephyrproject/zephyr/scripts/native_simulator/native/src/include -I/home/ubieda/zephyrproject/zephyr/subsys/rtio -I/home/ubieda/zephyrproject/zephyr/subsys/testsuite/include -I/home/ubieda/zephyrproject/zephyr/subsys/testsuite/coverage -I/home/ubieda/zephyrproject/zephyr/subsys/testsuite/ztest/include -Wshadow -fno-strict-aliasing -Os -imacros /home/ubieda/zephyrproject/zephyr/build/zephyr/include/generated/zephyr/autoconf.h -fno-common -g -gdwarf-4 -fdiagnostics-color=always -Wall -Wformat -Wformat-security -Wno-format-zero-length -Wdouble-promotion -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-unused-but-set-variable -Werror=implicit-int -fno-pic -fno-pie -fno-asynchronous-unwind-tables -fno-reorder-functions --param=min-pagesize=0 -fno-defer-pop -fmacro-prefix-map=/home/ubieda/zephyrproject/zephyr/tests/subsys/rtio/rtio_api=CMAKE_SOURCE_DIR -fmacro-prefix-map=/home/ubieda/zephyrproject/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/home/ubieda/zephyrproject=WEST_TOPDIR -ffunction-sections -fdata-sections -m32 -msse2 -mfpmath=sse -fvisibility=hidden -fno-freestanding -std=c11 -MD -MT zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_executor.c.obj -MF zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_executor.c.obj.d -o zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_executor.c.obj -c /home/ubieda/zephyrproject/zephyr/subsys/rtio/rtio_executor.c
In file included from /home/ubieda/zephyrproject/zephyr/subsys/rtio/rtio_executor.c:7:
/home/ubieda/zephyrproject/zephyr/include/zephyr/rtio/rtio.h:41:10: fatal error: timeout_q.h: No such file or directory
   41 | #include <timeout_q.h>
      |          ^~~~~~~~~~~~~
compilation terminated.
[2/8] Building C object zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_init.c.obj
FAILED: zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_init.c.obj 
ccache /usr/bin/gcc -DKERNEL -DK_HEAP_MEM_POOL_SIZE=0 -D__ZEPHYR__=1 -I/home/ubieda/zephyrproject/zephyr/build/zephyr/include/generated/zephyr -I/home/ubieda/zephyrproject/zephyr/include -I/home/ubieda/zephyrproject/zephyr/build/zephyr/include/generated -I/home/ubieda/zephyrproject/zephyr/soc/native/inf_clock -I/home/ubieda/zephyrproject/zephyr/boards/native/native_sim -I/home/ubieda/zephyrproject/zephyr/scripts/native_simulator/common/src/include -I/home/ubieda/zephyrproject/zephyr/scripts/native_simulator/native/src/include -I/home/ubieda/zephyrproject/zephyr/subsys/rtio -I/home/ubieda/zephyrproject/zephyr/subsys/testsuite/include -I/home/ubieda/zephyrproject/zephyr/subsys/testsuite/coverage -I/home/ubieda/zephyrproject/zephyr/subsys/testsuite/ztest/include -Wshadow -fno-strict-aliasing -Os -imacros /home/ubieda/zephyrproject/zephyr/build/zephyr/include/generated/zephyr/autoconf.h -fno-common -g -gdwarf-4 -fdiagnostics-color=always -Wall -Wformat -Wformat-security -Wno-format-zero-length -Wdouble-promotion -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-unused-but-set-variable -Werror=implicit-int -fno-pic -fno-pie -fno-asynchronous-unwind-tables -fno-reorder-functions --param=min-pagesize=0 -fno-defer-pop -fmacro-prefix-map=/home/ubieda/zephyrproject/zephyr/tests/subsys/rtio/rtio_api=CMAKE_SOURCE_DIR -fmacro-prefix-map=/home/ubieda/zephyrproject/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/home/ubieda/zephyrproject=WEST_TOPDIR -ffunction-sections -fdata-sections -m32 -msse2 -mfpmath=sse -fvisibility=hidden -fno-freestanding -std=c11 -MD -MT zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_init.c.obj -MF zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_init.c.obj.d -o zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_init.c.obj -c /home/ubieda/zephyrproject/zephyr/subsys/rtio/rtio_init.c
In file included from /home/ubieda/zephyrproject/zephyr/subsys/rtio/rtio_init.c:7:
/home/ubieda/zephyrproject/zephyr/include/zephyr/rtio/rtio.h:41:10: fatal error: timeout_q.h: No such file or directory
   41 | #include <timeout_q.h>
      |          ^~~~~~~~~~~~~
compilation terminated.
[3/8] Building C object zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_sched.c.obj
FAILED: zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_sched.c.obj 
ccache /usr/bin/gcc -DKERNEL -DK_HEAP_MEM_POOL_SIZE=0 -D__ZEPHYR__=1 -I/home/ubieda/zephyrproject/zephyr/build/zephyr/include/generated/zephyr -I/home/ubieda/zephyrproject/zephyr/include -I/home/ubieda/zephyrproject/zephyr/build/zephyr/include/generated -I/home/ubieda/zephyrproject/zephyr/soc/native/inf_clock -I/home/ubieda/zephyrproject/zephyr/boards/native/native_sim -I/home/ubieda/zephyrproject/zephyr/scripts/native_simulator/common/src/include -I/home/ubieda/zephyrproject/zephyr/scripts/native_simulator/native/src/include -I/home/ubieda/zephyrproject/zephyr/subsys/rtio -I/home/ubieda/zephyrproject/zephyr/subsys/testsuite/include -I/home/ubieda/zephyrproject/zephyr/subsys/testsuite/coverage -I/home/ubieda/zephyrproject/zephyr/subsys/testsuite/ztest/include -Wshadow -fno-strict-aliasing -Os -imacros /home/ubieda/zephyrproject/zephyr/build/zephyr/include/generated/zephyr/autoconf.h -fno-common -g -gdwarf-4 -fdiagnostics-color=always -Wall -Wformat -Wformat-security -Wno-format-zero-length -Wdouble-promotion -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-unused-but-set-variable -Werror=implicit-int -fno-pic -fno-pie -fno-asynchronous-unwind-tables -fno-reorder-functions --param=min-pagesize=0 -fno-defer-pop -fmacro-prefix-map=/home/ubieda/zephyrproject/zephyr/tests/subsys/rtio/rtio_api=CMAKE_SOURCE_DIR -fmacro-prefix-map=/home/ubieda/zephyrproject/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/home/ubieda/zephyrproject=WEST_TOPDIR -ffunction-sections -fdata-sections -m32 -msse2 -mfpmath=sse -fvisibility=hidden -fno-freestanding -std=c11 -MD -MT zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_sched.c.obj -MF zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_sched.c.obj.d -o zephyr/subsys/rtio/CMakeFiles/subsys__rtio.dir/rtio_sched.c.obj -c /home/ubieda/zephyrproject/zephyr/subsys/rtio/rtio_sched.c
In file included from /home/ubieda/zephyrproject/zephyr/subsys/rtio/rtio_sched.c:8:
/home/ubieda/zephyrproject/zephyr/include/zephyr/rtio/rtio.h:41:10: fatal error: timeout_q.h: No such file or directory
   41 | #include <timeout_q.h>
      |          ^~~~~~~~~~~~~
compilation terminated.
ninja: build stopped: subcommand failed.
FATAL ERROR: command exited with status 1: /usr/bin/cmake --build /home/ubieda/zephyrproject/zephyr/build --target run

[ubieda]

Pushed to fix the CI failures. I'd like to get it green so I mark the PR ready for reviewing.

[teburd]

I'm game for this, but its going to need some scrutiny and/or making it less private and/or moving rtio into the kernel part of the tree perhaps (though this will bring yet more scrutiny to this code).

I like the concept though, as noted timeouts already do the shortest time queuing and are well tested. So makes good sense to reuse as @bjarki-andreasen noted, the concern before was the size cost.

[bjarki-andreasen]

strange regarding #include <timeout_q.h>m its included like that in other places in tree :)

[teburd]

strange regarding #include <timeout_q.h>m its included like that in other places in tree :)

likely only in kernel/ where the include path is added?

This is really a private kernel API which is why its there, and to some degree we are creating a crucial IPC primitive here... so maybe rtio just needs to live closer to the kernel or something, or maybe this would be useful to expose for this sort of thing better. I've added some kernel maintainers and @nashif to provide input on this.

Just to give some high level notes...

• k_timer is kind of big for this I think
• We could do this ourselves with our own timer queue, but the kernel has one already so why would we want to reinvent
• The kernel timer queue is somewhat private hidden by k_timer, k_sleep, and various timeout parameters to kernel IPC primitives
• RTIO is itself a IPC primitive of sorts here so we'd like to get the same access I think as say a k_semaphore, without living in kernel/ maybe

[ubieda]

FWIW - I'll move the kernel header to the .c file so it's more hidden. It certainly does not have to be in rtio.h

[ubieda]

Moved include directive to rtio_sched.c as mentioned above. Also, added a comment-block explaining why we're not going instead with k_timers.

[ubieda]

Rebased to address misspelling on this comment block:

/** Required to access Timeout Queue APIs, which are used instead of the
 * Timer APIs because of concerns on size on rtio_sqe (k_timer is more
 * than double the size of _timeout). Users will have to instantiate a
 * pool of SQE objects, thus its size directly impacts memory footprint
 * of RTIO applications.
 */
#include <../kernel/include/timeout_q.h>

[ubieda]

Rebased to resolve conflicts since #87888 landed