[PATCH v4 1/2] client audio: Support memfd transport

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

On Fri, 2016-04-15 at 23:06 +0200, Ahmed S. Darwish wrote:
> Now that all layers in the stack support memfd blocks, add memfd
> pools support for client context and audio playback data.
> 
> Use such memfd pools by default only if the server signals memfd
> support in its connection negotiations.
> 
> Also add ability for clients to force-disable memfd transport
> through the `enable-memfd=' client configuration option.
> 
> Signed-off-by: Ahmed S. Darwish <darwish.07 at gmail.com>
> ---
> 
> Notes:
>     [v4 changes]
>     - updated commit log: memfd used only upon server support
>     - protocol version is now bumped in next commit instead
> 
>  man/pulse-client.conf.5.xml.in  |  8 ++++++++
>  src/pulse/client-conf.c         |  1 +
>  src/pulse/client-conf.h         |  2 +-
>  src/pulse/context.c             | 44 +++++++++++++++++++++++++++++++++++++----
>  src/pulse/internal.h            |  3 +++
>  src/pulsecore/mem.h             |  9 +++++++++
>  src/pulsecore/protocol-native.c | 28 ++++++++++++++++++++++++--
>  7 files changed, 88 insertions(+), 7 deletions(-)
> 
> diff --git a/man/pulse-client.conf.5.xml.in b/man/pulse-client.conf.5.xml.in
> index cca2219..b88898c 100644
> --- a/man/pulse-client.conf.5.xml.in
> +++ b/man/pulse-client.conf.5.xml.in
> @@ -102,6 +102,14 @@ License along with PulseAudio; if not, see .
>  
>      
>        enable-shm= Enable data transfer via POSIX
> +      or memfd shared memory. Takes a boolean argument, defaults to
> +      yes. If set to no, communication with
> +      the server will be exclusively done through data-copy over
> +      sockets.
> +    
> +
> +    
> +      enable-memfd=. Enable data transfer via memfd
>        shared memory. Takes a boolean argument, defaults to
>        yes.
>      
> diff --git a/src/pulse/client-conf.c b/src/pulse/client-conf.c
> index c23aa6b..a3c9486 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 },
> +        { "enable-memfd",           pa_config_parse_not_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 ef39416..69be5f4 100644
> --- a/src/pulse/context.c
> +++ b/src/pulse/context.c
> @@ -173,7 +173,12 @@ 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;
> +    c->memfd_on_local = (!c->conf->disable_memfd && pa_memfd_is_locally_supported());
> +
> +    type = (c->conf->disable_shm) ? PA_MEM_TYPE_PRIVATE :
> +           ((!c->memfd_on_local) ?
> +               PA_MEM_TYPE_SHARED_POSIX : PA_MEM_TYPE_SHARED_MEMFD);
> +
>      if (!(c->mempool = pa_mempool_new(type, c->conf->shm_size, true))) {
>  
>          if (!c->conf->disable_shm) {
> @@ -482,6 +487,7 @@ static void setup_complete_callback(pa_pdispatch *pd, uint32_t command, uint32_t
>          case PA_CONTEXT_AUTHORIZING: {
>              pa_tagstruct *reply;
>              bool shm_on_remote = false;
> +            bool memfd_on_remote = false;
>  
>              if (pa_tagstruct_getu32(t, &c->version) < 0 ||
>                  !pa_tagstruct_eof(t)) {
> @@ -500,7 +506,15 @@ static void setup_complete_callback(pa_pdispatch *pd, uint32_t command, uint32_t
>                 not. */
>              if (c->version >= 13) {
>                  shm_on_remote = !!(c->version & 0x80000000U);
> -                c->version &= 0x7FFFFFFFU;
> +
> +                /* Starting with protocol version 31, the second MSB of the version
> +                 * tag reflects whether memfd is supported on the other PA end. */
> +                if (c->version >= 31)

You should be using (c->version & 0x40000000U) for the comparison to
get the actual protocol version.

Not a comment for this patch, but we should probably introduce some
macros to deal with this flag setting to keep things cleaner.

> +                    memfd_on_remote = !!(c->version & 0x40000000U);
> +
> +                /* Reserve the two most-significant _bytes_ of the version tag
> +                 * for flags. */
> +                c->version &= 0x0000FFFFU;
>              }
>  
>              pa_log_debug("Protocol version: remote %u, local %u", c->version, PA_PROTOCOL_VERSION);
> @@ -526,6 +540,26 @@ static void setup_complete_callback(pa_pdispatch *pd, uint32_t command, uint32_t
>              pa_log_debug("Negotiated SHM: %s", pa_yes_no(c->do_shm));
>              pa_pstream_enable_shm(c->pstream, c->do_shm);
>  
> +            c->shm_type = PA_MEM_TYPE_PRIVATE;
> +            if (c->do_shm) {
> +                if (c->version >= 31 && memfd_on_remote && c->memfd_on_local) {
> +                    const char *reason;
> +
> +                    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 to regester memfd mempool. Reason: %s", reason);
> +
> +                    /* Even if memfd pool registration fails, the negotiated SHM type
> +                     * shall remain memfd as both endpoints claim to support it. */
> +                    c->shm_type = PA_MEM_TYPE_SHARED_MEMFD;
> +                } else
> +                    c->shm_type = PA_MEM_TYPE_SHARED_POSIX;
> +            }
> +
> +            pa_log_debug("Memfd possible: %s", pa_yes_no(c->memfd_on_local));
> +            pa_log_debug("Negotiated SHM type: %s", pa_mem_type_to_string(c->shm_type));
> +
>              reply = pa_tagstruct_command(c, PA_COMMAND_SET_CLIENT_NAME, &tag);
>  
>              if (c->version >= 13) {
> @@ -593,8 +627,10 @@ static void setup_context(pa_context *c, pa_iochannel *io) {
>      pa_log_debug("SHM possible: %s", pa_yes_no(c->do_shm));
>  
>      /* Starting with protocol version 13 we use the MSB of the version
> -     * tag for informing the other side if we could do SHM or not */
> -    pa_tagstruct_putu32(t, PA_PROTOCOL_VERSION | (c->do_shm ? 0x80000000U : 0));
> +     * tag for informing the other side if we could do SHM or not.
> +     * Starting from version 31, second MSB is used to flag memfd support. */
> +    pa_tagstruct_putu32(t, PA_PROTOCOL_VERSION | (c->do_shm ? 0x80000000U : 0) |
> +                        (c->memfd_on_local ? 0x40000000 : 0));
>      pa_tagstruct_put_arbitrary(t, cookie, sizeof(cookie));
>  
>  #ifdef HAVE_CREDS
> diff --git a/src/pulse/internal.h b/src/pulse/internal.h
> index eefd181..9bbb903 100644
> --- a/src/pulse/internal.h
> +++ b/src/pulse/internal.h
> @@ -88,6 +88,7 @@ struct pa_context {
>  
>      bool is_local:1;
>      bool do_shm:1;
> +    bool memfd_on_local:1;
>      bool server_specified:1;
>      bool no_fail:1;
>      bool do_autospawn:1;
> @@ -95,6 +96,8 @@ struct pa_context {
>      bool filter_added:1;
>      pa_spawn_api spawn_api;
>  
> +    pa_mem_type_t shm_type;
> +
>      pa_strlist *server_list;
>  
>      char *server;
> diff --git a/src/pulsecore/mem.h b/src/pulsecore/mem.h
> index 11a8086..cba1410 100644
> --- a/src/pulsecore/mem.h
> +++ b/src/pulsecore/mem.h
> @@ -22,6 +22,7 @@
>  
>  #include 
>  
> +#include 
>  #include 
>  
>  typedef enum pa_mem_type {
> @@ -48,4 +49,12 @@ static inline bool pa_mem_type_is_shared(pa_mem_type_t t) {
>      return (t == PA_MEM_TYPE_SHARED_POSIX) || (t == PA_MEM_TYPE_SHARED_MEMFD);
>  }
>  
> +static inline bool pa_memfd_is_locally_supported() {
> +#if defined(HAVE_CREDS) && defined(HAVE_MEMFD)
> +    return true;
> +#else
> +    return false;
> +#endif
> +}
> +
>  #endif
> diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c
> index ffa5c4d..3f39431 100644
> --- a/src/pulsecore/protocol-native.c
> +++ b/src/pulsecore/protocol-native.c
> @@ -48,6 +48,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2676,7 +2677,9 @@ static void command_enable_srbchannel(pa_pdispatch *pd, uint32_t command, uint32
>  static void command_auth(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
>      pa_native_connection *c = PA_NATIVE_CONNECTION(userdata);
>      const void*cookie;
> +    bool memfd_on_remote = false;
>      pa_tagstruct *reply;
> +    pa_mem_type_t shm_type;
>      bool shm_on_remote = false, do_shm;
>  
>      pa_native_connection_assert_ref(c);
> @@ -2700,7 +2703,15 @@ static void command_auth(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_ta
>         not. */
>      if (c->version >= 13) {
>          shm_on_remote = !!(c->version & 0x80000000U);
> -        c->version &= 0x7FFFFFFFU;
> +
> +        /* Starting with protocol version 31, the second MSB of the version
> +         * tag reflects whether memfd is supported on the other PA end. */
> +        if (c->version >= 31)
> +            memfd_on_remote = !!(c->version & 0x40000000U);

Same comment as before here. If you're fine with it, I'll just patch
those two lines locally and push after some sanity testing.

-- Arun

> +
> +        /* Reserve the two most-significant _bytes_ of the version tag
> +         * for flags. */
> +        c->version &= 0x0000FFFFU;
>      }
>  
>      pa_log_debug("Protocol version: remote %u, local %u", c->version, PA_PROTOCOL_VERSION);
> @@ -2787,8 +2798,21 @@ static void command_auth(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_ta
>      pa_log_debug("Negotiated SHM: %s", pa_yes_no(do_shm));
>      pa_pstream_enable_shm(c->pstream, do_shm);
>  
> +    shm_type = PA_MEM_TYPE_PRIVATE;
> +    if (do_shm) {
> +        if (c->version >= 31 && memfd_on_remote && pa_memfd_is_locally_supported()) {
> +            pa_pstream_enable_memfd(c->pstream);
> +            shm_type = PA_MEM_TYPE_SHARED_MEMFD;
> +        } else
> +            shm_type = PA_MEM_TYPE_SHARED_POSIX;
> +
> +        pa_log_debug("Memfd possible: %s", pa_yes_no(pa_memfd_is_locally_supported()));
> +        pa_log_debug("Negotiated SHM type: %s", pa_mem_type_to_string(shm_type));
> +    }
> +
>      reply = reply_new(tag);
> -    pa_tagstruct_putu32(reply, PA_PROTOCOL_VERSION | (do_shm ? 0x80000000 : 0));
> +    pa_tagstruct_putu32(reply, PA_PROTOCOL_VERSION | (do_shm ? 0x80000000 : 0) |
> +                        (pa_memfd_is_locally_supported() ? 0x40000000 : 0));
>  
>  #ifdef HAVE_CREDS
>  {
[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux