[pulseaudio-discuss] [PATCH 00/11] Introduce memfd support

[Ahmed S. Darwish]

Hi!

On Mon, Sep 28, 2015 at 10:47:09AM +0200, David Henningsson wrote:
> 1)
> 
> There was some discussion about how the pa_mem struct should look like. I
> think it should just look like this:
> 
> struct pa_mem {
>   void *ptr;
>   size_t size;
>   pa_mem_type_t type;
>   int fd;
>   unsigned id; /* both shm and memfd need an id, see below */
>   bool shm_do_unlink;
> }
> 
> Memfd and shm can then share some methods because they're both fd backed (e
> g, closing is to call munmap and then fclose for both).
> 
> Plus, it's all much simpler than anonymous unions and structs, IMO.
>

Indeed. After finishing doing so now in v2, a lot of code duplication
between posix SHM and memfds has been removed and the patch series is
now much smaller. Thanks a lot.

> 
> 2)
> 
> The second big thing that makes me confused is how the memfd is initially
> set up and sent over the pipe to the client. Am I reading your code wrong,
> or do you actually send an extra memfd packet with the fd in *for every
> packet* sent?
>

You're reading the code right. Definitely a problem to be fixed.

> The per-client memfd should be set up by the server at SHM negotiation time
> (and sealed, but you've said that's a future patch set). Then send a packet
> with the memfd as ancil data (or extend PA_COMMAND_ENABLE_SRBCHANNEL, if
> that's simpler). This packet should also have an ID that identifies the
> memfd.

I'm having a problem in this part. By doing as said above, aren't we
limiting the pstream connection to send memblocks only from a _single_
memfd-backed pool?

Imagine the following:

// PA node 1
pa_pstream_send_memblock(p, memblock1);    // memblock1 is inside pool1
pa_pstream_send_memblock(p, memblock2);    // memblock2 is inside pool2

If pool1 and pool2 are backed by different memfd regions, how would the
above suggestion handle that case?

> Every memfd and shm now have a unique ID, so you can just put that ID when
> you do the memexport, so on the sender side you don't need to distinguish
> between the two types.
> And on the memimport side you'd look this ID up, and see if it matches a
> previously received memfd, if not, it's a posix-shm ID that you need to call
> pa_shm_attach on.
>
> Does that make sense to you?
>

Ah, definitely agree. This is much simpler.

If my claim above is valid though, I might just implement in a slightly
different way:

``Instead of sending the memfd file descriptor at SHM negotiation
  time, do it in pstream.c::prepare_next_write_item().

  To avoid the problem of sending the memfd fd for every packet sent,
  track the memfd-backed blocks earlier sent using their randomly
  generated IDs.

  If this is the first time we encounter this ID, send the memfd file
  descriptor as ancil data with the packet. If this is not the first
  encouter, just send the ID without any ancil data. The other end
  is already tracking this ID and have a memfd socket opened for it
  from earlier communication''

What do you think?

Thanks again, and happy holidays :)

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