SystemLayerImplSelect: add libev support: CHIP_SYSTEM_CONFIG_USE_LIBEV #22043
Conversation
|
PR #22043: Size comparison from 67d6821 to 6a0d64c Increases (10 builds for bl602, cc13x2_26x2, esp32, nrfconnect, psoc6, telink)
Decreases (2 builds for cc13x2_26x2, psoc6)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
| @@ -283,6 +325,24 @@ CHIP_ERROR LayerImplSelect::RequestCallbackOnPendingRead(SocketWatchToken token) | |||
| dispatch_activate(watch->mRdSource); | |||
| } | |||
| } | |||
| #elif CHIP_SYSTEM_CONFIG_USE_LIBEV | |||
| if (mLibEvLoopP == nullptr) | |||
There was a problem hiding this comment.
This makes CHIP_SYSTEM_CONFIG_USE_LIBEV mutually exclusive with CHIP_SYSTEM_CONFIG_USE_DISPATCH. But in the Darwin platform manager code the ifdefs are assuming both could be defined at the same time, no?
I think we should stick to them being mutually exclusive and static_asserting that as needed.
There was a problem hiding this comment.
I agree. I started with #if/#endif because this gives clearer blocks in diffs and does not touch the existing code. But it was not possible to keep it that way in many places, so sticking with #if/#elif in all places is probably better.
There was a problem hiding this comment.
See new commit, now changed to #if/#elif throughout.
| @@ -322,10 +382,27 @@ CHIP_ERROR LayerImplSelect::RequestCallbackOnPendingWrite(SocketWatchToken token | |||
| } | |||
| }); | |||
| // only now we are sure the source exists and can become active | |||
| watch->mPendingIO.Set(SocketEventFlags::kWrite); | |||
There was a problem hiding this comment.
Why is this being removed?
There was a problem hiding this comment.
Oh, that was a functionally irrelevant leftover I found after #21349. Does not belong here, but was not worth a separate PR, and then I forgot. It originated in a then abandoned version of the code that tried to suspend/resume the source watching, and needed the flag to set late to avoid race conditions. But in the version accepted the flag is being set already at the top of the method. Same in RequestCallbackOnPendingRead but there I deleted that line in #21349 already.
src/system/SystemLayerImplSelect.cpp
Outdated
| #endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH | ||
| #endif |
There was a problem hiding this comment.
Must be an editing accident, possibly because this is one of the cases that should become #elif as per discussion above, but I was still trying to keep the separate #if blocks.
| #if CHIP_SYSTEM_CONFIG_USE_LIBEV | ||
| #include <ev.h> | ||
| #if CHIP_SYSTEM_CONFIG_USE_DISPATCH | ||
| #error "CHIP_SYSTEM_CONFIG_USE_LIBEV and CHIP_SYSTEM_CONFIG_USE_DISPATCH are mutually exclusive" |
There was a problem hiding this comment.
Right, so they are mutually exclusive, so why the platform manager changes on Darwin?
The Darwin platform manager always uses dispatch, period.
There was a problem hiding this comment.
The Darwin platform manager always uses dispatch, period.
Not necessarily ;-) Using dispatch for I/O and timers is a decision orthogonal to using the Darwin platform manager. In fact, for development of my libev based app (on the mac, in XCode, just because it's so much more comfortable to use…), I use the Darwin platform, but just swap dispatch for libev.
Of course, this is never going to be a production app configuration, so I'd understand you wouldn't want it in mainline. On the other hand, these types of unorthodox setups tend to reveal otherwise hard to find platform dependencies and edge cases.
There was a problem hiding this comment.
I use the Darwin platform, but just swap dispatch for libev.
How do you guarantee that I/O processing is happening on the right dispatch queue in that case, and in particular that it does not race with other Matter things that do run on that dispatch queue?
There was a problem hiding this comment.
How do you guarantee that I/O processing is happening on the right dispatch queue in that case
Without CHIP_SYSTEM_CONFIG_USE_DISPATCH, nothing in the non-Darwin-specific code can run anywhere else than under libev control (timer and IO callbacks) on the same thread/queue.
Only Darwin specific code could indirectly make use of APIs that would call back from other queue/thread contexts. Of those, my already-on-network app only uses dns-sd and only for publishing. I cannot guarantee anything here, I just tried it works in my context, which is, as said, not a setup for a productive build, but just a development convenience. For that, the possible dangers are isolated enough.
Bottom line - I'd understand if you didn't want #ifs in the platform manager that only support a non-100% safe development setup. You decide 😄
There was a problem hiding this comment.
Without CHIP_SYSTEM_CONFIG_USE_DISPATCH, nothing in the non-Darwin-specific code can run anywhere else than under libev control (timer and IO callbacks) on the same thread/queue.
Does this assume that something other than src/platform/darwin/PlatformManagerImpl.cpp is being used as the platform manager? There's a not-entirely-obvious dependency here where ScheduleLambda on the System::Layer will run it via PostEvent on the platform manager...
And of course various things directly PostEvent and whatnot.
If you're not using the platform manager's event loop at all (so not calling RunEventLoop or StartEventLoopTask), then I'd like to understand the setup and what things are running. If you are, then the assumption is that PostEvent and whatever System::Layer is doing run things on the same thread, or at least interlocked sanely so they can't run at the same time.
Mostly I feel that event loops and the stuff around them are complex enough people get confused easily, so I would rather not have code around that makes them more confused.
- as discussed in project-chip#22043 - also add conditional definition for `ev_io_modify()` required for older libev versions (taken from current libev source, blown up to multiline by clang-format)
- as discussed in project-chip#22043 - also add conditional definition for `ev_io_modify()` required for older libev versions (taken from current libev source, blown up to multiline by clang-format)
plan44
force-pushed
the
PR/add-support-for-libev
branch
from
5d66185 to
c4bf883
Compare
…LIBEV=1 When CHIP_SYSTEM_CONFIG_USE_LIBEV is set, SystemLayerImplSelect expects a *libev* mainloop to be present to schedule timers and socket watches (similar to CHIP_SYSTEM_CONFIG_USE_DISPATCH for Darwin). A libev mainloop must be passed to SystemLayer using `SetLibEvLoop()` before any timers or socket watches are used - otherwise, `chipDie()` is invoked. # Usage The entire project needs to be build with `CHIP_SYSTEM_CONFIG_USE_LIBEV=1` (this can be done via invoking a project-specific extra config via the `default_configs_extra` argument in args.gni) Setting up the libev mainloop and handing it over to SystemLayer must be done in application specific code, outside the code provided by chip examples. Also adding libev as a dependency must be done in the application's BUILD.gn. # Background *libev* is a multi-platform event library often used in embedded linux context to handle events, and builds the basis for various libraries with non-blocking APIs. This changeset allows using the *connectedhomeip* stack with libev based applications.
- as discussed in project-chip#22043 - also add conditional definition for `ev_io_modify()` required for older libev versions (taken from current libev source, blown up to multiline by clang-format)
plan44
force-pushed
the
PR/add-support-for-libev
branch
from
6918036 to
36e7aa5
Compare
|
PR #22043: Size comparison from 6329339 to 36e7aa5 Increases (5 builds for cc13x2_26x2, psoc6)
Decreases (3 builds for cc13x2_26x2, esp32)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
| @@ -54,8 +54,10 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack() | |||
|
|
|||
| mRunLoopSem = dispatch_semaphore_create(0); | |||
|
|
|||
| #if !CHIP_SYSTEM_CONFIG_USE_LIBEV | |||
There was a problem hiding this comment.
This part is still not clear to me. If we are here (or more to the point if the System::Layer will look for this queue), then CHIP_SYSTEM_CONFIG_USE_DISPATCH is defined, right? But that's mutually exclusive with CHIP_SYSTEM_CONFIG_USE_LIBEV. So why do we need the conditioning here?
There was a problem hiding this comment.
If we get here, we're on Darwin, which does have dispatch and could provide a queue to the system layer. But when we're compiling with CHIP_SYSTEM_CONFIG_USE_LIBEV (which is possible even under Darwin), the system layer does not have queue support, and does not have the SetDispatchQueue method.
For clarity, I guess it would be better to just use #if CHIP_SYSTEM_CONFIG_USE_DISPATCHhere instead of #if !CHIP_SYSTEM_CONFIG_USE_LIBEV. Under normal Darwin builds, that conditional is redundant, but it allows to use other mainloops such as the libev based one. Agree?
There was a problem hiding this comment.
BTW: I fully agree this be better a "POSTPONED past 1.0" thing, that's why I posted it as a draft in the first place. I'm happy though about you looking at it right now, and I'm using and testing it daily in my setup right now, so by the time it gets a regular PR it should be reasonably mature :-)
There was a problem hiding this comment.
For clarity, I guess it would be better to just use #if CHIP_SYSTEM_CONFIG_USE_DISPATCHhere
Yes, that would be much more clear, since that's the thing we really care about: that the SetDispatchQueue method exists. OK.
| #if CHIP_SYSTEM_CONFIG_USE_LIBEV | ||
| #include <ev.h> | ||
| #if CHIP_SYSTEM_CONFIG_USE_DISPATCH | ||
| #error "CHIP_SYSTEM_CONFIG_USE_LIBEV and CHIP_SYSTEM_CONFIG_USE_DISPATCH are mutually exclusive" |
There was a problem hiding this comment.
Without CHIP_SYSTEM_CONFIG_USE_DISPATCH, nothing in the non-Darwin-specific code can run anywhere else than under libev control (timer and IO callbacks) on the same thread/queue.
Does this assume that something other than src/platform/darwin/PlatformManagerImpl.cpp is being used as the platform manager? There's a not-entirely-obvious dependency here where ScheduleLambda on the System::Layer will run it via PostEvent on the platform manager...
And of course various things directly PostEvent and whatnot.
If you're not using the platform manager's event loop at all (so not calling RunEventLoop or StartEventLoopTask), then I'd like to understand the setup and what things are running. If you are, then the assumption is that PostEvent and whatever System::Layer is doing run things on the same thread, or at least interlocked sanely so they can't run at the same time.
Mostly I feel that event loops and the stuff around them are complex enough people get confused easily, so I would rather not have code around that makes them more confused.
| // just a timer with no delay | ||
| return StartTimer(Clock::Timeout(0), onComplete, appState); |
There was a problem hiding this comment.
This will be buggy if someone calls ScheduleWork twice with the same args: it should schedule twice, but this will cancel the previous one, right?
The Darwin impl gets this right. The select-based impl will get it right once #22375 merges.
There was a problem hiding this comment.
There's a not-entirely-obvious dependency here where ScheduleLambda on the System::Layer will run it via PostEvent on the platform manager...
And of course various things directly PostEvent and whatnot.
It's beginning to dawn on me that this is more complicated - I wasn't aware that PostEvent also runs on dispatch, directly.
Mostly I feel that event loops and the stuff around them are complex enough people get confused easily, so I would rather not have code around that makes them more confused.
I guess you're right. I'll remove the things related to that Darwin/libev Frankenstein's monster type of setup from the PR ;-)
After all, the libev support does not aim at replacing dispatch when it is available, but is useful for embedded linux targets like openwrt. Running the hybrid setup on the mac is just convenient for my develop-in-Xcode workflow, I can easily keep a few private patches for that.
This will be buggy if someone calls ScheduleWork twice with the same args: it should schedule twice, but this will cancel the previous one, right?
Right. I'll have a look at #22375 and find a way to make the libev variant working in the same way.
- as discussed in project-chip#22043 - also add conditional definition for `ev_io_modify()` required for older libev versions.
- as discussed in project-chip#22043 - also add conditional definition for `ev_io_modify()` required for older libev versions.
- as discussed in project-chip#22043 - also add conditional definition for `ev_io_modify()` required for older libev versions.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Hi @plan44 , are you still working on this PR? It is a very useful feature! |
Yes, I'm actively using it but currently from a fork of connectedhomeip, see top few commits there. I'll rebase and post it upstream as soon as I find the time to do so with the needed care - unfortunately I'm currently distracted from working a lot on matter right now, it might take a few weeks to get it ready for posting again (and remove the draft status). |
- as discussed in project-chip#22043 - also add conditional definition for `ev_io_modify()` required for older libev versions.
|
This stale pull request has been automatically closed. Thank you for your contributions. |
|
dear bot, the PR is not really stale... ;-) |
- as discussed in project-chip#22043 - also add conditional definition for `ev_io_modify()` required for older libev versions (taken from current libev source, blown up to multiline by clang-format)
|
@plan44 For future reference, I could have reopened this PR (and kept the history), if you had not created a new one from the same branch.... I was just on vacation a few days ago and did not get to this until now. |
|
@bzbarsky-apple sorry, I did not realize it is possible at all to reopen a PR once it is closed - I did not want to stress anybody, just follow up on my promise to update the old PRs from pre-1.0 before Jan 3rd 😉 |
|
@plan44 Yeah, not a problem, just letting you know for future. ;) |
- as discussed in project-chip#22043 - also add conditional definition for `ev_io_modify()` required for older libev versions.
When CHIP_SYSTEM_CONFIG_USE_LIBEV is set, SystemLayerImplSelect expects a libev [1] mainloop to be present to schedule timers and socket watches (similar to CHIP_SYSTEM_CONFIG_USE_DISPATCH for Darwin).
A libev mainloop must be passed to SystemLayer using
SetLibEvLoop()before any timers or socket watches are used - otherwise,chipDie()is invoked.Usage
The entire project needs to be build with
CHIP_SYSTEM_CONFIG_USE_LIBEV=1(this can be done via invoking a project-specific extra config via thedefault_configs_extraargument in args.gni)Setting up the libev mainloop and handing it over to SystemLayer must be done in application specific code, outside the code provided by chip examples. Also adding libev as a dependency must be done in the application's BUILD.gn.
Background
libev is a multi-platform event library often used in embedded linux context to handle events, and builds the basis for various libraries with non-blocking APIs. This changeset allows using the connectedhomeip stack with libev based applications.
Testing
CHIP_SYSTEM_CONFIG_USE_LIBEVnot set.CHIP_SYSTEM_CONFIG_USE_LIBEV=1: chip timer and socket watches work the same way as with select or dispatch based applications.