| Message ID | 20190123085923.12524-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | d370c1b46e1733905fd76eec004be26475afae9d |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 23/01/2019 08:59, Laurent Pinchart wrote: > The ppoll() call can be interrupted if a signal is delivered. Handle the > EINTR error code gracefully by restarting the call. This fixes the > event-dispatcher test failure. > Two discussion points below - but otherwise: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/event_dispatcher_poll.cpp | 58 +++++++++++-------- > src/libcamera/include/event_dispatcher_poll.h | 1 + > 2 files changed, 34 insertions(+), 25 deletions(-) > > diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp > index eefac54ca6da..6e0609c34ddc 100644 > --- a/src/libcamera/event_dispatcher_poll.cpp > +++ b/src/libcamera/event_dispatcher_poll.cpp > @@ -128,32 +128,11 @@ void EventDispatcherPoll::processEvents() > 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(Event, Debug) > - << "timeout " << timeout.tv_sec << "." > - << std::setfill('0') << std::setw(9) > - << timeout.tv_nsec; > - } > - > /* Wait for events and process notifiers and timers. */ > - ret = ppoll(pollfds.data(), pollfds.size(), > - nextTimer ? &timeout : nullptr, nullptr); > + do { > + ret = poll(&pollfds); Eeep - you almost caught me here ... : int poll(struct pollfd *fds, nfds_t nfds, int timeout); No problem conflicting with the std library namespace I hope? > + } while (ret == -1 && errno == EINTR); > + > if (ret < 0) { > ret = -errno; > LOG(Event, Warning) << "poll() failed with " << strerror(-ret); > @@ -178,6 +157,35 @@ short EventDispatcherPoll::EventNotifierSetPoll::events() const > return events; > } > > +int EventDispatcherPoll::poll(std::vector<struct pollfd> *pollfds) > +{ > + /* 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(Event, Debug) > + << "timeout " << timeout.tv_sec << "." > + << std::setfill('0') << std::setw(9) > + << timeout.tv_nsec; > + } > + > + return ppoll(pollfds->data(), pollfds->size(), > + nextTimer ? &timeout : nullptr, nullptr); I see comparisons (particularly from a table in Michael Kerrisk's kernel interface book) looking at how epoll is much better than poll/select when there are more than about 10 fds to consider... I wonder how many FDs we'll end up monitoring in our event code... Especially as external code will likely use our event loop. One day it might be worth looking at the comparisons, but I don't think today is that day. > +} > + > void EventDispatcherPoll::processNotifiers(const std::vector<struct pollfd> &pollfds) > { > static const struct { > diff --git a/src/libcamera/include/event_dispatcher_poll.h b/src/libcamera/include/event_dispatcher_poll.h > index a41926e11a11..ac3efde082b4 100644 > --- a/src/libcamera/include/event_dispatcher_poll.h > +++ b/src/libcamera/include/event_dispatcher_poll.h > @@ -41,6 +41,7 @@ private: > std::map<int, EventNotifierSetPoll> notifiers_; > std::list<Timer *> timers_; > > + int poll(std::vector<struct pollfd> *pollfds); > void processNotifiers(const std::vector<struct pollfd> &pollfds); > void processTimers(); > }; >
In addendum On 23/01/2019 11:30, Kieran Bingham wrote: > Hi Laurent, <snip> >> + return ppoll(pollfds->data(), pollfds->size(), >> + nextTimer ? &timeout : nullptr, nullptr); > > I see comparisons (particularly from a table in Michael Kerrisk's kernel > interface book) looking at how epoll is much better than poll/select > when there are more than about 10 fds to consider... > > I wonder how many FDs we'll end up monitoring in our event code... > Especially as external code will likely use our event loop. > > One day it might be worth looking at the comparisons, but I don't think > today is that day. I think that might deserve it's own implementation, so that would be: class EventDispatcherEpoll; - not this class anyway :) -- Kieran <snip>
Hi Kieran, On Wed, Jan 23, 2019 at 11:30:25AM +0000, Kieran Bingham wrote: > On 23/01/2019 08:59, Laurent Pinchart wrote: > > The ppoll() call can be interrupted if a signal is delivered. Handle the > > EINTR error code gracefully by restarting the call. This fixes the > > event-dispatcher test failure. > > Two discussion points below - but otherwise: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/event_dispatcher_poll.cpp | 58 +++++++++++-------- > > src/libcamera/include/event_dispatcher_poll.h | 1 + > > 2 files changed, 34 insertions(+), 25 deletions(-) > > > > diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp > > index eefac54ca6da..6e0609c34ddc 100644 > > --- a/src/libcamera/event_dispatcher_poll.cpp > > +++ b/src/libcamera/event_dispatcher_poll.cpp > > @@ -128,32 +128,11 @@ void EventDispatcherPoll::processEvents() > > 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(Event, Debug) > > - << "timeout " << timeout.tv_sec << "." > > - << std::setfill('0') << std::setw(9) > > - << timeout.tv_nsec; > > - } > > - > > /* Wait for events and process notifiers and timers. */ > > - ret = ppoll(pollfds.data(), pollfds.size(), > > - nextTimer ? &timeout : nullptr, nullptr); > > + do { > > + ret = poll(&pollfds); > > Eeep - you almost caught me here ... : > > int poll(struct pollfd *fds, nfds_t nfds, int timeout); > > No problem conflicting with the std library namespace I hope? No, the compiler will pick the right one, and if me ever need to call the libc function, we can write ::poll(). > > + } while (ret == -1 && errno == EINTR); > > + > > if (ret < 0) { > > ret = -errno; > > LOG(Event, Warning) << "poll() failed with " << strerror(-ret); > > @@ -178,6 +157,35 @@ short EventDispatcherPoll::EventNotifierSetPoll::events() const > > return events; > > } > > > > +int EventDispatcherPoll::poll(std::vector<struct pollfd> *pollfds) > > +{ > > + /* 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(Event, Debug) > > + << "timeout " << timeout.tv_sec << "." > > + << std::setfill('0') << std::setw(9) > > + << timeout.tv_nsec; > > + } > > + > > + return ppoll(pollfds->data(), pollfds->size(), > > + nextTimer ? &timeout : nullptr, nullptr); > > I see comparisons (particularly from a table in Michael Kerrisk's kernel > interface book) looking at how epoll is much better than poll/select > when there are more than about 10 fds to consider... > > I wonder how many FDs we'll end up monitoring in our event code... > Especially as external code will likely use our event loop. > > One day it might be worth looking at the comparisons, but I don't think > today is that day. That's a good point. Out of scope for this patch as you mentioned, but a good point. If someone wants to have a good, be my guest :-) > > +} > > + > > void EventDispatcherPoll::processNotifiers(const std::vector<struct pollfd> &pollfds) > > { > > static const struct { > > diff --git a/src/libcamera/include/event_dispatcher_poll.h b/src/libcamera/include/event_dispatcher_poll.h > > index a41926e11a11..ac3efde082b4 100644 > > --- a/src/libcamera/include/event_dispatcher_poll.h > > +++ b/src/libcamera/include/event_dispatcher_poll.h > > @@ -41,6 +41,7 @@ private: > > std::map<int, EventNotifierSetPoll> notifiers_; > > std::list<Timer *> timers_; > > > > + int poll(std::vector<struct pollfd> *pollfds); > > void processNotifiers(const std::vector<struct pollfd> &pollfds); > > void processTimers(); > > };
diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp index eefac54ca6da..6e0609c34ddc 100644 --- a/src/libcamera/event_dispatcher_poll.cpp +++ b/src/libcamera/event_dispatcher_poll.cpp @@ -128,32 +128,11 @@ void EventDispatcherPoll::processEvents() 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(Event, Debug) - << "timeout " << timeout.tv_sec << "." - << std::setfill('0') << std::setw(9) - << timeout.tv_nsec; - } - /* Wait for events and process notifiers and timers. */ - ret = ppoll(pollfds.data(), pollfds.size(), - nextTimer ? &timeout : nullptr, nullptr); + do { + ret = poll(&pollfds); + } while (ret == -1 && errno == EINTR); + if (ret < 0) { ret = -errno; LOG(Event, Warning) << "poll() failed with " << strerror(-ret); @@ -178,6 +157,35 @@ short EventDispatcherPoll::EventNotifierSetPoll::events() const return events; } +int EventDispatcherPoll::poll(std::vector<struct pollfd> *pollfds) +{ + /* 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(Event, Debug) + << "timeout " << timeout.tv_sec << "." + << std::setfill('0') << std::setw(9) + << timeout.tv_nsec; + } + + return ppoll(pollfds->data(), pollfds->size(), + nextTimer ? &timeout : nullptr, nullptr); +} + void EventDispatcherPoll::processNotifiers(const std::vector<struct pollfd> &pollfds) { static const struct { diff --git a/src/libcamera/include/event_dispatcher_poll.h b/src/libcamera/include/event_dispatcher_poll.h index a41926e11a11..ac3efde082b4 100644 --- a/src/libcamera/include/event_dispatcher_poll.h +++ b/src/libcamera/include/event_dispatcher_poll.h @@ -41,6 +41,7 @@ private: std::map<int, EventNotifierSetPoll> notifiers_; std::list<Timer *> timers_; + int poll(std::vector<struct pollfd> *pollfds); void processNotifiers(const std::vector<struct pollfd> &pollfds); void processTimers(); };
The ppoll() call can be interrupted if a signal is delivered. Handle the EINTR error code gracefully by restarting the call. This fixes the event-dispatcher test failure. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/event_dispatcher_poll.cpp | 58 +++++++++++-------- src/libcamera/include/event_dispatcher_poll.h | 1 + 2 files changed, 34 insertions(+), 25 deletions(-)