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
