Skip to content
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

Enable SimpleLink socket blocking/non-blocking mode to be set via fcntl() #11891

Closed
GAnthony opened this issue Dec 5, 2018 · 8 comments
Closed
Labels
area: Sockets Networking sockets area: Wi-Fi Wi-Fi Enhancement Changes/Updates/Additions to existing features

Comments

@GAnthony
Copy link
Collaborator

GAnthony commented Dec 5, 2018

In Zephyr (and for POSIX sockets), setting blocking/non-blocking mode for TCP sockets is done using fcntl():

opts = fcntl(sock, F_GETFL);
opts = (opts | O_NONBLOCK);
fcntl(sock, F_SETFL,opts);

However, the SimpleLink host driver currently used in by the cc3220sf WiFi socket offload driver only allows setting of the blocking mode via its setsockopt() API.

This fix is needed for the samples/net/sockets/echo_async sample program to work, as well as other future networking apps.

A possible solution:
Add a new fcntl() function field in the socket_offload_ops.h:struct socket_offload structure, and implement that fcntl function in simplelink_sockets.c to call sl_SetSockOpt().

@GAnthony GAnthony added area: Sockets Networking sockets area: Wi-Fi Wi-Fi labels Dec 5, 2018
@GAnthony GAnthony added this to the v1.14.0 milestone Dec 5, 2018
@GAnthony GAnthony added the Enhancement Changes/Updates/Additions to existing features label Dec 5, 2018
@pfalcon
Copy link
Contributor

pfalcon commented Dec 12, 2018

Add a new fcntl() function field in the socket_offload_ops.h:struct socket_offload

@GAnthony, #11438 introduced struct socket_op_vtable, https://github.com/zephyrproject-rtos/zephyr/pull/11438/files#diff-ade678982c222392003232afd1e25cd8R33 , it's supposed to supersede struct socket_offload.

Do you transfer socket offload maintainership to Vincent? Can you introduce him to these matters and invite to the discussions in these tickets?

@GAnthony
Copy link
Collaborator Author

@pfalcon, thanks for your comment.

Do you transfer socket offload maintainership to Vincent? Can you introduce him to these matters and invite to the discussions in these tickets?

Yes to both.
Note: Vincent is currently in "ramp-up" mode on Zephyr. Going through getting started docs, etc.

I recommended updating the socket_offload struct with fcntl() as a solution for a few reasons:

  1. There will be some ramp-up curve on Zephyr, and taking the approach in sockets: tls: Make secure sockets support posix APIs #11438 would be a bit more advanced, and in my view, unnecessary as a first step to getting fcntl() working for socket offload, which is a solution that already works;
  2. The socket offload framework was just merged in Oct 2018, and just two months later already talking about changing it to a new approach, which itself may change again in a few months given the need to incorporate more socket types (i.e., PF_CAN);
  3. Most networking protocols I've seen prefer to use socket APIs rather than read/write/ioctl, so the switch to fd_table isn't that urgent in my view, and can be done as a lower priority enhancement.

So, my suggestion would be to update the socket_offload_ops first; then start to move socket offload over to the new socket offload point (whatever that will be in a few months time).

@GAnthony
Copy link
Collaborator Author

Do you transfer socket offload maintainership to Vincent?

To clarify a bit more, we are in the process of such 'transfer of ownership', but I would say this month is mainly taken up by "ramp-up" activities (oh, and I guess a bit of holiday time). For this month, I should probably still be considered as "owner".

@pfalcon
Copy link
Contributor

pfalcon commented Dec 13, 2018

and in my view, unnecessary as a first step to getting fcntl() working for socket offload, which is a solution that already works;

As long as we accept that it's going to be "wasted work" in the end, yes ;-).

The socket offload framework was just merged in Oct 2018, and just two months later already talking about changing it to a new approach,

Well, it was merged in 2018-10, it exists as an implementation for much longer than that, and from day 1, a concern was raised that a proper offloading implementation should not hog the entire socket subsys for itself, there should be a way for different socket impls to co-exists, and #11438 implements just that.

which itself may change again in a few months given the need to incorporate more socket types (i.e., PF_CAN);

#11438 is exactly the answer to those challenges. We of course may need to tweak things (more so specifically for re-incorporating offloading features in that framework), but otherwise it already has laid the ground where there's a framework for integrating different socket types under a common API.

Most networking protocols I've seen prefer to use socket APIs rather than read/write/ioctl

Again, that's not the scope #11438, it extended fdtable approach as implemented previously to extend beyond just file operations (read/write/ioctl) to all individual socket operations (poll/select aren't directly addressed, yeah). It effectively pushed what offloading did a level up in the architecture, with support for multiple such "offloadings". Even native plain Zephyr sockets are now "offloaded" to their own implementation.

So, my suggestion would be to update the socket_offload_ops first;

Sounds good to me, given that conversion to sock_op_table probably won't fit into 1.14. Otherwise, wanted to share the perspective, and surely, things may change, there's e.g. cleanup/elaboration to do for that infra.

@rlubos
Copy link
Contributor

rlubos commented Jan 15, 2019

Hello @GAnthony

Is there any work in progress regarding adding support to fcntl() offload? We've recently faced the same issue at Nordic with our offloaded sockets, that we need a way to set a socket into non-blocking mode. If noone is working on this and we agree to extend socket_offload_ops for starters I think I could look into this, it sounds like a pretty straightforward extension.

I agree with @pfalcon that eventually we should switch to the extended fdtable with socket offloading, but there's no need to rush things IMO, let's allow the API to settle.

@pfalcon
Copy link
Contributor

pfalcon commented Jan 15, 2019

I agree with @pfalcon that eventually we should switch to the extended fdtable with socket offloading, but there's no need to rush things IMO, let's allow the API to settle.

IMHO, with such approach we just do extra legwork and accumulate technical debt (which we have a lot already).

@GAnthony
Copy link
Collaborator Author

Is there any work in progress regarding adding support to fcntl() offload?
@rlubos, not yet.
It's on the list, but we're currently working other priorities.
Getting that done would definitely help!
We would basically be resolving that fcntl() call into an offloaded setsockopt() call.

@pfalcon
Copy link
Contributor

pfalcon commented Jan 15, 2019

that fcntl() call into an offloaded setsockopt() call

Note that one idea I have is to make ioctl(FIONBIO) to be primary means to set non-blocking mode, and route fcntl() via it. More background: https://stackoverflow.com/questions/1150635/unix-nonblocking-i-o-o-nonblock-vs-fionbio

@nashif nashif removed this from the v1.14.0 milestone Mar 4, 2019
vanti added a commit to vanti/zephyr that referenced this issue Mar 14, 2019
This commit implements fcntl() in the SimpleLink Wifi driver to set and
get the non-blocking mode on a socket.

Fixes zephyrproject-rtos#11891.

Signed-off-by: Vincent Wan <vincent.wan@linaro.org>
@galak galak closed this as completed in a699557 Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Sockets Networking sockets area: Wi-Fi Wi-Fi Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

No branches or pull requests

4 participants