Re: [PATCH] jack: use eventfd which never blocks.

["Alexander E. Patrakov" <patrakov@gmail.com>]

19.11.2010 19:22, Torben Hohn wrote:
> the use of the socketpair for synchronisation is not correct.
> jack does not allow blocking calls in the process_callback,
> the socket buffers are quite small, and seem to overflow under some
> conditions.

Some remarks.

First, your patch preserves the incorrect code for the case when eventfd 
is not available. While this is not a regression, I think you can do 
better (possibly in a separate patch).

Second, I disagree that socketpair()-based synchronization cannot be 
made correct and thus that eventfd is needed for correctness. It might 
be needed for efficiency, though.

> the result was write(2) blocking, and jack kicking the client.

So why not just make the file descriptor non-blocking? This way, instead 
of blocking the write(2), the socket pair would lose data when it gets 
too many writes. No problem - the reader will still see its file 
descriptor as active, eat a byte from the socket in 
snd_pcm_jack_poll_revents() and presumably do something to fill the 
audio buffer.

> (jack needs to be running in RT mode, with a pretty low period size (<=128)
> but this really is the normal usecase)
>
> this patch uses eventfd(2) when its available to mitigate the problem.
>
> Signed-off-by: Torben Hohn<torbenh@gmx.de>
> ---
>   configure.in    |    5 +++++
>   jack/pcm_jack.c |   30 +++++++++++++++++++++++++++---
>   2 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/configure.in b/configure.in
> index 5d71cae..9fa7f27 100644
> --- a/configure.in
> +++ b/configure.in
> @@ -21,6 +21,11 @@ AC_CHECK_LIB(asound, snd_pcm_ioplug_create,,
>   AC_ARG_ENABLE([jack],
>         AS_HELP_STRING([--disable-jack], [Disable building of JACK plugin]))
>
> +AC_CHECK_HEADER([sys/eventfd.h], [HAVE_EVENTFD=yes], [HAVE_EVENTFD=no])
> +if test "$HAVE_EVENTFD" = "yes"; then
> +	AC_DEFINE(HAVE_EVENTFD, 1,"eventfd is available")
> +fi
> +
>   if test "x$enable_jack" != "xno"; then
>     PKG_CHECK_MODULES(JACK, jack>= 0.98, [HAVE_JACK=yes], [HAVE_JACK=no])
>   fi
> diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
> index 3370a26..83ac56a 100644
> --- a/jack/pcm_jack.c
> +++ b/jack/pcm_jack.c
> @@ -20,6 +20,8 @@
>    *
>    */
>
> +#include "config.h"
> +
>   #include<byteswap.h>
>   #include<sys/shm.h>
>   #include<sys/types.h>
> @@ -28,6 +30,10 @@
>   #include<alsa/asoundlib.h>
>   #include<alsa/pcm_external.h>
>
> +#ifdef HAVE_EVENTFD
> +#include<sys/eventfd.h>
> +#endif
> +
>   typedef enum _jack_format {
>   	SND_PCM_JACK_FORMAT_RAW
>   } snd_pcm_jack_format_t;
> @@ -63,8 +69,10 @@ static void snd_pcm_jack_free(snd_pcm_jack_t *jack)
>   		}
>   		if (jack->fd>= 0)
>   			close(jack->fd);
> +#ifndef HAVE_EVENTFD
>   		if (jack->io.poll_fd>= 0)
>   			close(jack->io.poll_fd);
> +#endif
>   		free(jack->areas);
>   		free(jack);
>   	}
> @@ -81,11 +89,19 @@ static int snd_pcm_jack_poll_revents(snd_pcm_ioplug_t *io ATTRIBUTE_UNUSED,
>   				     struct pollfd *pfds, unsigned int nfds,
>   				     unsigned short *revents)
>   {
> +#ifdef HAVE_EVENTFD
> +	eventfd_t buf[1];
> +#else
>   	static char buf[1];
> -	
> +#endif
> +
>   	assert(pfds&&  nfds == 1&&  revents);
>
> -	read(pfds[0].fd, buf, 1);
> +	read(pfds[0].fd, buf, sizeof(buf));
> +#ifdef HAVE_EVENTFD
> +	buf[0] -= 1;
> +	write(pfds[0].fd, buf, sizeof(buf));
> +#endif
>
>   	*revents = pfds[0].revents;
>   	return 0;
> @@ -103,7 +119,11 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
>   	snd_pcm_jack_t *jack = io->private_data;
>   	const snd_pcm_channel_area_t *areas;
>   	snd_pcm_uframes_t xfer = 0;
> +#ifdef HAVE_EVENTFD
> +	static eventfd_t buf[1] = { 0x00000001 };
> +#else
>   	static char buf[1];
> +#endif
>   	unsigned int channel;
>   	
>   	for (channel = 0; channel<  io->channels; channel++) {
> @@ -143,7 +163,7 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
>   		xfer += frames;
>   	}
>
> -	write(jack->fd, buf, 1); /* for polling */
> +	write(jack->fd, buf, sizeof(buf)); /* for polling */
>
>   	return 0;
>   }
> @@ -361,7 +381,11 @@ static int snd_pcm_jack_open(snd_pcm_t **pcmp, const char *name,
>   		return -ENOMEM;
>   	}
>
> +#ifdef HAVE_EVENTFD
> +	fd[0] = fd[1] = eventfd(0, 0);
> +#else
>   	socketpair(AF_LOCAL, SOCK_STREAM, 0, fd);
> +#endif
>   	
>   	jack->fd = fd[0];
>