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

[Kieran Bingham]

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 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?


> +	} 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();
>  };
> 

-- 
Regards
--
Kieran