[pulseaudio-discuss] [PATCH v2 11/12] pulse: client audio: Use memfd transport by default

[David Henningsson]

On 2016-02-12 01:20, Ahmed S. Darwish wrote:
> Now that all layers in the stack support memfd blocks, use memfd
> pools for client context and audio data by default.
>
> Signed-off-by: Ahmed S. Darwish <darwish.07 at gmail.com>
> ---
>   man/pulse-client.conf.5.xml.in |  5 +++++
>   src/pulse/client-conf.c        |  1 +
>   src/pulse/client-conf.h        |  2 +-
>   src/pulse/context.c            | 11 ++++++++++-
>   4 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/man/pulse-client.conf.5.xml.in b/man/pulse-client.conf.5.xml.in
> index cca2219..a04dedc 100644
> --- a/man/pulse-client.conf.5.xml.in
> +++ b/man/pulse-client.conf.5.xml.in
> @@ -107,6 +107,11 @@ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
>       </option>
>
>       <option>
> +      <p><opt>disable-memfd=</opt>. Disable memfd shared memory. Takes
> +      a boolean argument, defaults to <opt>no</opt>.</p>

Good idea.

> +    </option>
> +
> +    <option>
>         <p><opt>shm-size-bytes=</opt> Sets the shared memory segment
>         size for clients, in bytes. If left unspecified or is set to 0
>         it will default to some system-specific default, usually 64
> diff --git a/src/pulse/client-conf.c b/src/pulse/client-conf.c
> index c23aa6b..58259e4 100644
> --- a/src/pulse/client-conf.c
> +++ b/src/pulse/client-conf.c
> @@ -141,6 +141,7 @@ void pa_client_conf_load(pa_client_conf *c, bool load_from_x11, bool load_from_e
>           { "cookie-file",            pa_config_parse_string,   &c->cookie_file_from_client_conf, NULL },
>           { "disable-shm",            pa_config_parse_bool,     &c->disable_shm, NULL },
>           { "enable-shm",             pa_config_parse_not_bool, &c->disable_shm, NULL },
> +        { "disable-memfd",          pa_config_parse_bool,     &c->disable_memfd, NULL },
>           { "shm-size-bytes",         pa_config_parse_size,     &c->shm_size, NULL },
>           { "auto-connect-localhost", pa_config_parse_bool,     &c->auto_connect_localhost, NULL },
>           { "auto-connect-display",   pa_config_parse_bool,     &c->auto_connect_display, NULL },
> diff --git a/src/pulse/client-conf.h b/src/pulse/client-conf.h
> index eac705a..7691ec7 100644
> --- a/src/pulse/client-conf.h
> +++ b/src/pulse/client-conf.h
> @@ -37,7 +37,7 @@ typedef struct pa_client_conf {
>       bool cookie_from_x11_valid;
>       char *cookie_file_from_application;
>       char *cookie_file_from_client_conf;
> -    bool autospawn, disable_shm, auto_connect_localhost, auto_connect_display;
> +    bool autospawn, disable_shm, disable_memfd, auto_connect_localhost, auto_connect_display;
>       size_t shm_size;
>   } pa_client_conf;
>
> diff --git a/src/pulse/context.c b/src/pulse/context.c
> index 204c3e5..c31dcf4 100644
> --- a/src/pulse/context.c
> +++ b/src/pulse/context.c
> @@ -171,7 +171,10 @@ pa_context *pa_context_new_with_proplist(pa_mainloop_api *mainloop, const char *
>       c->srb_template.readfd = -1;
>       c->srb_template.writefd = -1;
>
> -    type = !c->conf->disable_shm ? PA_MEM_TYPE_SHARED_POSIX : PA_MEM_TYPE_PRIVATE;
> +    type = (c->conf->disable_shm) ? PA_MEM_TYPE_PRIVATE :
> +           ((c->conf->disable_memfd || !pa_memfd_is_locally_supported()) ?
> +               PA_MEM_TYPE_SHARED_POSIX : PA_MEM_TYPE_SHARED_MEMFD);
> +
>       if (!(c->mempool = pa_mempool_new(type, c->conf->shm_size))) {
>
>           if (!c->conf->disable_shm) {
> @@ -464,6 +467,7 @@ int pa_context_handle_error(pa_context *c, uint32_t command, pa_tagstruct *t, bo
>
>   static void setup_complete_callback(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
>       pa_context *c = userdata;
> +    const char *reason;
>
>       pa_assert(pd);
>       pa_assert(c);
> @@ -536,7 +540,12 @@ static void setup_complete_callback(pa_pdispatch *pd, uint32_t command, uint32_t
>               c->shm_type = PA_MEM_TYPE_PRIVATE;
>               if (c->do_shm) {
>                   if (c->version >= 31 && memfd_on_remote && pa_memfd_is_locally_supported()) {
> +
>                       pa_pstream_enable_memfd(c->pstream);
> +                    if (pa_mempool_is_memfd_backed(c->mempool))
> +                        if (pa_pstream_register_memfd_mempool(c->pstream, c->mempool, &reason))
> +                            pa_log("Failed regestering memfd mempool. Reason: %s", reason);

Nitpicks: "Failed to register memfd mempool". "const char* reason can 
move to just above "pa_pstream_enable_memfd".

> +
>                       c->shm_type = PA_MEM_TYPE_SHARED_MEMFD;

Is it correct to still have c->shm_type = PA_MEM_TYPE_SHARED_MEMFD even 
if registering the memfd failed? I think it is, but maybe this can be 
indicated better by adding a comment stating that.

>                   } else
>                       c->shm_type = PA_MEM_TYPE_SHARED_POSIX;
>
> Regards,
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic