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.

Sign up for GitHub

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

SystemLayerImplSelect: add libev support: CHIP_SYSTEM_CONFIG_USE_LIBEV #22043

Closed
wants to merge 2 commits into from
Closed

SystemLayerImplSelect: add libev support: CHIP_SYSTEM_CONFIG_USE_LIBEV #22043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4310e2a
SystemLayerImplSelect: add libev support when CHIP_SYSTEM_CONFIG_USE_…
plan44 Aug 12, 2022
36e7aa5
SystemLayerImplSelect: consistently use #if/#elif for DISPATCH vs LIBEV
plan44 Aug 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/platform/Darwin/PlatformManagerImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,10 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack()

mRunLoopSem = dispatch_semaphore_create(0);

#if !CHIP_SYSTEM_CONFIG_USE_LIBEV
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Ensure there is a dispatch queue available
static_cast<System::LayerSocketsLoop &>(DeviceLayer::SystemLayer()).SetDispatchQueue(GetWorkQueue());
#endif

// Call _InitChipStack() on the generic implementation base class
// to finish the initialization process.
Expand Down
5 changes: 5 additions & 0 deletions src/platform/Darwin/SystemPlatformConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,14 @@ struct ChipDeviceEvent;

// ==================== Platform Adaptations ====================

#if !CHIP_SYSTEM_CONFIG_USE_LIBEV
// FIXME: these should not be hardcoded here, it is set via build config
// Need to exclude these for now in libev case
#define CHIP_SYSTEM_CONFIG_POSIX_LOCKING 0
#define CHIP_SYSTEM_CONFIG_FREERTOS_LOCKING 0
#define CHIP_SYSTEM_CONFIG_NO_LOCKING 1
#endif

#define CHIP_SYSTEM_CONFIG_PLATFORM_PROVIDES_TIME 1
#define CHIP_SYSTEM_CONFIG_USE_POSIX_TIME_FUNCTS 1
#define CHIP_SYSTEM_CONFIG_POOL_USE_HEAP 1
Expand Down
1 change: 1 addition & 0 deletions src/system/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ buildconfig_header("system_buildconfig") {
"CHIP_SYSTEM_CONFIG_TEST=${chip_build_tests}",
"CHIP_WITH_NLFAULTINJECTION=${chip_with_nlfaultinjection}",
"CHIP_SYSTEM_CONFIG_USE_DISPATCH=${chip_system_config_use_dispatch}",
"CHIP_SYSTEM_CONFIG_USE_LIBEV=${chip_system_config_use_libev}",
"CHIP_SYSTEM_CONFIG_USE_LWIP=${chip_system_config_use_lwip}",
"CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT=${chip_system_config_use_open_thread_inet_endpoints}",
"CHIP_SYSTEM_CONFIG_USE_SOCKETS=${chip_system_config_use_sockets}",
Expand Down
10 changes: 7 additions & 3 deletions src/system/SystemLayer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
#include <dispatch/dispatch.h>
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
#elif CHIP_SYSTEM_CONFIG_USE_LIBEV
#include <ev.h>
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV

#include <utility>

Expand Down Expand Up @@ -171,7 +173,7 @@ class DLL_EXPORT Layer

private:
// Copy and assignment NOT DEFINED
Layer(const Layer &) = delete;
Layer(const Layer &) = delete;
Layer & operator=(const Layer &) = delete;
};

Expand Down Expand Up @@ -250,7 +252,9 @@ class LayerSocketsLoop : public LayerSockets
#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
virtual void SetDispatchQueue(dispatch_queue_t dispatchQueue) = 0;
virtual dispatch_queue_t GetDispatchQueue() = 0;
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
#elif CHIP_SYSTEM_CONFIG_USE_LIBEV
virtual void SetLibEvLoop(struct ev_loop * aLibEvLoopP) = 0;
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV
};

