golioth / golioth-zephyr-sdk

Golioth SDK For Zephyr
https://www.golioth.io
Apache License 2.0
67 stars 19 forks source link

sockets_offload_poll_wrapper: introduce new module and use it by nRF91 #298

Closed mniestroj closed 1 year ago

mniestroj commented 1 year ago

There is a special ZFD_IOCTL_POLL_OFFLOAD ioctl() request used inside poll() system call handling, which allows to offload whole poll() system call to driver offloading sockets operations (like NCS' nRF91 sockets driver). Offloading whole poll() execution doesn't allow to use Zephyr native sockets (either from another network driver or just software concepts like socketpair and eventfd). This is a huge restriction, because it prevents "interrupting" poll() system call and blocks thread calling poll() until new data arrives (at least when using POLLIN event).

Introduce offloaded sockets wrapper, which wraps all socket operations with special behavior added specifically in poll() system call handling. ZFD_IOCTL_POLL_PREPARE ioctl() request is handled (instead of returning -EXDEV by offloaded sockets drivers) and execution of offloaded poll() is actually scheduled in helper (one per socket) thread. Potentially infinite (in case of no data arriving on socket) execution of poll() just makes the helper thread to sleep, while the application thread is not affected. Application thread is still notified about POLLIN event, by raising k_poll_signal by helper thread. Such k_poll_signal event notification is added next to other native Zephyr notification mechanisms (like semaphore, fifo, msgq, pipe) to k_poll() doing the heavylifting in poll() implementation. This means that such wrapped poll() handling allows to combine it with other Zephyr sockets, eliminating major restriction of "offloaded poll" mechanism.

Limitations of current implementation:

Use of this module allows to utilize eventfd mechanism, which is used by all other platforms. For now this change should not result in functional changes (this is just implementation mechanism change). However 'eventfd' will be the requirement with the upcoming CoAP rework.

github-actions[bot] commented 1 year ago

Visit the preview URL for this PR (updated for commit b4eee79):

https://golioth-zephyr-sdk-doxygen-dev--pr298-golioth-lightdb-32ewaknq.web.app

(expires Fri, 14 Oct 2022 11:10:37 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

lemrey commented 1 year ago

Hello @mniestroj haven't had a thorough look at this yet. It seems to address the fact that poll in Zephyr can't be used on both Zephyr sockets and nRF91 sockets. Do I understand you'd like to leverage eventfd and similar mechanisms to interrupt poll() calls on Zephyr and offloaded sockets alike? If so, would it help with your use-case if nrf_poll supported callbacks for the various POLLIN etc events, that could be set via a dedicated socket option? I guess that could be used the extend the code in the offloading implementation for nR91 to support your use case.

mniestroj commented 1 year ago

If so, would it help with your use-case if nrf_poll supported callbacks for the various POLLIN etc events, that could be set via a dedicated socket option? I guess that could be used the extend the code in the offloading implementation for nR91 to support your use case.

I am not sure what you mean by such callback. Could you provide some example (could be pseudo-code) that demonstrates that?

IMO here should be some mechanism (other than timeout that is passed as argument) to trigger nrf_poll() to finish it's execution before data is received/sent on nRF socket. Ideally, there should be some mechanism implemented in low-level nRF91 sockets drivers to implement Zephyr's k_poll(), so that nRF91 sockets would support Zephyr poll() and not the "offloaded" variant of poll() (which cannot wait on generic Zephyr sockets at the same time as it waits on nRF sockets events). Looking at the upstream Zephyr driver it looks like all the drivers (which use offloaded sockets) could actually implement Zephyr poll(), as they wait on a generic Zephyr object like k_sem or something similar. Right now the only exception is nRF91 sockets implementation, which requires offloaded poll() implementation and hence does not support passing other sockets (e.g. from other network interfaces, or eventfd file descriptors).

lemrey commented 1 year ago

You could take a look at this commit perhaps. Let me know if this would work for you and happy holidays!

mniestroj commented 1 year ago

It seems exactly what I need! Keep me posted when you have something and I'll test it with my use case.

Happy holidays!

lemrey commented 1 year ago

Hello @mniestroj, it should be possible to do what you need now in ncs/main.

mniestroj commented 1 year ago

@lemrey Thanks, I'll try it out soon!

mniestroj commented 1 year ago

@lemrey Thanks for your development. Last week I tried NCS 2.3.0 and the way nRF91 sockets poll() is handled now is exactly what we needed. Works great, thanks once again!