[pulseaudio-discuss] [PATCH v2 06/12] pulsecore: memexport/memimport: Support memfd blocks

[Ahmed S. Darwish]

Hi :-)

On Tue, Feb 16, 2016 at 03:08:03PM +0100, David Henningsson wrote:
> 
> 
> On 2016-02-12 01:15, Ahmed S. Darwish wrote:
...
> >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?
>

No problem. Variable is now removed and below macro is used
instead:

static bool segment_is_permanent(pa_memimport_segment *seg) {
    pa_assert(seg);
    return seg->memory.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.
>

That's a much better name indeed. Now used.

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

Unless I'm missing something obvious, here's why the situation
above cannot hold:

1. A memimport is actually owned by the pstream, and not by any
   mempool. This is evidinced by the fact that there's only a
   single pa_memimport_new() call in the entire PA code base and
   that call resides at:

   pa_pstream *pa_pstream_new() {
       pa_pstream *p;
       ...
       p->memimport = pa_memimport_new(p->mempool, ...);
   }

2. As you know there's only one pstream per client connection.
   Thus two clients, one memfd and one posix SHM will lead to
   _two_ pstreams, which from (1) will lead to two different
   memimports

3. The mempool attached to the memimport is actually for cosmetic
   purposes only! A memblock cannot stand naked by its own and
   it must be attached to a parent mempool -- thus memfd or posix
   SHM _imported_ blocks get attached to the mempool attached to
   the memimport.. and that's the only use case for that mempool!

   This is evidenced by the fact that the only place a memimport
   pool is used is when importing a block in pa_memimport_get():

   pa_memblock* pa_memimport_get(pa_memimport *i, pa_mem_type_t
                 type, uint32_t block_id, uint32_t shm_id, ...) {
       ...
       b = pa_xnew(pa_memblock, 1);
       PA_REFCNT_INIT(b);
       b->pool = i->pool;
       b->type = PA_MEMBLOCK_IMPORTED;       
       ...
   }

   So I'm not seeing anything significant here. On the client
   side, the memimport's mempool is taken from the pstream which
   is the context audio data mempool.

   And on the server side the memimport's mempool is also taken
   from the pstream which is the pa_core's global mempool (yuk!)

4. The code above is actually for the PA endpoint creating a
   memfd mempool and registering it with the other side. That
   is, pa_mempool_take_memfd_fd() is just used when registering
   a memfd mempool over the pstream.

   It's basically used to extract the fd from the pool, send a
   SHM_ID/memfd mapping to the other end, register the same
   mapping with our own pstream for inbound communication, then
   close the fd..

   The abgove usage scenario is covered is in patch #9, function
   pa_pstream_register_memfd_mempool(pstream, pool, fail_reason)

5. Using LD_LIBRARY_PATH tricks, I've tested the situation of
   multiple PA 9.0 memfd clients + multiple PA 8.0 posix SHM
   clients before the submission of v2.

   I've given this situation another round of testing now and
   things are really working as expected. If the situation quoted
   above was valid, things would've just blown up


6. Well, that's it :-)

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

Hmmm, n_blocks is used across the code as a segment refcount.
So after printing the error what would you pick from below.. ;-)

1. Continue and detach the segment memory as usual.. thus risking
   a free() on memory that's currently in use

2. Abort and don't do any detachment.. thus having dangling
   segments and memory leaks

If there is no third option beside the above two, then it seems
like an assert() is the only way to go. right?

...
> >@@ -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);
>     }
>

Before submitting v2, I've checked the implementation to make
sure that the PA_HASHMAP_FOREACH looping mechanism is safe for
node deletion.

It's just like the kernel's list_for_each_safe() iterator...
PA_HASHMAP_FOREACH uses pa_hashmap_iterate() which has this
comment on top:

  /* May be used to iterate through the hashmap. Initially the
     opaque pointer *state has to be set to NULL. The hashmap may
     not be modified during iteration -- except for deleting the
     current entry via pa_hashmap_remove()... */
  void *pa_hashmap_iterate(pa_hashmap h, void **state, void **key);

and segment_detach() actually uses pa_hashmap_remove() to remove
the hash node itself as usual.

==

Second part is transforming the if condition to an assert. IMHO
this is not needed since an already-existing assert() after the
loop covers the same ground:

  PA_HASHMAP_FOREACH(seg, i->segments, state) {
      if (segment_is_permanent(seg))
          segment_detach(seg);
  }
  pa_assert(pa_hashmap_size(i->segments) == 0);

> >      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() ?
>

That's better. Now used too.

Thanks for your very interesting reviews ;-)

-- 
Darwish
http://darwish.chasingpointers.com