[libcamera-devel] [PATCH 2/4] libcamera: event_dispatcher_poll: Handle interrupted ppoll() calls

[Laurent Pinchart]

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 at ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at 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();
> >  };

-- 
Regards,

Laurent Pinchart