[pulseaudio-discuss] [PATCH v2 04/12] pulsecore: Split pa_shm mempool backend into pa_shm and pa_privatemem

[Ahmed S. Darwish]

Hi :-)

On Fri, Feb 12, 2016 at 03:25:19PM +0100, David Henningsson wrote:
> 
> On 2016-02-12 01:12, Ahmed S. Darwish wrote:
stating> >Mempools use "pa_shm" for their pools memory allocation. pa_shm is
> >then overloaded with two unrelated tasks: either allocating the pool
> >using posix SHM or allocating the same pool using private malloc()s.
> >The choice depends on system configuration and abilities.
> >
> >This will not scale when a third pool memory allocation mechanism
> >is added (memfds) . Thus, split pa_shm responsibility into:
> >
> >   - pa_shm: responsible for all *shared* memory, posix and memfds
> >   - pa_privatemem: only responsible for private memory allocations
> >
> >This way, memfd support can easily be added later.
> 
> So, this part I'm still quite hesitant about. It would be easier just to
> change pa_shm to look like this, which is similar to I suggested earlier
> [1]:
> 
> struct pa_shm {
>   void *ptr;
>   size_t size;
>   pa_mem_type_t type;
>   /* The three fields below are unused if type is PA_MEM_TYPE_PRIVATE */
>   int fd;
>   unsigned id;
>   bool do_unlink; /* Used for PA_MEM_TYPE_SHARED_POSIX only */
> }
>
> Instead you did a massive refactoring, add unions inside other unions, and a
> privatemem type which contains just one other field, and even has its own
> files? Sorry, but I don't see the point in doing all of that. (Maybe it
> would make sense in an OOP language, but I don't think it does in C.)
>

OK, it's clear you don't like this. Let's tackle the critique
above in two parts.

First, regarding the 'per_type' construct...

Here's what is done in this memfd patch series:

  struct pa_shm {
      ..
      union {
          struct {
              bool do_unlink;
          } posix_shm;
          struct {
              int fd;
          } memfd;
      } per_type;
  };

And here are some other core PA structures which does the same:

  struct pa_memblock {
      ..
      union {
          struct {
              pa_free_cb_t free_cb;
              void *free_cb_data;
          } user;
          struct {
              uint32_t id;
              pa_memimport_segment *segment;
          } imported;
      } per_type;
  };

  struct pa_packet {
      ..
      union {
          uint8_t appended[MAX_APPENDED_SIZE];
      } per_type;
  };

  struct pa_tagstruct {
      ..
      union {
          uint8_t appended[MAX_APPENDED_SIZE];
      } per_type;
  };

In our case it gives _a clear warning_ that we are accessing
parts only related to posix SHM or memfds.

Tastes differ, but a 'per_type' is more verbose and clear than
putting /*This is only for memfds*/, /*This is only for posix*/
comments over different shm elements. That is even way more true
on the caller side of the equation where such markings won't be
visible.

==

Second part of your critique are the refactorings...

The reason a refactoring was initiated is that shm.c was just
too complex: I could't add _any_ extra code to it anymore.

Giving it a second look though, I admit that _the way_ the
refactoring was done is an overkill. It added a pa_mem parent,
then made pa_shm and pa_privatemem inherit from it, then added a
per_type on the mempool to distinguish whether it's using shm or
privatemem.. this made things worse.

So I'll refactor this in v3 in a much more _graceful_ manner:
just move the private memory allocs and free to their own static
methods in shm.c. This way, the memfd additions can be added
without introducing too much disturbances to the system.

==

Given the above, hopefully a common ground is now reached...
to v3 ;-)

Thanks,

Darwish
http://darwish.chasingpointers.com