[pulseaudio-discuss] [PATCH v4 1/2] client audio: Support memfd transport
Arun Raghavan
arun at accosted.net
Mon Apr 25 12:06:23 UTC 2016
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
> > {
More information about the pulseaudio-discuss
mailing list