net: sockets: Rework socket offloading #21209

rlubos · 2019-12-05T16:06:46Z

This PR is an attempt to unify offloaded socket engines underneath the zsock_* API. Opening as a draft PR to trigger some discussion. If we agree that it's the way we want to go, I can rework remaining offloading users (eswifi only left I believe) and proceed with a regular PR.

Currently, we've ended up with two different vtables for socket API - one for native socket solutions, other for the offloaded ones. It's not only cumbersome in maintenance (for example, the "native" vtable got extended with sendmsg function recently, which is missing in the offloaded one) but also has its limitations (as mentioned in #18106, only one offloaded interface can be registered and it cannot be used along with native stack).

In this approach, I propose to drop the existing offloaded socket vtable, switch to native one instead and re-use NET_SOCKET_REGISTER macro for offloaded sockets registration. There are some caveats for this approach though:

Some offloaded engines (nRF91, SimpleLink) offload entire poll operation. I found it difficult to integrate them with poll solution for native sockets. Therefore, I propose to extend the list of functions registered along with socket engine, with custom poll and a helper function which determines whether custom poll should be used or not for that specific poll API call.
getaddrinfo/freeaddrinfo are not really related to specific sockets, yet we offload them as nRF91 and SimpleLink provide offloaded DNS operations. I didn't have a good idea how to tackle this issue, so I've left it as it was - offloaded socket implementation can register custom DNS provider during registration. I'm open for a better proposal.
Socket creation is controlled by an is_supported function, provided during socket engine registration. For now, offloaded socket engines I've converted return true in any case, meaning it offloading will always take preference over native stack. A more sophisticated logic can be added though at per-implementation basis, to control this function behavior.

@vanti @mike-scott I've taken the liberty and reworked SimpleLink and ublox-sara-r4 sockets. Unfortunately I have no hardware to test the changes, all I could do was to guarantee that the new solution build for these platforms. As for the overall solution, I've tested it with nRF91 offloaded sockets, unfortunately, it's not available in the Zephyr tree.

@vanti SimpleLink was pretty similar to the nRF91 sockets so I hope I did not mess it up. Please verify at convenience.

@mike-scott ublox-sara-r4 was a little trickier, as it already used Zephyr file descriptors. I tried to pay extra attention to make sure the changes I've made make sense, but I would appreciate if you could verify this solution as well.

zephyrbot · 2019-12-05T16:14:46Z

All checks are passing now.

checkpatch (informational only, not a failure)

-:1628: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#1628: FILE: include/posix/unistd.h:44:
+unsigned sleep(unsigned int seconds);

- total: 0 errors, 1 warnings, 1681 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

pfalcon · 2019-12-06T10:13:23Z

@rlubos

In this approach, I propose to drop the existing offloaded socket vtable, switch to native one instead

Thanks for bringing this up, and starting work on that! That's exactly what we discussed with @vanti in September, and while we both agreed that it's a nice idea, we didn't come forward with a specific plan, as he was busy with other things and sought for managerial approval of taking that load, and I didn't have any offloading hardware to test it. I didn't even submitted tickets on that, given that there already a lot of tickets with great ideas, sitting stale.

So, I'm very much in favor of this refactor. While we're in this pre-release limbo, I switched to other tasks, so leaving just this quick comment, will look in details later. But while pondering about this, I had 2 ideas:

To make even "default" socket implementation to be pluggable, in the same way as any other. This way, we can plug sockets (and associated network (sub)stacks) in a consistent, natural way, without adhoc handling of the "native" implementation.
But then, we'd need a kind of priority for different socket handlers, to make sure that offloading handler had a chance to process socket() request before "default" (impl).
(Oh, let me add this too, to explore the bounds of it. To make different impls truly coexists, we should be aware that it may be not possible to select socket handler on socket() call. It might be needed to be postponed until bind() or connect() calls. While supporting that is definitely as separate task with their own would-be-stakeholders and requirements, we might want to think that architecture and implementation is supportive of such future extension, not obstructing it).

vanti

Thanks @rlubos. As @pfalcon mentioned we discussed about this before, but I never found the time to put energy into it, so am glad you are taking this on.

I tried the echo sample on TI CC3220, and saw at least two issues thus far:

simplelink_accept() needs to finalize the client fd similar to how simplelink_socket_create() does it.
The TI SimpleLink WiFi host driver does not support NULL address and address length pointers in recvfrom and sendto. zsock_send/zsock_recv seem to rely on this mechanism in its implementation. I put together a couple of workarounds to get the echo sample to work.

Here are the changes that worked for me:
vanti@3fcbf23
vanti@f36f2c7

Feel free to take them and apply any tweaks if necessary.

rlubos · 2019-12-09T08:50:00Z

Thanks for the feedback. I understand that everyone is busy with the release and I don't think there's any rush with this PR, we should proceed with it carefully. Especially that this PR reworks a feature that already has quite a few users. I'm happy to see though that we all agree it's a desired change.

@pfalcon Regarding your comments, I see that we have quite similar thoughts:

I totally agree, this approach feels the most natural with the current design. We'd end up with a separate dispatching layer, and a set of socket implementations, all integrated in a consistent way.
A natural consequence of 1., agreed we'd need some kind of prioritization.
This one I have actually not thought of so I don't have much to comment. I'm open though for any suggestions in this PR, if something needs to be reworked to make it possible in the future.

@vanti Thank you for your time testing (and fixing) the SimpleLink! I had to switch to some other task right now, I'll integrate your fixes once I'm done.
I'm glad you've caught the issue with accept - I haven't spotted it with nRF91 as I only tested client apps. Regarding recv/send, perhaps it would make sense to extend the zsock vtable? Or we can just stick to the workaround if you think it's good enough. I'm open for discussion here.

jukkar · 2019-12-09T13:26:11Z

3\. (Oh, let me add this too, to explore the bounds of it. To make different impls truly coexists, we should be aware that it may be not possible to select socket handler on socket() call. It might be needed to be postponed until bind() or connect() calls. While supporting that is definitely as separate task with their own would-be-stakeholders and requirements, we might want to think that architecture and implementation is supportive of such future extension, not obstructing it).

This might be tricky to do. I investigated this when implementing the socket registration code and it is quite difficult to postpone things done by socket() to when bind() is called.

vanti

@vanti Thank you for your time testing (and fixing) the SimpleLink! I had to switch to some other task right now, I'll integrate your fixes once I'm done.
I'm glad you've caught the issue with accept - I haven't spotted it with nRF91 as I only tested client apps. Regarding recv/send, perhaps it would make sense to extend the zsock vtable? Or we can just stick to the workaround if you think it's good enough. I'm open for discussion here.

Extending 'socket_op_vtable' with extra send and recv functions is an interesting idea. It would allow these functions to be implemented with the corresponding SimpleLink APIs as opposed to relying on the SimpleLink recvfrom/sendto implementations, simliar to how they were implemented originally. However, there is still a need for the workaround, as users may wish to directly call recvfrom/sendto with NULL address pointers.

This does give me an idea to simplify the workaround by using SimpleLink's equivalent for send/recv. I will look into it and update my commit later.

Note that I have only tested the echo example thus far, to give us an idea of how well this works on SimpleLink. More testing is needed to ensure correctness in the implementation.

vanti

Did some more testing and found another issue in simplelink_poll(). After fixing that and cleaning up the commit on recvfrom/sendto, I am successful in running the network samples supported by CC32xx.

Here are the updated commits (the top 3): https://github.com/vanti/zephyr/commits/offloading-rework-review

vanti · 2019-12-13T19:12:52Z

drivers/wifi/simplelink/simplelink_sockets.c

-		for (i = 0; i < (max_fd + 1); i++) {
-			if (SL_SOCKET_FD_ISSET(fds[i].fd, &rfds)) {
+		for (i = 0; i < nfds; i++) {
+			fd = simplelink_offload_fd[fds[i].fd];


Need to check that fds[i].fd is not negative here. I have submitted a fix in my branch to fix this:
vanti@17d2a28

vanti · 2019-12-13T19:44:39Z

drivers/wifi/simplelink/simplelink_sockets.c

 			continue;
+		} else if ((fds[i].fd >= ARRAY_SIZE(simplelink_offload_fd)) ||
+			   (simplelink_offload_fd[fds[i].fd] == -1)) {
+			/* Non-offloaded socket, return an error. */


Not sure how this would work when poll() is called on a set that contains both offloaded and non-offloaded sockets. The SimpleLink implementation sl_Select() only deals with "SimpleLink sockets", so there isn't an easy way to wait on all sockets at once.

This approach doesn't allow to mix up socket types, that's the limitation of offloading the entire poll call.

For native sockets, we also have a less restrictive approach for poll, utilizing ZFD_IOCTL_POLL_PREPARE and ZFD_IOCTL_POLL_UPDATE ioctl requests. In this approach, every registered socket provider can provide a kernel object to observe with ZFD_IOCTL_POLL_PREPARE handler, and then update the poll results in ZFD_IOCTL_POLL_UPDATE handler. This allows to provide different socket types for a single poll call.

If the offloaded socket implementation could easily provide such kernel objects, it's free to use this approach. For nRF/Simplelink though I found this difficult, simply because these engines offload entire poll/select calls.

This approach doesn't allow to mix up socket types, that's the limitation of offloading the entire poll call.

I'd formulate it a bit differently: this approach leaves handling of mixed socket types to the socket (offloading) handler. And the baseline of such handling would be of course to not allow and error out with appropriate error code. I just starting with a detailed review, but I hope we'll take that as a baseline indeed to check for mixed case and return error, instead of not handling it in explicit manner.

If the offloaded socket implementation could easily provide such kernel objects, it's free to use this approach. For nRF/Simplelink though I found this difficult, simply because these engines offload entire poll/select calls.

For the purpose of discussion: the whole idea of poll() is to wait for async event on sockets. Hardware-wise, async events are implemented by interrupts. Which in turn translates to a callback at the lowest software level. So, the minimum requirement to support baseline-efficient handling of mixed socket types would be: an underlying offloading impl to provide means to register an "even callback". Just one callback, not per-socket. Of course filtering of event types is helpful. Then on Zephyr side of things, this callback would signal an event or semaphore object. And ZFD_IOCTL_POLL_PREPARE would return this single event/sema for every offloaded socket. And then, after getting an event, ZFD_IOCTL_POLL_UPDATE will take care of figuring out which socket(s) this event actually affected.

So, it shouldn't be that too hard to support mixed sockets if underlying provider supports the suitable low-level interface. Obviously, I don't for implementation of that any time soon. But it's helpful to discuss approaches to that, to validate our design.

rlubos · 2019-12-16T08:32:02Z

Thanks @vanti, I'll try to update my branch with your changes today or tomorrow.

rlubos · 2019-12-16T13:41:29Z

@vanti I've squashed your fixes into the commit reworking SimpleLink.

pfalcon · 2019-12-26T11:19:02Z

include/net/socket.h

@@ -788,17 +788,22 @@ struct net_socket_register {
 	int family;
 	bool (*is_supported)(int family, int type, int proto);
 	int (*handler)(int family, int type, int proto);
+	bool (*poll_takeover)(struct zsock_pollfd *fds, int nfds);


I guess we should start adding comments (not necessarily doxygen comments). From mere name, I can't exactly imagine what a method called "poll_takeover" would do. And looking at the signature, I'm not even sure that "poll_takeover" is a good name for such a method.

pfalcon · 2019-12-26T11:21:49Z

subsys/net/lib/sockets/sockets.c

+			continue;
+		}
+
+		if (!sock_family->poll_takeover(fds, nfds)) {


Ok, so now seeing "poll_takeover" in action, and obvious question: why do we need both poll_takeover and custom_poll?

Oh, and this looks quite inefficient of course. Did you consider extending existing ioctl ZFD_IOCTL_POLL_PREPARE protocol? Actually, I would say that you should consider it. Like, add a special error return code meaning "stop right here, and handover all the poll list to me (offloading)".

I just wanted to separate the decision making from the actual poll handling. But indeed, having the decision done via the ioctl's ZFD_IOCTL_POLL_PREPARE sounds very sane, I'll rework this.

pfalcon · 2019-12-26T12:24:35Z

drivers/wifi/simplelink/simplelink_sockets.c

 			continue;
+		} else if ((fds[i].fd >= ARRAY_SIZE(simplelink_offload_fd)) ||
+			   (simplelink_offload_fd[fds[i].fd] == -1)) {
+			/* Non-offloaded socket, return an error. */


This approach doesn't allow to mix up socket types, that's the limitation of offloading the entire poll call.

I'd formulate it a bit differently: this approach leaves handling of mixed socket types to the socket (offloading) handler. And the baseline of such handling would be of course to not allow and error out with appropriate error code. I just starting with a detailed review, but I hope we'll take that as a baseline indeed to check for mixed case and return error, instead of not handling it in explicit manner.

If the offloaded socket implementation could easily provide such kernel objects, it's free to use this approach. For nRF/Simplelink though I found this difficult, simply because these engines offload entire poll/select calls.

For the purpose of discussion: the whole idea of poll() is to wait for async event on sockets. Hardware-wise, async events are implemented by interrupts. Which in turn translates to a callback at the lowest software level. So, the minimum requirement to support baseline-efficient handling of mixed socket types would be: an underlying offloading impl to provide means to register an "even callback". Just one callback, not per-socket. Of course filtering of event types is helpful. Then on Zephyr side of things, this callback would signal an event or semaphore object. And ZFD_IOCTL_POLL_PREPARE would return this single event/sema for every offloaded socket. And then, after getting an event, ZFD_IOCTL_POLL_UPDATE will take care of figuring out which socket(s) this event actually affected.

So, it shouldn't be that too hard to support mixed sockets if underlying provider supports the suitable low-level interface. Obviously, I don't for implementation of that any time soon. But it's helpful to discuss approaches to that, to validate our design.

pfalcon · 2019-12-26T13:10:44Z

Sorry for the delay here, was working mind-deep on other matters. Hope to catch up with a detailed review now.

@pfalcon:

(Oh, let me add this too, to explore the bounds of it. To make different impls truly coexists, we should be aware that it may be not possible to select socket handler on socket() call. It might be needed to be postponed until bind() or connect() calls. While supporting that is definitely as separate task with their own would-be-stakeholders and requirements, we might want to think that architecture and implementation is supportive of such future extension, not obstructing it).

@rlubos:

This one I have actually not thought of so I don't have much to comment. I'm open though for any suggestions in this PR, if something needs to be reworked to make it possible in the future.

@jukkar:

This might be tricky to do. I investigated this when implementing the socket registration code and it is quite difficult to postpone things done by socket() to when bind() is called.

So, answering to @jukkar first, from a PoV of high-level programmer, there's nothing too complicated with that. Of course, we do low-level programming here, and simple high-level algorithms are swamped with matters like resource usage, error handling, race conditions, etc. Still, high-level algorithm would be:

Socket offloading provider(s) to have higher priority than native impl, so one of them gets socket() call.
This offloading provide creates a dummy fd entry, and just stores socket() params.
It waits for connect() or bind() calls, at which point it can check whether it belongs its interfaces.
If yes, it creates underlying socket and replaces fd entry to refer to it.
But of not, it forwards both socket() and bind/connect() calls down the chain of lower-priority providers. One caveat that in this case, the already allocated fd should be passed to the next provider. I.e., there should be something like z_net_socket_next(domain, type, protocol, CONFIG_MY_PRIORITY + 1, allocated_fd).

Note that there can be more than one offloading providers of successively lower priorities, and forwarded calls would just cascade thru them, until reaching a fallback, native impl. But there's another caveat that between socket() and connect()/bind() there can be other calls, like setsockopt(). So, an offloading provider would need to record and replay them for complete transparency, which adds complexity, but again, not impossible.

So, as before, I don't call for immediate implementation of that, just to validate that our design is supportive of such usage.

@rlubos, So, back to your question, what it would take to implement the scheme above is: 1) add priorities to socket handlers, so there was explicit order and concept of "next provider"; 2) Refactor of ->handler()'s to not allocate an fd themselves, but accept it as param; 3) then, just provide that z_net_socket_next(domain, type, protocol, next_prio, allocated_fd) call.

Now, seeing that this patchset is already pretty big, I'd say the matter of actually supporting the scenario above is largely orthogonal to it. In particular, I may image adding priority to handlers would require a (small) bunch of changes, down to linker scripts. It might be a good idea to consider "2) Refactor of ->handler()'s to not allocate an fd themselves, but accept it as param", but even that is largely orthogonal to the current patchset.

So well, I just invite to consider this scenario, and see if the approach above makes sense, or you see issues with it or better alternatives.

pfalcon

Some offloaded engines (nRF91, SimpleLink) offload entire poll operation. I found it difficult to integrate them with poll solution for native sockets. Therefore, I propose to extend the list of functions registered along with socket engine, with custom poll and a helper function which determines whether custom poll should be used or not for that specific poll API call.

Well, I so far reviewed just one commit in series, "net: sockets: Allow to register custom poll function", and I don't really agree with the quote below. I'd suggest that extending existing internal poll handling protocol (occurring via ioctl) would allow to make it more slim/efficient.

pfalcon

Reviewed "drivers: wifi: simplelink: Rework offloading mechanism", IMHO there's a bit of over-engineering, and everything can be done using existing fdtable.h API, without introduction of extra mapping table.

pfalcon · 2020-01-06T08:50:43Z

drivers/wifi/simplelink/simplelink_sockets.c

+/* Socket LUT, matching system file descriptor with offloaded descriptor.
+ * Note: system FD will always be less than CONFIG_POSIX_MAX_FDS.
+ */
+static int simplelink_offload_fd[CONFIG_POSIX_MAX_FDS] = {


So, fairly speaking, fdtable mini-subsys was designed to hold fd to the underlying object mapping and preclude the need for such duplicate mappings. Can you explain why it's needed? If https://github.com/zephyrproject-rtos/zephyr/blob/master/include/sys/fdtable.h API is not flexible enough, we'd rather look into extending it, than add resource duplication like that. And if it's truly needed, then there rather be comment why it's needed and can't just use fdtable API.

So the reason I need this mapping is that SimpleLink (and nRF91 as well) offloaded socket engines keep their own track of the socket descriptors assigned. These engines return "their" socket descriptor when socket is created, completely unaware from what's happening on the Zephyr side.

So I'm afraid it's not a matter of fdtable API flexibility. Even if we could request a specific fd number from Zephyr, matching the one returned by the offloaded engine, we could end-up in a situation, that this specific fd is already in use on the Zephyr side. That's why decided to have this LUT, to keep track of what Zephyr file descriptors are assigned to which offloaded socket descriptors.

Theoretically, I could hack the offloaded socket descriptor value into the void *obj field within the struct fd_entry, but again, I'd consider it a hack. As long as we treat void *obj as a pointer, we need to allocate the memory somewhere to keep the offloaded socket descriptor, that's what I use the LUT for as well.

So the reason I need this mapping is that SimpleLink (and nRF91 as well) offloaded socket engines keep their own track of the socket descriptors assigned. These engines return "their" socket descriptor when socket is created, completely unaware from what's happening on the Zephyr side.

Absolutely. This is no way different from native Zephyr networking API, which has "its" net_context*, and fdtable mini-subsys maps POSIX-level file descriptor to an arbitrary void* of the underlying implementation. So, as long as you need to data which is castable to void*, just use fdtable directly. If you need to store more data than that - then year, you'd need to define a struct for that data, and keep "shadow array" of such structs. My understanding, is that "modem" offloading from this PR does that.

Theoretically, I could hack the offloaded socket descriptor value into the void *obj field within the struct fd_entry, but again, I'd consider it a hack.

It's not a hack, is the intended usage. void* is effectively a datum of size of bitness of the target platform. If it's not pure enough for you, feel free to submit a patch to replace it with explicit uintptr_t. But that way, we'll just have more casts around overall.

Ok, if it's not against the original concept to reuse the obj field for such purpose (as storing some custom integer value), then I see no reason to insist on this extra array. I'll rework this as well.

if it's not against the original concept

Nope, it was exactly the opposite. And I appreciate this discussion, just the same as your work on plugging TLS into this system, which allowed to extend it and clarify some aspects. It's a matter of fact that there're still not clear enough things in there. Which reminds me that we had similar situation with @jukkar in other patch, and since then, it's in my TODO to actually add more comments to all this stuff to clarify it's "internal API" (that's part of the issue, IMHO these matters shouldn't be in user-level docs, why it wasn't commented thoroughly in the first place).

So, these discussions are important, please provide any suggestions on how to clarify this "internal API contract", feel free to improve smaller things, and bigger ones which would sidetrack you too much, feel free to emphasize to me, and I intend to get to improve it.

Thanks!

pfalcon · 2020-01-06T08:53:55Z

drivers/wifi/simplelink/simplelink_sockets.c

 			continue;
+		} else if ((fds[i].fd >= ARRAY_SIZE(simplelink_offload_fd)) ||
+			   (simplelink_offload_fd[fds[i].fd] == -1)) {


For example, here'd you use something like z_get_fd_obj(fds[i].fd, &simplelink_socket_fd_op_vtable, ENOTSUPP).

vanti · 2020-01-15T18:14:04Z

drivers/wifi/simplelink/simplelink_sockets.c

+					   ENOTSUP);
+			if (obj != NULL) {
+				/* Offloaded socket found. */
+				fd = OBJ_TO_SD(obj);


Renaming 'fd' to 'sd' would make this more consistent with the other functions.

vanti · 2020-01-21T19:35:08Z

drivers/wifi/simplelink/simplelink_sockets.c

 		}
 		if (fds[i].events & POLLIN) {
 			SL_SOCKET_FD_SET(fd, &rfds);
 		}
 		if (fds[i].events & POLLOUT) {
 			SL_SOCKET_FD_SET(fd, &wfds);
 		}
-		if (fds[i].fd > max_fd) {
-			max_fd = fds[i].fd;
+		if (fd > max_fd) {


Should rename this to 'max_sd' for consistency.

vanti · 2020-01-22T22:20:26Z

subsys/net/lib/sockets/getaddrinfo.c

 int zsock_getaddrinfo(const char *host, const char *service,
 		      const struct zsock_addrinfo *hints,
 		      struct zsock_addrinfo **res)
 {
+	if (IS_ENABLED(CONFIG_NET_SOCKETS_OFFLOAD)) {


This works fine for cc32xx, but I wonder if there are other platforms out there that support socket offload but not DNS offload, that would want to use the DNS resolver code below. Maybe it is nicer to check whether DNS functions are offloaded by doing something similar to the following:

if (socket_offload_dns_is_registered()) { /* Check if an offloaded DNS implementation exists */ return socket_offload_getaddrinfo(host, service, hints, res); }

Then again, no one is currently asking for this, so I am ok with the way it is to have NET_SOCKETS_OFFLOAD imply DNS offload, and for the implementation to fail with an assertion if DNS offload is not registered.

Allow to use offloaded `poll` implementation via the existing ioctl poll control mechanism. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>

Instead of using a custom offloading interface, users can use `NET_SOCKET_REGISTER` macro to register custom socket API provider. This solution removes a limitation, that only one offloaded interface can be registered and that it cannot be used together with native IP stack. The only exception remainig are DNS releated operations - `getaddrinfo`/`freeaddrinfo`, which, when offloaded, have to be registered specifically. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>

This commit fixes an issue observed with SimpleLink sockets with multiple definitions of `gethostname` function. So far, the definition within `socket.h` was not visible when offloading was enabled. As this is no longer the case, and SimpleLink partially uses POSIX subsystem, builds for this platform resulted in compilation error. The issue was fixed by moving `gethostname` declaration in unistd.h inside the `#ifdef CONFIG_POSIX_API` block. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>

rlubos · 2020-01-23T11:04:13Z

@vanti Thanks, addressed the naming suggestions.

Also, rebased on top of the current master.

Switch to `NET_SOCKET_REGISTER` mechanism over the offloaded API registration. Including the following fixes from the review: * The fd returned by the socket accept call needs to be finalized, similar to how it is done for socket creation. * sl_RecvFrom() in TI SimpleLink Host driver does not support NULL pointers for 'from' address and address length, and sl_SendTo() does not ignore the destination address when in connection mode, so passing NULL would cause a failure. These issues have been reported to TI (CC3X20SDK-1970, CC3X20SDK-1971). Let's use sl_Recv and sl_Send to implement recvfrom/sendto in the case of NULL addresses. * simplelink_poll() should not process negative file descriptors in the fds array after sl_Selecti() returns. A negative fd value indicates that the entry is invalid and should be ignored. Signed-off-by: Vincent Wan <vincent.wan@linaro.org> Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>

Switch to `NET_SOCKET_REGISTER` mechanism over the offloaded API registration. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>

rlubos · 2020-01-23T15:03:03Z

Pushed a rework of the last socket-offloaded implementation I've found - eswifi. This I believe makes this PR more or less complete, content-wise.

I based on the SimpleLink work done before along with @vanti. Again, I've made sure the implementation builds with no issues, I have no hardware to verify it though. @ssekar15 or @loicpoulain I saw you were involved in this implementation originally, any chance you could have a look?

Switch to `NET_SOCKET_REGISTER` mechanism over the offloaded API registration. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>

mike-scott · 2020-01-28T00:05:57Z

@rlubos I have a rather large PR for some modem layer bugs and changes to the SARA modem driver here: #22149

This is going to conflict with this PR. What was the timing of this PR with regards to the merge window?

rlubos · 2020-01-28T08:42:02Z

@mike-scott I had no specific timing for this PR, other than "when it's ready and approved".

Don't worry about the conflicts, I can rebase and resolve them after you get your PR merged. As long as you could have a look at this PR afterwards, it should be fine, the ublox-sara-r4 commit in here is not that large after all.

pfalcon · 2020-01-29T14:58:06Z

I think we should merge this ASAP, exactly because there're growing number of offloading driver PRs, which are effectively blocked on this PR.

I see there's no way to "dismiss" review for a "draft" PR, even one's own. I thus "re-requested" review from myself to remove explicit -1, and let me re-request review from @vanti too. I otherwise took day offs beginning of the week, but re-reviewing this is my priority for tomorrow, and I assume this would be +1 or very minor changes requests. So, this mostly could use extra testing from offloading driver maintainers.

pfalcon

Looks good, thanks for all the changes done! There're a few more comments which are IMHO worth looking into, but let me approve this.

I'd also suggest to turn this from "Draft" into normal PR. And if it's expected that it may spend more time in queue, I'd suggest to split out "net: sockets: Fix gethostname socket.h/unistd.h clash" commit (which seems to not depend on other changes) into a separate PR, to still have gradual progress with this.

pfalcon · 2020-01-30T05:55:46Z

subsys/net/lib/sockets/sockets.c

+			 * it detected an offloaded socket.
+			 * In case the fds array contains a mixup of offloaded
+			 * and non-offloaded sockets, the offloaded poll handler
+			 * shall return an error.


shall return an error

I'd suggest to not stipulate artificial constraints and limit creativity, and word it along the lines of "it's up to offload poll handler to deal with such situation, or return an error."

drivers/modem/modem_socket.h

pfalcon · 2020-01-30T06:58:38Z

subsys/net/lib/sockets/getaddrinfo.c

+	return socket_offload_freeaddrinfo(ai);
+#endif
+
+	free(ai);


I'll rework this to if (IS_ENABLED(CONFIG_NET_SOCKETS_OFFLOAD)) { ... } construct.

Seems this wasn't done?

rlubos · 2020-01-30T10:10:18Z

@pfalcon Thanks, I've pulled the PR out of the draft stage, will address the remaining feedback when I get some time.

vanti

Thanks for the changes. I re-ran the samples on cc3220sf_launchxl, and was successful. LGTM!

jukkar

LGTM

rlubos requested review from mike-scott, vanti, GAnthony, pfalcon and jukkar and removed request for mike-scott December 5, 2019 16:07

zephyrbot added area: Modem area: Networking area: API Changes to public APIs area: Samples Samples area: Tests Issues related to a particular existing or missing test labels Dec 5, 2019

rlubos force-pushed the sockets/offloading-rework branch from dc2a532 to dc92a7b Compare December 6, 2019 09:56

pfalcon added the area: Sockets Networking sockets label Dec 6, 2019

vanti requested changes Dec 7, 2019

View reviewed changes

vanti reviewed Dec 10, 2019

View reviewed changes

vanti reviewed Dec 13, 2019

View reviewed changes

rlubos force-pushed the sockets/offloading-rework branch from dc92a7b to 557a926 Compare December 16, 2019 13:34

zephyrbot added the area: Documentation label Dec 16, 2019

rlubos force-pushed the sockets/offloading-rework branch from 557a926 to 442c9f9 Compare December 16, 2019 16:04

pfalcon reviewed Dec 26, 2019

View reviewed changes

pfalcon requested changes Dec 26, 2019

View reviewed changes

pfalcon requested changes Jan 6, 2020

View reviewed changes

rlubos mentioned this pull request Jan 22, 2020

net: sockets: Rework ioctl poll helpers error handling #22096

Merged

vanti reviewed Jan 22, 2020

View reviewed changes

rlubos added 3 commits January 23, 2020 11:43

net: sockets: Extend ioctl poll protocol with offloading feature

54a4053

Allow to use offloaded `poll` implementation via the existing ioctl poll control mechanism. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>

rlubos force-pushed the sockets/offloading-rework branch from 04d5860 to db4b8cf Compare January 23, 2020 11:02

rlubos added 2 commits January 23, 2020 13:02

drivers: modem: ublox-sara-r4: Rework offloading mechanism

db4b8cf

Switch to `NET_SOCKET_REGISTER` mechanism over the offloaded API registration. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>

drivers: wifi: eswifi: Rework offloading mechanism

523ccb7

Switch to `NET_SOCKET_REGISTER` mechanism over the offloaded API registration. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>

pfalcon self-requested a review January 25, 2020 14:56

pfalcon requested a review from vanti January 29, 2020 14:58

pfalcon approved these changes Jan 30, 2020

View reviewed changes

rlubos marked this pull request as ready for review January 30, 2020 09:55

rlubos requested review from carlescufi, galak, loicpoulain, MaureenHelm, nashif and tbursztyka as code owners January 30, 2020 09:55

vanti approved these changes Jan 30, 2020

View reviewed changes

jukkar approved these changes Jan 31, 2020

View reviewed changes

loicpoulain approved these changes Jan 31, 2020

View reviewed changes

nashif merged commit aa17850 into zephyrproject-rtos:master Jan 31, 2020

rlubos deleted the sockets/offloading-rework branch March 12, 2020 14:47

net: sockets: Rework socket offloading #21209

net: sockets: Rework socket offloading #21209

Conversation

rlubos commented Dec 5, 2019

zephyrbot commented Dec 5, 2019 • edited Loading

checkpatch (informational only, not a failure)

pfalcon commented Dec 6, 2019

vanti left a comment

Choose a reason for hiding this comment

rlubos commented Dec 9, 2019

jukkar commented Dec 9, 2019

vanti left a comment • edited Loading

Choose a reason for hiding this comment

vanti 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

rlubos commented Dec 16, 2019

rlubos commented Dec 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlubos Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfalcon commented Dec 26, 2019

pfalcon left a comment

Choose a reason for hiding this comment

pfalcon 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanti Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

rlubos commented Jan 23, 2020

rlubos commented Jan 23, 2020

mike-scott commented Jan 28, 2020

rlubos commented Jan 28, 2020

pfalcon commented Jan 29, 2020

pfalcon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlubos commented Jan 30, 2020

vanti left a comment

Choose a reason for hiding this comment

jukkar left a comment

Choose a reason for hiding this comment

zephyrbot commented Dec 5, 2019 •

edited

Loading

vanti left a comment •

edited

Loading

rlubos Jan 9, 2020 •

edited

Loading

vanti Jan 22, 2020 •

edited

Loading