#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
Expand Down
174 changes: 163 additions & 11 deletions src/system/SystemLayerImplSelect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@
#define PTHREAD_NULL 0
#endif // CHIP_SYSTEM_CONFIG_POSIX_LOCKING && !defined(PTHREAD_NULL)

#if CHIP_SYSTEM_CONFIG_USE_LIBEV
// older libev do not yet have ev_io_modify
#ifndef ev_io_modify
#define ev_io_modify(ev, events_) \
do \
{ \
(ev)->events = ((ev)->events & EV__IOFDSET) | (events_); \
} while (0)
#endif // ev_io_modify
#endif // CHIP_SYSTEM_CONFIG_USE_LIBEV

namespace chip {
namespace System {

Expand Down Expand Up @@ -82,11 +93,25 @@ void LayerImplSelect::Shutdown()
{
w.DisableAndClear();
}
#elif CHIP_SYSTEM_CONFIG_USE_LIBEV
TimerList::Node * timer;
while ((timer = mTimerList.PopEarliest()) != nullptr)
{
if (ev_is_active(&timer->mLibEvTimer))
{
ev_timer_stop(mLibEvLoopP, &timer->mLibEvTimer);
}
}
mTimerPool.ReleaseAll();

#else // CHIP_SYSTEM_CONFIG_USE_DISPATCH
for (auto & w : mSocketWatchPool)
{
w.DisableAndClear();
}
#else
mTimerList.Clear();
mTimerPool.ReleaseAll();
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV

mWakeEvent.Close(*this);

Expand Down Expand Up @@ -155,14 +180,28 @@ CHIP_ERROR LayerImplSelect::StartTimer(Clock::Timeout delay, TimerCompleteCallba
dispatch_resume(timerSource);
return CHIP_NO_ERROR;
}
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH

#elif CHIP_SYSTEM_CONFIG_USE_LIBEV
if (mLibEvLoopP == nullptr)
{
chipDie();
}
ev_timer_init(&timer->mLibEvTimer, &LayerImplSelect::HandleLibEvTimer, 1, 0);
timer->mLibEvTimer.data = timer;
auto t = Clock::Milliseconds64(delay).count();
ev_timer_set(&timer->mLibEvTimer, static_cast<double>(t) / 1E3, 0.);
(void) mTimerList.Add(timer);
ev_timer_start(mLibEvLoopP, &timer->mLibEvTimer);
return CHIP_NO_ERROR;
#endif
#if !CHIP_SYSTEM_CONFIG_USE_LIBEV
// Note: dispatch based implementation needs this as fallback, but not LIBEV (and dead code is not allowed with -Werror)
if (mTimerList.Add(timer) == timer)
{
// The new timer is the earliest, so the time until the next event has probably changed.
Signal();
}
return CHIP_NO_ERROR;
#endif // !CHIP_SYSTEM_CONFIG_USE_LIBEV
}

void LayerImplSelect::CancelTimer(TimerCompleteCallback onComplete, void * appState)
Expand All @@ -178,7 +217,13 @@ void LayerImplSelect::CancelTimer(TimerCompleteCallback onComplete, void * appSt
dispatch_source_cancel(timer->mTimerSource);
dispatch_release(timer->mTimerSource);
}
#endif
#elif CHIP_SYSTEM_CONFIG_USE_LIBEV
if (mLibEvLoopP == nullptr)
{
chipDie();
}
ev_timer_stop(mLibEvLoopP, &timer->mLibEvTimer);
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV

mTimerPool.Release(timer);
Signal();
Expand All @@ -197,8 +242,12 @@ CHIP_ERROR LayerImplSelect::ScheduleWork(TimerCompleteCallback onComplete, void
});
return CHIP_NO_ERROR;
}
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH

#elif CHIP_SYSTEM_CONFIG_USE_LIBEV
// just a timer with no delay
return StartTimer(Clock::Timeout(0), onComplete, appState);
Comment on lines +246 to +247
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV
#if !CHIP_SYSTEM_CONFIG_USE_LIBEV
// Note: dispatch based implementation needs this as fallback, but not LIBEV (and dead code is not allowed with -Werror)
CancelTimer(onComplete, appState);

TimerList::Node * timer = mTimerPool.Create(*this, SystemClock().GetMonotonicTimestamp(), onComplete, appState);
Expand All @@ -210,6 +259,7 @@ CHIP_ERROR LayerImplSelect::ScheduleWork(TimerCompleteCallback onComplete, void
Signal();
}
return CHIP_NO_ERROR;
#endif // !CHIP_SYSTEM_CONFIG_USE_LIBEV
}

CHIP_ERROR LayerImplSelect::StartWatchingSocket(int fd, SocketWatchToken * tokenOut)
Expand All @@ -231,6 +281,11 @@ CHIP_ERROR LayerImplSelect::StartWatchingSocket(int fd, SocketWatchToken * token
VerifyOrReturnError(watch != nullptr, CHIP_ERROR_ENDPOINT_POOL_FULL);

watch->mFD = fd;
#if CHIP_SYSTEM_CONFIG_USE_LIBEV
ev_io_init(&watch->mIoWatcher, &LayerImplSelect::HandleLibEvIoWatcher, 0, 0);
watch->mIoWatcher.data = watch;
watch->mLayerImplSelectP = this;
#endif

*tokenOut = reinterpret_cast<SocketWatchToken>(watch);
return CHIP_NO_ERROR;
Expand Down Expand Up @@ -283,6 +338,24 @@ CHIP_ERROR LayerImplSelect::RequestCallbackOnPendingRead(SocketWatchToken token)
dispatch_activate(watch->mRdSource);
}
}
#elif CHIP_SYSTEM_CONFIG_USE_LIBEV
if (mLibEvLoopP == nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See new commit, now changed to #if/#elif throughout.

{
chipDie();
}
int evs = (watch->mPendingIO.Has(SocketEventFlags::kRead) ? EV_READ : 0) |
(watch->mPendingIO.Has(SocketEventFlags::kWrite) ? EV_WRITE : 0);
if (!ev_is_active(&watch->mIoWatcher))
{
// First time actually using that watch
ev_io_set(&watch->mIoWatcher, watch->mFD, evs);
ev_io_start(mLibEvLoopP, &watch->mIoWatcher);
}
else
{
// already active, just change flags
ev_io_modify(&watch->mIoWatcher, evs);
}
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH

return CHIP_NO_ERROR;
Expand Down Expand Up @@ -322,10 +395,27 @@ CHIP_ERROR LayerImplSelect::RequestCallbackOnPendingWrite(SocketWatchToken token
}
});
// only now we are sure the source exists and can become active
watch->mPendingIO.Set(SocketEventFlags::kWrite);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dispatch_activate(watch->mWrSource);
}
}
#elif CHIP_SYSTEM_CONFIG_USE_LIBEV
if (mLibEvLoopP == nullptr)
{
chipDie();
}
int evs = (watch->mPendingIO.Has(SocketEventFlags::kRead) ? EV_READ : 0) |
(watch->mPendingIO.Has(SocketEventFlags::kWrite) ? EV_WRITE : 0);
if (!ev_is_active(&watch->mIoWatcher))
{
// First time actually using that watch
ev_io_set(&watch->mIoWatcher, watch->mFD, evs);
ev_io_start(mLibEvLoopP, &watch->mIoWatcher);
}
else
{
// already active, just change flags
ev_io_modify(&watch->mIoWatcher, evs);
}
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH

return CHIP_NO_ERROR;
Expand All @@ -338,6 +428,14 @@ CHIP_ERROR LayerImplSelect::ClearCallbackOnPendingRead(SocketWatchToken token)

watch->mPendingIO.Clear(SocketEventFlags::kRead);

#if CHIP_SYSTEM_CONFIG_USE_LIBEV
if (ev_is_active(&watch->mIoWatcher) && watch->mPendingIO.Raw() == 0)
{
// all flags cleared now, stop watching
ev_io_stop(mLibEvLoopP, &watch->mIoWatcher);
}
#endif

return CHIP_NO_ERROR;
}

Expand All @@ -348,6 +446,14 @@ CHIP_ERROR LayerImplSelect::ClearCallbackOnPendingWrite(SocketWatchToken token)

watch->mPendingIO.Clear(SocketEventFlags::kWrite);

#if CHIP_SYSTEM_CONFIG_USE_LIBEV
if (ev_is_active(&watch->mIoWatcher) && watch->mPendingIO.Raw() == 0)
{
// all flags cleared now, stop watching
ev_io_stop(mLibEvLoopP, &watch->mIoWatcher);
}
#endif

return CHIP_NO_ERROR;
}

Expand All @@ -359,7 +465,7 @@ CHIP_ERROR LayerImplSelect::StopWatchingSocket(SocketWatchToken * tokenInOut)
VerifyOrReturnError(watch != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(watch->mFD >= 0, CHIP_ERROR_INCORRECT_STATE);

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
#if CHIP_SYSTEM_CONFIG_USE_DISPATCH || CHIP_SYSTEM_CONFIG_USE_LIBEV
watch->DisableAndClear();
#else
watch->Clear();
Expand Down Expand Up @@ -494,12 +600,47 @@ void LayerImplSelect::HandleEvents()
}

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH

void LayerImplSelect::HandleTimerComplete(TimerList::Node * timer)
{
mTimerList.Remove(timer);
mTimerPool.Invoke(timer);
}
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH

#elif CHIP_SYSTEM_CONFIG_USE_LIBEV

void LayerImplSelect::HandleLibEvTimer(EV_P_ struct ev_timer * t, int revents)
{
TimerList::Node * timer = static_cast<TimerList::Node *>(t->data);
VerifyOrDie(timer);
LayerImplSelect * layerP = dynamic_cast<LayerImplSelect *>(timer->mCallback.mSystemLayer);
VerifyOrDie(layerP);
layerP->mTimerList.Remove(timer);
layerP->mTimerPool.Invoke(timer);
}

void LayerImplSelect::HandleLibEvIoWatcher(EV_P_ struct ev_io * i, int revents)
{
SocketWatch * watch = static_cast<SocketWatch *>(i->data);
if (watch != nullptr && watch->mCallback != nullptr && watch->mLayerImplSelectP != nullptr)
{
SocketEvents events;
if (revents & EV_READ)
{
events.Set(SocketEventFlags::kRead);
}
if (revents & EV_WRITE)
{
events.Set(SocketEventFlags::kWrite);
}
if (events.HasAny())
{
watch->mCallback(events, watch->mCallbackData);
}
}
}

#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV

void LayerImplSelect::SocketWatch::Clear()
{
Expand All @@ -510,6 +651,8 @@ void LayerImplSelect::SocketWatch::Clear()
#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
mRdSource = nullptr;
mWrSource = nullptr;
#elif CHIP_SYSTEM_CONFIG_USE_LIBEV
mLayerImplSelectP = nullptr;
#endif
}

Expand All @@ -528,7 +671,16 @@ void LayerImplSelect::SocketWatch::DisableAndClear()
}
Clear();
}
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
#elif CHIP_SYSTEM_CONFIG_USE_LIBEV
void LayerImplSelect::SocketWatch::DisableAndClear()
{
if (mLayerImplSelectP != nullptr && mLayerImplSelectP->mLibEvLoopP != nullptr)
{
ev_io_stop(mLayerImplSelectP->mLibEvLoopP, &mIoWatcher);
}
Clear();
}
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV

} // namespace System
} // namespace chip
Loading