[pulseaudio-discuss] [PATCH v2 09/12] pulsecore: pstream: Support memfd blocks transport

[David Henningsson]

On 2016-02-12 01:19, Ahmed S. Darwish wrote:
> Now that we have the necessary infrastructure for memexporting and
> mempimporting a memfd memblock, extend that support higher up in the
> chain with pstreams.
>
> A PulseAudio endpoint can now _transparently_ send a memfd memblock
> to the other end by simply calling pa_pstream_send_memblock().

...provided the memfds are first registered.

>
> If the pipe does not support memfd trannsfers, we fall back to
> sending the full block's data instead of just its reference.
>
> # Further details:
>
> A single pstream connection usually transfers blocks from multiple
> pools including the server's srbchannel mempool, the client's audio
> data mempool, and the server's shared core mempool.
>
> If these mempools are memfd-backed, we now require registering them
> with the pstream before sending any blocks they cover. This is done
> to minimize fd passing overhead and the possibility of fd leaks.
>
> Moreover, to support all these pools without hard-coding their number
> (or nature) in the Pulse communication protocol, a new pstream packet
> type is introduced. That special packet can be sent _anytime_ during
> the pstrem's lifetime and is used for creating on demand SHM ID to
> memfd mappings.
>
> Signed-off-by: Ahmed S. Darwish <darwish.07 at gmail.com>
> ---
>   src/pulsecore/pstream.c | 257 +++++++++++++++++++++++++++++++++++++++++++-----
>   src/pulsecore/pstream.h |   2 +
>   2 files changed, 233 insertions(+), 26 deletions(-)
>
> diff --git a/src/pulsecore/pstream.c b/src/pulsecore/pstream.c
> index ef2bbf9..11ea7f3 100644
> --- a/src/pulsecore/pstream.c
> +++ b/src/pulsecore/pstream.c
> @@ -38,16 +38,20 @@
>   #include <pulsecore/creds.h>
>   #include <pulsecore/refcnt.h>
>   #include <pulsecore/flist.h>
> +#include <pulsecore/hashmap.h>
>   #include <pulsecore/macro.h>
>
>   #include "pstream.h"
>
>   /* We piggyback information if audio data blocks are stored in SHM on the seek mode */
>   #define PA_FLAG_SHMDATA     0x80000000LU
> +#define PA_FLAG_SHMDATA_MEMFD_BLOCK         0x20000000LU
>   #define PA_FLAG_SHMRELEASE  0x40000000LU
>   #define PA_FLAG_SHMREVOKE   0xC0000000LU
>   #define PA_FLAG_SHMMASK     0xFF000000LU
>   #define PA_FLAG_SEEKMASK    0x000000FFLU
> +#define PA_FLAG_PACKET_MASK 0x00000F00LU
> +#define PA_FLAG_PACKET_SHMID_TO_MEMFD_FD    0x00000100LU
>   #define PA_FLAG_SHMWRITABLE 0x00800000LU
>
>   /* The sequence descriptor header consists of 5 32bit integers: */
> @@ -92,6 +96,9 @@ struct item_info {
>
>       /* packet info */
>       pa_packet *packet;
> +    bool shmid_to_memfd_packet:1;

This seems a bit strange to me. When you set up a srbchannel, there is a 
special packet sent (PA_COMMAND_ENABLE_SRBCHANNEL) which has two fds as 
ancil data.

Would it not make sense (and lead to less code changes, probably) if 
this followed a similar scheme, i e, add a new command 
(PA_COMMAND_SHMID_FD or something) that would have an memfd as ancil 
data and the ID as packet content?

As a side note, we could potentially optimise setting up an srbchannel 
by sending all three memfds in PA_COMMAND_ENABLE_SRBCHANNEL, it'll be 
one less package to send...

> +    bool close_memfd_fd_after_send:1;
> +
>   #ifdef HAVE_CREDS
>       bool with_ancil_data;
>       pa_cmsg_ancil_data ancil_data;
> @@ -147,6 +154,10 @@ struct pa_pstream {
>       pa_memimport *import;
>       pa_memexport *export;
>
> +    /* This pipe supports sending memfd fd as ancillary data */
> +    bool use_memfd;
> +    pa_hashmap *registered_memfd_ids;

This hashmap could use a comment (maps from what to what? does it own 
any fds that should be closed?).

Also, it seems this hashmap is never freed anywhere...

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