[libcamera-devel] [PATCH v2 07/11] libcamera: Add a poll-based event dispatcher
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jan 8 15:18:38 CET 2019
Hi Jacopo,
On Tuesday, 8 January 2019 14:32:41 EET Jacopo Mondi wrote:
> On Tue, Jan 08, 2019 at 12:54:57PM +0200, Laurent Pinchart wrote:
> > On Tuesday, 8 January 2019 12:28:01 EET Jacopo Mondi wrote:
> >> On Tue, Jan 08, 2019 at 01:11:47AM +0200, Laurent Pinchart wrote:
> >>> Provide a poll-based event dispatcher implementation as convenience
> >>> for applications that don't need a custom event loop. The poll-based
> >>> dispatcher is automatically instantiated if the application doesn't
> >>> provide its own dispatcher.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>> Changes since v1:
> >>>
> >>> - Fix ASSERT() condition check
> >>> - Ignore duplicate notifiers
> >>> - Clarify warning messages for event notifier (un)registration errors
> >>> ---
> >>>
> >>> include/libcamera/libcamera.h | 1 +
> >>> src/libcamera/camera_manager.cpp | 19 +-
> >>> src/libcamera/event_dispatcher_poll.cpp | 234 ++++++++++++++++++
> >>> src/libcamera/include/event_dispatcher_poll.h | 50 ++++
> >>> src/libcamera/meson.build | 2 +
> >>> 5 files changed, 301 insertions(+), 5 deletions(-)
> >>> create mode 100644 src/libcamera/event_dispatcher_poll.cpp
> >>> create mode 100644 src/libcamera/include/event_dispatcher_poll.h
[snip]
> >>> diff --git a/src/libcamera/event_dispatcher_poll.cpp
> >>> b/src/libcamera/event_dispatcher_poll.cpp new file mode 100644
> >>> index 000000000000..0cd08b2b4cc1
> >>> --- /dev/null
> >>> +++ b/src/libcamera/event_dispatcher_poll.cpp
[snip]
> >>> +void EventDispatcherPoll::unregisterTimer(Timer *timer)
> >>> +{
> >>> + for (auto iter = timers_.begin(); iter != timers_.end(); ++iter) {
> >>> + if (*iter == timer) {
> >>> + timers_.erase(iter);
> >>> + return;
> >>> + }
> >>> +
> >>> + /*
> >>> + * As the timers list is ordered, we can stop as soon as we go
> >>> + * past the deadline.
> >>> + */
> >>> + if ((*iter)->deadline() > timer->deadline())
> >>> + break;
> >>> + }
> >>> +}
> >>> +
> >>
> >> This function is probably worth being documented a bit. Just to point
> >> out it is blocking maybe.
> >
> > It's not supposed to be blocking :-)
>
> I mean processEvents() not unregisterTimer() :)
That makes more sense indeed :-)
The EventDispatcher::processEvents() function is already documented as
blocking, so I don't think we need to repeat this here.
> >>> +void EventDispatcherPoll::processEvents()
> >>> +{
> >>> + int ret;
> >>> +
> >>> + /* Create the pollfd array. */
> >>> + std::vector<struct pollfd> pollfds;
> >>> + pollfds.reserve(notifiers_.size());
> >>> +
> >>> + for (auto notifier : notifiers_)
> >>> + pollfds.push_back({ notifier.first, notifier.second.events(), 0
});
> >>> +
> >>> + /* Compute the timeout. */
> >>> + Timer *nextTimer = !timers_.empty() ? timers_.front() : nullptr;
> >>> + struct timespec timeout;
> >>> +
> >>> + if (nextTimer) {
> >>> + clock_gettime(CLOCK_MONOTONIC, &timeout);
> >>> + uint64_t now = timeout.tv_sec * 1000000000ULL + timeout.tv_nsec;
> >>> +
> >>> + if (nextTimer->deadline() > now) {
> >>> + uint64_t delta = nextTimer->deadline() - now;
> >>> + timeout.tv_sec = delta / 1000000000ULL;
> >>> + timeout.tv_nsec = delta % 1000000000ULL;
> >>> + } else {
> >>> + timeout.tv_sec = 0;
> >>> + timeout.tv_nsec = 0;
> >>> + }
> >>> +
> >>> + LOG(Debug) << "timeout " << timeout.tv_sec << "."
> >>> + << std::setfill('0') << std::setw(9)
> >>> + << timeout.tv_nsec;
> >>> + }
> >>> +
> >>> + /* Wait for events and process notifers and timers. */
> >>> + ret = ppoll(pollfds.data(), pollfds.size(),
> >>> + nextTimer ? &timeout : nullptr, nullptr);
> >>> + if (ret < 0) {
> >>> + ret = -errno;
> >>> + LOG(Warning) << "poll() failed with " << strerror(-ret);
> >>
> >> When poll fails, are we ok processing timers?
> >>
> >>> + } else if (ret > 0) {
> >>> + processNotifiers(pollfds);
> >>> + }
> >>> +
> >>
> >> shouldn't we process timers when poll returns 0 (timeout) ? or we go
> >> there to check just in case we missed a deadline?
> >
> > I think processing timers is fine in that case, just in case. A poll()
> > failure will likely be fatal anyway, so it won't matter much.
>
> So it is not worth aborting/returning the poll errno?
According to its man page, poll() and ppoll() can return the following errors:
- EFAULT The array given as argument was not contained in the calling
program's address space.
- EINTR A signal occurred before any requested event; see signal(7).
- EINVAL The nfds value exceeds the RLIMIT_NOFILE value.
- ENOMEM There was no space to allocate file descriptor tables.
EFAULT means we did something really wrong and I expect a crash to follow soon
afterwards. ENOMEM is also a symptom of a really bad problem that will at best
invoke the OOM killer in the near future. EINVAL is more debatable, but I'm
not sure what we could do there. EINTR is likely the only error we really need
to handle, and that should be handled internally. I'll post a patch on top of
this series (with a related test case).
> >>> + processTimers();
> >>> +}
> >>> +
> >>> +short EventDispatcherPoll::EventNotifierSetPoll::events() const
> >>> +{
> >>> + short events = 0;
> >>> +
> >>> + if (notifiers[EventNotifier::Read])
> >>> + events |= POLLIN;
> >>> + if (notifiers[EventNotifier::Write])
> >>> + events |= POLLOUT;
> >>> + if (notifiers[EventNotifier::Exception])
> >>> + events |= POLLPRI;
> >>> +
> >>> + return events;
> >>> +}
> >>> +
> >>> +void EventDispatcherPoll::processNotifiers(std::vector<struct pollfd>
> >>> &pollfds)
> >>> +{
> >>> + static const struct {
> >>> + EventNotifier::Type type;
> >>> + short events;
> >>> + } events[] = {
> >>> + { EventNotifier::Read, POLLIN },
> >>> + { EventNotifier::Write, POLLOUT },
> >>> + { EventNotifier::Exception, POLLPRI },
> >>> + };
> >>> +
> >>> + for (const struct pollfd &pfd : pollfds) {
> >>> + auto iter = notifiers_.find(pfd.fd);
> >>> + ASSERT(iter != notifiers_.end());
> >>
> >> Is this fatal? I mean, this should not happen, I agree, but do we want
> >> to abort?
> >
> > It's a serious bug in the library that would likely indicate a memory
> > corruption of some sort, or really incorrect library code. I expect the
> > program to crash later in that case, aborting early will make it easier to
> > debug the problem.
>
> A notifier can be removed between the time poll is called and poll is
> returned. I agree it's a corner case though.
Not in a single-threaded application though. For the multi-threaded, if other
threads start corrupting our state, we're screwed anyway. They must not do
that. We'll have to document the thread-related requirements of the API when
we'll implement multi-thread support.
> >>> +
> >>> + EventNotifierSetPoll &set = iter->second;
> >>> +
> >>> + for (const auto &event : events) {
> >>> + EventNotifier *notifier = set.notifiers[event.type];
> >>> +
> >>> + if (!notifier)
> >>> + continue;
> >>> +
> >>> + /*
> >>> + * If the file descriptor is invalid, disable the
> >>> + * notifier immediately.
> >>> + */
> >>> + if (pfd.revents & POLLNVAL) {
> >>> + LOG(Warning) << "Disabling " << notifierType(event.type)
> >>> + << " due to invalid file descriptor "
> >>> + << pfd.fd;
> >>> + unregisterEventNotifier(notifier);
> >>
> >> I'm not sure if POLLNVAL is exclusive (when it's set, no other bit
> >> field is set in pfd.revents), but if it is not, and some other event
> >> source flag is set, after unregistering, shouldn't you continue?
> >
> > I would assume it's exclusive indeed, a continue shouldn't hurt here, I'll
> > add one (but it shouldn't make a big difference either, given that
> > POLLNVAL shouldn't occur at all in the normal case).
> >
> >>> + }
> >>> +
> >>> + if (pfd.revents & event.events)
> >>> + notifier->activated.emit(notifier);
> >>> + }
> >>> + }
> >>> +}
[snip]
> >>> diff --git a/src/libcamera/include/event_dispatcher_poll.h
> >>> b/src/libcamera/include/event_dispatcher_poll.h new file mode 100644
> >>> index 000000000000..600289e455e2
> >>> --- /dev/null
> >>> +++ b/src/libcamera/include/event_dispatcher_poll.h
> >>> @@ -0,0 +1,50 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2019, Google Inc.
> >>> + *
> >>> + * event_loop.h - Camera object interface
> >>
> >> Camera object?
> >
> > Can you guess where this was copied from ? :-)
> >
> > I've now fixed it (as well as the file name).
> >
> >>> + */
> >>> +#ifndef __LIBCAMERA_EVENT_DISPATCHER_POLL_H__
> >>> +#define __LIBCAMERA_EVENT_DISPATCHER_POLL_H__
> >>> +
> >>> +#include <libcamera/event_dispatcher.h>
> >>> +
> >>> +#include <list>
> >>> +#include <map>
> >>> +#include <vector>
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +class EventNotifier;
> >>> +class Timer;
> >>
> >> Still a bit skeptical about forward declarations when an include would
> >> suffice, but that's up to you actually.
> >
> > On the plus side, they speed up compilation and avoid potential future
> > circular inclusions. Do they cause issues ?
>
> Not particularly, as it is unlikely we miss recompiling this file if
> the header change as the library is built together.
>
> Thanks for the multiple clarifications.
> It is my opinion you should push the next iteration of the series.
Thank you.
[snip]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list