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

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

 

On Mon, 2016-04-25 at 16:30 +0530, Arun Raghavan wrote:
> 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.

That should read (c->version & 0x0000FFFFU).

-- Arun

> 
> 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