[pulseaudio-discuss] [PATCH 4/5] rtpoll: Implement pa_mainloop_api support

[Arun Raghavan]

On 4 March 2016 at 11:33, Tanu Kaskinen <tanuk at iki.fi> wrote:
>
> On Fri, 2016-03-04 at 09:45 +0530, Arun Raghavan wrote:
> > On Thu, 2016-03-03 at 18:02 +0200, Tanu Kaskinen wrote:
> > > On Tue, 2016-02-09 at 12:42 +0200, Tanu Kaskinen wrote:
> > > >
> > > > On Tue, 2016-02-09 at 15:47 +0530, Arun Raghavan wrote:
> > > > >
> > > > > On Fri, 2015-01-02 at 15:04 +0200, Tanu Kaskinen wrote:
> > > > > >
> > > > > > +}
> > > > > > +
> > > > > > +static void find_expired_time_events(pa_rtpoll *rtpoll) {
> > > > > > +    pa_usec_t now;
> > > > > > +    pa_time_event *event;
> > > > > > +    unsigned idx;
> > > > > > +
> > > > > > +    pa_assert(rtpoll);
> > > > > > +    pa_assert(pa_dynarray_size(rtpoll->expired_time_events) == 0);
> > > > > > +
> > > > > > +    now = pa_rtclock_now();
> > > > > > +
> > > > > > +    PA_DYNARRAY_FOREACH(event, rtpoll->enabled_time_events, idx) {
> > > > > > +        if (event->time <= now)
> > > > > > +            pa_dynarray_append(rtpoll->expired_time_events, event);
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > > +static pa_time_event *find_next_time_event(pa_rtpoll *rtpoll) {
> > > > > > +    pa_time_event *event;
> > > > > > +    pa_time_event *result = NULL;
> > > > > > +    unsigned idx;
> > > > > > +
> > > > > > +    pa_assert(rtpoll);
> > > > > > +
> > > > > > +    if (rtpoll->cached_next_time_event)
> > > > > > +        return rtpoll->cached_next_time_event;
> > > > > > +
> > > > > > +    PA_DYNARRAY_FOREACH(event, rtpoll->enabled_time_events, idx) {
> > > > > > +        if (!result || event->time < result->time)
> > > > > > +            result = event;
> > > > > > +    }
> > > > > > +
> > > > > > +    rtpoll->cached_next_time_event = result;
> > > > > > +
> > > > > > +    return result;
> > > > > > +}
> > > > > > +
> > > > > >  static void reset_revents(pa_rtpoll_item *i) {
> > > > > >      struct pollfd *f;
> > > > > >      unsigned n;
> > > > > > @@ -204,9 +632,14 @@ static void reset_all_revents(pa_rtpoll *p) {
> > > > > >  }
> > > > > >
> > > > > >  int pa_rtpoll_run(pa_rtpoll *p) {
> > > > > > +    pa_defer_event *defer_event;
> > > > > > +    pa_time_event *time_event;
> > > > > >      pa_rtpoll_item *i;
> > > > > >      int r = 0;
> > > > > >      struct timeval timeout;
> > > > > > +    pa_time_event *next_time_event;
> > > > > > +    struct timeval next_time_event_elapse;
> > > > > > +    bool timer_enabled;
> > > > > >
> > > > > >      pa_assert(p);
> > > > > >      pa_assert(!p->running);
> > > > > > @@ -218,7 +651,28 @@ int pa_rtpoll_run(pa_rtpoll *p) {
> > > > > >      p->running = true;
> > > > > >      p->timer_elapsed = false;
> > > > > >
> > > > > > -    /* First, let's do some work */
> > > > > > +    /* Dispatch all enabled defer events. */
> > > > > > +    while ((defer_event = pa_dynarray_last(p-
> > > > > > >
> > > > > > > enabled_defer_events))) {
> > > > > > +        if (p->quit)
> > > > > > +            break;
> > > > > > +
> > > > > > +        defer_event->callback(&p->mainloop_api, defer_event,
> > > > > > defer_event->userdata);
> > > > > > +    }
> > > > > Am I missing something, or is this an infinite loop if there are any
> > > > > enabled defer events?
> > > > As discussed in IRC, I did this like this, because pa_mainloop_run() in
> > > > practice behaves the same way. However, mainloop-api.h says that each
> > > > defer event runs only once per mainloop iteration, so I'll have to
> > > > change this.
> > > I started looking into this, and realized that the "once per mainloop
> > > iteration" clause makes no difference to pa_mainloop_api users. There
> > > is no pa_rtpoll_iterate(), and pa_rtpoll_run() will behave the same way
> > > in any case, so is it really worth the effort to artificially refactor
> > > the code to have "iterations"?
> >
> > But there is a difference of order, right? The mainloop API idea of an
> > iteration is not just about pa_mainloop_iterate(), but also about
> > something like:
> >
> >   1. Run all I/O events
> >   2. Run all expired time events
> >   3. Run all defer events
> >   4. Goto 1
> >
> > The order is not something we commit to, but the fact that we do one
> > run of each is. Your code runs a loop in (3) until the defer event
> > basically goes away.
>
> That's not how pa_mainloop works. If there are any defer events enabled
> when pa_mainloop_iterate() is called, one of each is run, and none of
> time and IO events. In practice this means that pa_mainloop_run()
> processes defer events as long as there are any enabled, and only then
> it processes each expired time and IO event once.

Ugh, that's terrible. So our documentation of the mainloop (which has
a reasonable definition of these concepts) is completely different
from our implementation (which seems ad-hoc, and all our mainloop
implementations reproduce the behaviour).

> > From what I can tell, this should not required a radical restructuring.
> > Instead of always picking pa_dynarray_last(), you could just iterate
> > over all the enabled events once.
>
> That would mean processing time and IO events more aggressively than
> what pa_mainloop does, and I tried to avoid differing from pa_mainloop
> behaviour.

Okay, so that part of my comment on your patch stands invalidated.
It's better that all our implementations are inconsistent with the
documentation and reason in the same way.

We'll just have to fix it all at one go at some point.

-- Arun