[pulseaudio-discuss] [PATCH RFCv3 25/51] mainloop: Clear wakeup pipe only when necessary

[Peter Meerwald]

Hi,

> > probably not in a measureable way;
> 
> Hmm, but yet you listed this one as one of your "critical patches"...?

ah, critical in the sense that it might break something, needs review...

for most patches it is hard to say what the impact is (besides argueing 
that at malloc(), read(), etc. is saved, sometimes at the expense of a 
memcpy())

> > though about it as well; I micro-benchmarked pipe() vs. eventfd()
> > write-poll-read and there was only a slight benefit for eventfd
> > 
> > since the fdsem would be used in poll, there would be almost-always
> > someone waiting?
> 
> So, consider the case where someone calls pa_mainloop_wakeup when it does not
> need to be woken up.
> 
> In that case, doing clear_wakeup as late as possible is preferable, because
> then you don't have to poll just to wakeup because the pipe is readable.
> 
> In addition, pa_fdsem would deal with this situation by not doing anything at
> all on the writer side, because it knows noone is waiting on the reader side.
> 
> To sum up, it seems to me like pa_fdsem is the best tool for the job. But if
> you don't want to consider that, how about setting a variable in
> dispatch_pollfds that indicates that the pipe should be cleared in
> pa_mainloop_prepare?

I see the point of clearing as late as possible

I can try with pa_fdsem and measure how often the atomic guards saves 
writing the eventfd, let's see...

thank you for the comments!

regards, p.

> > > > Signed-off-by: Peter Meerwald <pmeerw at pmeerw.net>
> > > > ---
> > > >    src/pulse/mainloop.c | 24 ++++++++++++++----------
> > > >    1 file changed, 14 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/src/pulse/mainloop.c b/src/pulse/mainloop.c
> > > > index c7a5236..a7a3c48 100644
> > > > --- a/src/pulse/mainloop.c
> > > > +++ b/src/pulse/mainloop.c
> > > > @@ -635,6 +635,15 @@ static void rebuild_pollfds(pa_mainloop *m) {
> > > >        m->rebuild_pollfds = false;
> > > >    }
> > > > 
> > > > +static void clear_wakeup(pa_mainloop *m) {
> > > > +    char c[10];
> > > > +
> > > > +    pa_assert(m);
> > > > +
> > > > +    while (pa_read(m->wakeup_pipe[0], &c, sizeof(c),
> > > > &m->wakeup_pipe_type)
> > > == sizeof(c))
> > > > +        ;
> > > > +}
> > > > +
> > > >    static unsigned dispatch_pollfds(pa_mainloop *m) {
> > > >        pa_io_event *e;
> > > >        unsigned r = 0, k;
> > > > @@ -642,6 +651,11 @@ static unsigned dispatch_pollfds(pa_mainloop *m) {
> > > >        pa_assert(m->poll_func_ret > 0);
> > > > 
> > > >        k = m->poll_func_ret;
> > > > +    if (m->pollfds[0].revents) {
> > > > +        clear_wakeup(m);
> > > > +        m->pollfds[0].revents = 0;
> > > > +        k--;
> > > > +    }
> > > > 
> > > >        PA_LLIST_FOREACH(e, m->io_events) {
> > > > 
> > > > @@ -775,20 +789,10 @@ void pa_mainloop_wakeup(pa_mainloop *m) {
> > > >            pa_log("pa_write() failed while trying to wake up the
> > > > mainloop:
> > > %s", pa_cstrerror(errno));
> > > >    }
> > > > 
> > > > -static void clear_wakeup(pa_mainloop *m) {
> > > > -    char c[10];
> > > > -
> > > > -    pa_assert(m);
> > > > -
> > > > -    while (pa_read(m->wakeup_pipe[0], &c, sizeof(c),
> > > > &m->wakeup_pipe_type)
> > > == sizeof(c))
> > > > -        ;
> > > > -}
> > > > -
> > > >    int pa_mainloop_prepare(pa_mainloop *m, int timeout) {
> > > >        pa_assert(m);
> > > >        pa_assert(m->state == STATE_PASSIVE);
> > > > 
> > > > -    clear_wakeup(m);
> > > >        scan_dead(m);
> > > > 
> > > >        if (m->quit)
> > > > 
> > > 
> > > 
> > 
> 
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)