[PATCH libinput] timer: drain data on the timerfd when it triggers

[Peter Hutterer]

On Thu, Apr 30, 2015 at 09:24:41AM -0500, Derek Foreman wrote:
> On 29/04/15 08:33 PM, Peter Hutterer wrote:
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > I admit, I'm not sure of the effect it has leaving the data sitting there.
> > timerfd_create states that only the value read changes, not the actual size
> > of the data (which I couldn't reproduce in a quick test, it's always 1)
> > epoll sounds like it should keep triggering as long as data is sitting there
> > but it doesn't. Didn't have time to investigate further, so here's the patch
> > for what seems to be the correct thing to do.
> > 
> >  src/timer.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/src/timer.c b/src/timer.c
> > index f6c8e42..285f75b 100644
> > --- a/src/timer.c
> > +++ b/src/timer.c
> > @@ -101,6 +101,9 @@ libinput_timer_handler(void *data)
> >  	struct libinput *libinput = data;
> >  	struct libinput_timer *timer, *tmp;
> >  	uint64_t now;
> > +	uint64_t discard;
> > +
> > +	read(libinput->timer.fd, &discard, sizeof(discard));
> 
> In some cases I think this can lead to a deadlock.  When you reset a
> timerfd it empties out the data.  

oh, right, this explains why I kept getting only one value even when
multiple timers had expired.

> So the following order of events:
> 
> Timer fires (fd becomes readable)
> change timer for some reason
> epoll
>   fd did change, call timer handler
> 
> At this point the read blocks until next timer fire (if you're lucky) or
> forever if no timer is set.
> 
> 
> I *think* you need this to make it safe...
> 
> diff --git a/src/timer.c b/src/timer.c
> index 285f75b..114a649 100644
> --- a/src/timer.c
> +++ b/src/timer.c
> @@ -122,7 +122,8 @@ libinput_timer_handler(void *data)
>  int
>  libinput_timer_subsys_init(struct libinput *libinput)
>  {
> -	libinput->timer.fd = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
> +	libinput->timer.fd = timerfd_create(CLOCK_MONOTONIC,
> +					    TFD_CLOEXEC | TFD_NONBLOCK);
>  	if (libinput->timer.fd < 0)
>  		return -1;
 
yep, I'll merge that in thanks.
 
> I don't know if you want to do anything in the handler when you catch an
> EAGAIN (which indicates the timer was reset between trigger and epoll...).

That can actually happen but shouldn't be an error, I think. We pull the
events off the devices in a loop, so there's the chance of the timer
triggering while we're in that loop and one subsequent event may cause e.g.
a tap timer reset. not 100% of the effect that has on the timerfd behaviour
though but it's an unrelated problem that we need to fix anyway to avoid
starving the timerfd.

Cheers,
   Peter

> >  
> >  	now = libinput_now(libinput);
> >  	if (now == 0)
> > 
>