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

[Ahmed S. Darwish]

On Fri, Feb 19, 2016 at 03:54:06PM +0100, David Henningsson wrote:
> 
> On 2016-02-16 22:41, Ahmed S. Darwish wrote:
> >Hi :-)
> >
...
> >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.
> 
> Ah, so that's only used on the sending side. Maybe I was just
> confused when I wrote the above.
> 
> But then I have a follow-up question. In shm_attach, which is on the
> receiving side, why do we need to save the fd in pa_shm at all? I
> mean, we don't need to send it to anyone else (right?), so all we
> need is the memory and mmap keeps that open for us. So, we could
> just close the memfd in shm_attach?
>

Yup, this is what exactly happens. No fd is ever cached on the
receiving side at shm_attch(). Similar point was raised in patch #5
review and a more detailed reply was posted here [*] -- in the 2nd
reply hunk.

Given that this issue is now raised twice, I'm sure the code is
badly unclear, possibly due to the per-type stuff now removed.

Anyway here's how shm_attch() now looks like in v3:

  static int shm_attach(pa_shm *m, pa_mem_type_t type, unsigned
                        id, int memfd_fd, ..) {
      ..
      /* In case of attaching to memfd areas, _the caller_
       * maintains ownership of the passed fd and has the sole
       * responsibility of closing it down.. For other types, we
       * are the code path  which created the fd in the first
       * place and we're thus the ones responsible for closing it
       * down */
      if (type != PA_MEM_TYPE_SHARED_MEMFD)
          pa_assert_se(pa_close(fd) == 0);

      m->type = type;
      m->id = id;
      m->size = (size_t) st.st_size;
      m->do_unlink = false;
      m->fd = -1;
  }

I guess this is now much more clear :-)

[*] http://news.gmane.org/find-root.php?message_id=<20160213015853.GB23069@darwish-pc>

...
> >
> >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?
> 
> Again, not sure. We can perhaps keep it that way. It was just a gut feeling
> over the fact that you added something for all segments, not just memfds.
>

Yup, I agree that adding an assert() which did not earlier exist
looks suspicous at best.

But the assert should've been there anyway. And the reason for
adding it right now in the the memfd patchset is because the same
patch sereis played a bit with the segments refcounting logic.

...
> >>>@@ -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:
> 
> Ok, I withdraw my comment then, although I think my version
> looks just as good as yours :-)
> 
> I think we commonly use pa_hashmap_steal_first for emptying
> hashmaps, that's why I'm more used to seeing that than your
> version of it.
>

I agree, and I blame `segment_detach(seg)' for that: it removes
itself from a hashmap that it does not actually own..

Thanks!

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