On 2016-02-12 01:15, Ahmed S. Darwish wrote:
> Add support for transfering memfd-backed blocks without passing their
> file descriptor every time, thus minimizing overhead and the
> possibility of fd leaks.
>
> To accomplish this, a new type of 'permanent' segments is introduced.
>
> Suggested-by: David Henningsson <david.henningsson at canonical.com>
> Signed-off-by: Ahmed S. Darwish <darwish.07 at gmail.com>
> ---
> src/pulsecore/memblock.c | 134 +++++++++++++++++++++++++++++++++++++++++++----
> src/pulsecore/memblock.h | 7 ++-
> src/pulsecore/shm.c | 7 +--
> src/pulsecore/shm.h | 2 +-
> 4 files changed, 132 insertions(+), 18 deletions(-)
>
> diff --git a/src/pulsecore/memblock.c b/src/pulsecore/memblock.c
> index 154bd67..959453e 100644
> --- a/src/pulsecore/memblock.c
> +++ b/src/pulsecore/memblock.c
> @@ -100,6 +100,19 @@ struct pa_memimport_segment {
> pa_memtrap *trap;
> unsigned n_blocks;
> bool writable;
> + /* If true, this segment's lifetime will not be limited by the
> + * number of active blocks (n_blocks) using its shared memory.
> + * Rather, it will exist for the full lifetime of the memimport.
> + *
> + * This is done to support SHM memfd blocks transport.
> + *
> + * To transfer memfd-backed blocks without passing their fd every
> + * time, thus minimizing overhead and the possibility of fd leaks,
> + * a packet is sent with the memfd fd as ancil data very early on.
> + * This packet has an ID that identifies the memfd region. Once
> + * received, a permanent mapping is added to the memimport's
> + * segments hash. */
> + bool permanent;
Do you need to add this? It looks like you're almost never read this
variable, except for in some assertions - and those assertions could
perhaps be replaced with an "pa_assert(seg->memory.type = MEMFD)" instead?
> };
>
> /* A collection of multiple segments */
> @@ -926,12 +939,46 @@ int pa_mempool_get_shm_id(pa_mempool *p, uint32_t *id) {
> }
>
> /* No lock necessary */
> -bool pa_mempool_is_shared(pa_mempool *p) {
> +bool pa_mempool_is_shared(const pa_mempool *p) {
> pa_assert(p);
>
> return p->shared;
> }
>
> +/* No lock necessary */
> +bool pa_mempool_is_memfd_backed(const pa_mempool *p) {
> + pa_assert(p);
> +
> + return pa_mempool_is_shared(p) &&
> + (p->per_type.shm.type == PA_MEM_TYPE_SHARED_MEMFD);
> +}
> +
> +/* Self-locked
> + *
> + * Check the comments over pa_shm->per_type.memfd.fd for context.
> + *
> + * After this method's return, the caller owns the file descriptor
> + * and is responsible for closing it in the appropriate time. This
> + * should only be called once during during a mempool's lifetime. */
> +int pa_mempool_get_and_reset_shm_memfd_fd(pa_mempool *p) {
Nitpick: pa_mempool_take_memfd_fd() is a shorter and more obvious name.
But there's something else I don't get here. A memimport has only one
pool, but more than one segment.
Can some of these segments be memfd and others be posix_shm (e g, one
sandboxed app has the new memfd stuff, and an older client outside the
sandbox uses shm)?
If so, why are you taking the memfd out of the pool, rather than out of
the segment?
> + pa_shm *memory;
> + int memfd_fd;
> +
> + pa_assert(pa_mempool_is_shared(p));
> + pa_assert(pa_mempool_is_memfd_backed(p));
> +
> + pa_mutex_lock(p->mutex);
> +
> + memory = &p->per_type.shm;
> + memfd_fd = memory->per_type.memfd.fd;
> + memory->per_type.memfd.fd = -1;
> +
> + pa_mutex_unlock(p->mutex);
> +
> + pa_assert(memfd_fd != -1);
> + return memfd_fd;
> +}
> +
> /* For receiving blocks from other nodes */
> pa_memimport* pa_memimport_new(pa_mempool *p, pa_memimport_release_cb_t cb, void *userdata) {
> pa_memimport *i;
> @@ -957,15 +1004,17 @@ pa_memimport* pa_memimport_new(pa_mempool *p, pa_memimport_release_cb_t cb, void
> static void memexport_revoke_blocks(pa_memexport *e, pa_memimport *i);
>
> /* Should be called locked */
> -static pa_memimport_segment* segment_attach(pa_memimport *i, uint32_t shm_id, bool writable) {
> +static pa_memimport_segment* segment_attach(pa_memimport *i, pa_mem_type_t type, uint32_t shm_id,
> + int memfd_fd, bool writable) {
> pa_memimport_segment* seg;
> + pa_assert(pa_mem_type_is_shared(type));
>
> if (pa_hashmap_size(i->segments) >= PA_MEMIMPORT_SEGMENTS_MAX)
> return NULL;
>
> seg = pa_xnew0(pa_memimport_segment, 1);
>
> - if (pa_shm_attach(&seg->memory, shm_id, writable) < 0) {
> + if (pa_shm_attach(&seg->memory, type, shm_id, memfd_fd, writable) < 0) {
> pa_xfree(seg);
> return NULL;
> }
> @@ -981,6 +1030,7 @@ static pa_memimport_segment* segment_attach(pa_memimport *i, uint32_t shm_id, bo
> /* Should be called locked */
> static void segment_detach(pa_memimport_segment *seg) {
> pa_assert(seg);
> + pa_assert(seg->n_blocks == ((seg->permanent) ? 1 : 0));
Not sure, but I wonder if this is a good idea. Could we change this to a
pa_log_error for now? Probably we want to cleanup rather than crash here.
>
> pa_hashmap_remove(seg->import->segments, PA_UINT32_TO_PTR(seg->memory.id));
> pa_shm_free(&seg->memory);
> @@ -995,6 +1045,8 @@ static void segment_detach(pa_memimport_segment *seg) {
> void pa_memimport_free(pa_memimport *i) {
> pa_memexport *e;
> pa_memblock *b;
> + pa_memimport_segment *seg;
> + void *state = NULL;
>
> pa_assert(i);
>
> @@ -1003,6 +1055,15 @@ void pa_memimport_free(pa_memimport *i) {
> while ((b = pa_hashmap_first(i->blocks)))
> memblock_replace_import(b);
>
> + /* Permanent segments exist for the lifetime of the memimport. Now
> + * that we're freeing the memimport itself, clear em all up.
> + *
> + * Careful! segment_detach() internally removes itself from the
> + * memimport's hash; the same hash we're now using for iteration. */
> + PA_HASHMAP_FOREACH(seg, i->segments, state) {
> + if (seg->permanent)
> + segment_detach(seg);
> + }
I don't think that's safe. Better do something like:
while (seg = pa_hashmap_first(i->segments)) {
pa_assert(seg->permanent);
segment_detach(seg);
}
> pa_assert(pa_hashmap_size(i->segments) == 0);
>
> pa_mutex_unlock(i->mutex);
> @@ -1025,9 +1086,38 @@ void pa_memimport_free(pa_memimport *i) {
> pa_xfree(i);
> }
>
> +/* Introduce a new mapping entry: SHM ID to memory mapped memfd region.
> + * For further details, check comments at `pa_shm->per_type.memfd.fd'
> + * and on top of `pa_memimport_segment.permanent' for context. */
> +int pa_memimport_add_permanent_shmid_to_memfd_mapping(pa_memimport *i, uint32_t shm_id, int memfd_fd,
> + bool writable)
or perhaps pa_memimport_attach_memfd() ?
> +{
> + pa_memimport_segment *seg;
> + int ret = -1;
> +
> + pa_assert(i);
> + pa_assert(memfd_fd != -1);
> +
> + pa_mutex_lock(i->mutex);
> +
> + if (!(seg = segment_attach(i, PA_MEM_TYPE_SHARED_MEMFD, shm_id, memfd_fd, writable)))
> + goto finish;
> +
> + /* n_blocks acts as a segment reference count. To avoid the segment
> + * being deleted when receiving silent memchunks, etc., mark our
> + * permanent presence by incrementing that refcount. */
> + seg->n_blocks++;
> + seg->permanent = true;
> + ret = 0;
> +
> +finish:
> + pa_mutex_unlock(i->mutex);
> + return ret;
> +}
> +
> /* Self-locked */
> -pa_memblock* pa_memimport_get(pa_memimport *i, uint32_t block_id, uint32_t shm_id,
> - size_t offset, size_t size, bool writable) {
> +static pa_memblock* NEW_API_pa_memimport_get(pa_memimport *i, pa_mem_type_t type, uint32_t block_id, uint32_t shm_id,
> + size_t offset, size_t size, bool writable) {
> pa_memblock *b = NULL;
> pa_memimport_segment *seg;
>
> @@ -1043,12 +1133,17 @@ pa_memblock* pa_memimport_get(pa_memimport *i, uint32_t block_id, uint32_t shm_i
> if (pa_hashmap_size(i->blocks) >= PA_MEMIMPORT_SLOTS_MAX)
> goto finish;
>
> - if (!(seg = pa_hashmap_get(i->segments, PA_UINT32_TO_PTR(shm_id))))
> - if (!(seg = segment_attach(i, shm_id, writable)))
> + if (!(seg = pa_hashmap_get(i->segments, PA_UINT32_TO_PTR(shm_id)))) {
> + if (type == PA_MEM_TYPE_SHARED_MEMFD) {
> + pa_log("Bailing out! Memfd segment with ID %u should've been earlier cached", shm_id);
> goto finish;
> + }
> + if (!(seg = segment_attach(i, type, shm_id, -1, writable)))
> + goto finish;
> + }
>
> - if (writable != seg->writable) {
> - pa_log("Cannot open segment - writable status changed!");
> + if (writable && !seg->writable) {
> + pa_log("Cannot import cached segment in write mode - previously mapped as read-only!");
> goto finish;
> }
>
> @@ -1082,6 +1177,11 @@ finish:
> return b;
> }
>
> +pa_memblock* pa_memimport_get(pa_memimport *i, uint32_t block_id, uint32_t shm_id,
> + size_t offset, size_t size, bool writable) {
> + return NEW_API_pa_memimport_get(i, PA_MEM_TYPE_SHARED_POSIX, block_id, shm_id, offset, size, writable);
> +}
> +
> int pa_memimport_process_revoke(pa_memimport *i, uint32_t id) {
> pa_memblock *b;
> int ret = 0;
> @@ -1238,7 +1338,8 @@ static pa_memblock *memblock_shared_copy(pa_mempool *p, pa_memblock *b) {
> }
>
> /* Self-locked */
> -int pa_memexport_put(pa_memexport *e, pa_memblock *b, uint32_t *block_id, uint32_t *shm_id, size_t *offset, size_t * size) {
> +static int NEW_API_pa_memexport_put(pa_memexport *e, pa_memblock *b, pa_mem_type_t *type, uint32_t *block_id,
> + uint32_t *shm_id, size_t *offset, size_t * size) {
> pa_shm *memory;
> struct memexport_slot *slot;
> void *data;
> @@ -1282,12 +1383,14 @@ int pa_memexport_put(pa_memexport *e, pa_memblock *b, uint32_t *block_id, uint32
> } else {
> pa_assert(b->type == PA_MEMBLOCK_POOL || b->type == PA_MEMBLOCK_POOL_EXTERNAL);
> pa_assert(b->pool);
> + pa_assert(pa_mempool_is_shared(b->pool));
> memory = &b->pool->per_type.shm;
> }
>
> pa_assert(data >= memory->mem.ptr);
> pa_assert((uint8_t*) data + b->length <= (uint8_t*) memory->mem.ptr + memory->mem.size);
>
> + *type = memory->type;
> *shm_id = memory->id;
> *offset = (size_t) ((uint8_t*) data - (uint8_t*) memory->mem.ptr);
> *size = b->length;
> @@ -1299,3 +1402,14 @@ int pa_memexport_put(pa_memexport *e, pa_memblock *b, uint32_t *block_id, uint32
>
> return 0;
> }
> +
> +int pa_memexport_put(pa_memexport *e, pa_memblock *b, uint32_t *block_id, uint32_t *shm_id,
> + size_t *offset, size_t *size) {
> + pa_mem_type_t type;
> + int ret;
> +
> + if (!(ret = NEW_API_pa_memexport_put(e, b, &type, block_id, shm_id, offset, size)))
> + pa_assert(type == PA_MEM_TYPE_SHARED_POSIX);
> +
> + return ret;
> +}
> diff --git a/src/pulsecore/memblock.h b/src/pulsecore/memblock.h
> index a760b6f..776b017 100644
> --- a/src/pulsecore/memblock.h
> +++ b/src/pulsecore/memblock.h
> @@ -127,14 +127,19 @@ void pa_mempool_free(pa_mempool *p);
> const pa_mempool_stat* pa_mempool_get_stat(pa_mempool *p);
> void pa_mempool_vacuum(pa_mempool *p);
> int pa_mempool_get_shm_id(pa_mempool *p, uint32_t *id);
> -bool pa_mempool_is_shared(pa_mempool *p);
> +bool pa_mempool_is_shared(const pa_mempool *p);
> +bool pa_mempool_is_memfd_backed(const pa_mempool *p);
> bool pa_mempool_is_remote_writable(pa_mempool *p);
> void pa_mempool_set_is_remote_writable(pa_mempool *p, bool writable);
> size_t pa_mempool_block_size_max(pa_mempool *p);
>
> +int pa_mempool_get_and_reset_shm_memfd_fd(pa_mempool *p);
> +
> /* For receiving blocks from other nodes */
> pa_memimport* pa_memimport_new(pa_mempool *p, pa_memimport_release_cb_t cb, void *userdata);
> void pa_memimport_free(pa_memimport *i);
> +int pa_memimport_add_permanent_shmid_to_memfd_mapping(pa_memimport *i, uint32_t shm_id, int memfd_fd,
> + bool writable);
> pa_memblock* pa_memimport_get(pa_memimport *i, uint32_t block_id, uint32_t shm_id,
> size_t offset, size_t size, bool writable);
> int pa_memimport_process_revoke(pa_memimport *i, uint32_t block_id);
> diff --git a/src/pulsecore/shm.c b/src/pulsecore/shm.c
> index 0ca4fb0..7ebe32a 100644
> --- a/src/pulsecore/shm.c
> +++ b/src/pulsecore/shm.c
> @@ -351,7 +351,7 @@ fail:
>
> /* Note: In case of attaching memfd SHM areas, the caller maintains ownership
> * of the passed fd and has the responsibility of closing it when appropriate. */
> -static int NEW_API_pa_shm_attach(pa_shm *m, pa_mem_type_t type, unsigned id, int memfd_fd, bool writable) {
> +int pa_shm_attach(pa_shm *m, pa_mem_type_t type, unsigned id, int memfd_fd, bool writable) {
> #if defined(HAVE_SHM_OPEN) || defined(HAVE_MEMFD)
> return shm_attach(m, type, id, memfd_fd, writable, false);
> #else
> @@ -359,11 +359,6 @@ static int NEW_API_pa_shm_attach(pa_shm *m, pa_mem_type_t type, unsigned id, int
> #endif
> }
>
> -/* Compatibility version until the new API is used in external sources */
> -int pa_shm_attach(pa_shm *m, unsigned id, bool writable) {
> - return NEW_API_pa_shm_attach(m, PA_MEM_TYPE_SHARED_POSIX, id, -1, writable);
> -}
> -
> int pa_shm_cleanup(void) {
>
> #ifdef HAVE_SHM_OPEN
> diff --git a/src/pulsecore/shm.h b/src/pulsecore/shm.h
> index 1c8ce83..e83a36c 100644
> --- a/src/pulsecore/shm.h
> +++ b/src/pulsecore/shm.h
> @@ -50,7 +50,7 @@ typedef struct pa_shm {
> } pa_shm;
>
> int pa_shm_create(pa_shm *m, pa_mem_type_t type, size_t size, mode_t mode);
> -int pa_shm_attach(pa_shm *m, unsigned id, bool writable);
> +int pa_shm_attach(pa_shm *m, pa_mem_type_t type, unsigned id, int memfd_fd, bool writable);
>
> void pa_shm_punch(pa_shm *m, size_t offset, size_t size);
>
> Regards,
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
