[pulseaudio-discuss] [PATCH v2 06/12] pulsecore: memexport/memimport: Support memfd blocks
David Henningsson
david.henningsson at canonical.com
Fri Feb 19 14:54:06 UTC 2016
On 2016-02-16 22:41, Ahmed S. Darwish wrote:
> 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.
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?
>
> 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?
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.
>
> ...
>>> @@ -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.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
More information about the pulseaudio-discuss
mailing list