[PATCH RFCv3 10/51] pstream: Unionize item_info

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> On 2014-11-05 00:26, Peter Meerwald wrote:
> > From: Peter Meerwald <p.meerwald at bct-electronic.com>
> >
> > item_info has per-type fields which should be within a union to
> > save space
> 
> I think this is mostly a bikeshed patch, because the memory saved can't 
> be significant, can it?

you are right, the patch's comment should not mention memory savings...
 
> If the memory saved is insignificant I'd say that just longer code 
> (added "per_type.memblock_info" here and there) is a slight drawback and 
> hence this patch could be skipped. But it's not a strong opinion.

probably the better argument is to codify which attributes are used by 
which packet type as is done in several other places in PA

> > Signed-off-by: Peter Meerwald <pmeerw at pmeerw.net>
> > ---
> >   src/pulsecore/pstream.c | 117
> +++++++++++++++++++++++++++---------------------
> >   1 file changed, 65 insertions(+), 52 deletions(-)
> >
> > diff --git a/src/pulsecore/pstream.c b/src/pulsecore/pstream.c
> > index 96ee247..f8217b3 100644
> > --- a/src/pulsecore/pstream.c
> > +++ b/src/pulsecore/pstream.c
> > @@ -92,21 +92,26 @@ struct item_info {
> >           PA_PSTREAM_ITEM_SHMREVOKE
> >       } type;
> >
> > -    /* packet info */
> > -    pa_packet *packet;
> > +    union {
> > +        /* packet info */
> > +        pa_packet *packet;
> > +
> > +        /* release/revoke info */
> > +        uint32_t block_id;
> > +
> > +        /* memblock info */
> > +        struct {
> > +            pa_memchunk chunk;
> > +            uint32_t channel;
> > +            int64_t offset;
> > +            pa_seek_mode_t seek_mode;
> > +        } memblock_info;
> > +    } per_type;
> > +
> >   #ifdef HAVE_CREDS
> >       bool with_ancil_data;
> >       pa_cmsg_ancil_data ancil_data;
> >   #endif
> > -
> > -    /* memblock info */
> > -    pa_memchunk chunk;
> > -    uint32_t channel;
> > -    int64_t offset;
> > -    pa_seek_mode_t seek_mode;
> > -
> > -    /* release/revoke info */
> > -    uint32_t block_id;
> >   };
> >
> >   struct pstream_read {
> > @@ -285,11 +290,11 @@ static void item_free(void *item) {
> >       pa_assert(i);
> >
> >       if (i->type == PA_PSTREAM_ITEM_MEMBLOCK) {
> > -        pa_assert(i->chunk.memblock);
> > -        pa_memblock_unref(i->chunk.memblock);
> > +        pa_assert(i->per_type.memblock_info.chunk.memblock);
> > +        pa_memblock_unref(i->per_type.memblock_info.chunk.memblock);
> >       } else if (i->type == PA_PSTREAM_ITEM_PACKET) {
> > -        pa_assert(i->packet);
> > -        pa_packet_unref(i->packet);
> > +        pa_assert(i->per_type.packet);
> > +        pa_packet_unref(i->per_type.packet);
> >       }
> >
> >       if (pa_flist_push(PA_STATIC_FLIST_GET(items), i) < 0)
> > @@ -324,25 +329,19 @@ static void pstream_free(pa_pstream *p) {
> >       pa_xfree(p);
> >   }
> >
> > -void pa_pstream_send_packet(pa_pstream*p, pa_packet *packet, const
> pa_cmsg_ancil_data *ancil_data) {
> > -    struct item_info *i;
> > -
> > +static void pa_pstream_send_item(pa_pstream*p, struct item_info *item,
> const pa_cmsg_ancil_data *ancil_data) {
> 
> It looks like pa_pstream_send_item is only used for 
> pa_pstream_send_packet and not for pa_pstream_send_memblock (etc), how come?
> 
> >       pa_assert(p);
> >       pa_assert(PA_REFCNT_VALUE(p) > 0);
> > -    pa_assert(packet);
> > +    pa_assert(item);
> >
> > -    if (p->dead)
> > +    if (p->dead) {
> > +        item_free(item);
> >           return;
> > -
> > -    if (!(i = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> > -        i = pa_xnew(struct item_info, 1);
> > -
> > -    i->type = PA_PSTREAM_ITEM_PACKET;
> > -    i->packet = pa_packet_ref(packet);
> > +    }
> >
> >   #ifdef HAVE_CREDS
> > -    if ((i->with_ancil_data = !!ancil_data)) {
> > -        i->ancil_data = *ancil_data;
> > +    if ((item->with_ancil_data = !!ancil_data)) {
> > +        item->ancil_data = *ancil_data;
> >           if (ancil_data->creds_valid)
> >               pa_assert(ancil_data->nfd == 0);
> >           else
> > @@ -350,11 +349,25 @@ void pa_pstream_send_packet(pa_pstream*p, pa_packet
> *packet, const pa_cmsg_ancil
> >       }
> >   #endif
> >
> > -    pa_queue_push(p->send_queue, i);
> > +    pa_queue_push(p->send_queue, item);
> >
> >       p->mainloop->defer_enable(p->defer_event, 1);
> >   }
> >
> > +void pa_pstream_send_packet(pa_pstream*p, pa_packet *packet, const
> pa_cmsg_ancil_data *ancil_data) {
> > +    struct item_info *i;
> > +
> > +    pa_assert(packet);
> > +
> > +    if (!(i = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> > +        i = pa_xnew(struct item_info, 1);
> > +
> > +    i->type = PA_PSTREAM_ITEM_PACKET;
> > +    i->per_type.packet = pa_packet_ref(packet);
> > +
> > +    pa_pstream_send_item(p, i, ancil_data);
> > +}
> > +
> >   void pa_pstream_send_memblock(pa_pstream*p, uint32_t channel, int64_t
> offset, pa_seek_mode_t seek_mode, const pa_memchunk *chunk) {
> >       size_t length, idx;
> >       size_t bsm;
> > @@ -381,13 +394,13 @@ void pa_pstream_send_memblock(pa_pstream*p, uint32_t
> channel, int64_t offset, pa
> >           i->type = PA_PSTREAM_ITEM_MEMBLOCK;
> >
> >           n = PA_MIN(length, bsm);
> > -        i->chunk.index = chunk->index + idx;
> > -        i->chunk.length = n;
> > -        i->chunk.memblock = pa_memblock_ref(chunk->memblock);
> > +        i->per_type.memblock_info.chunk.index = chunk->index + idx;
> > +        i->per_type.memblock_info.chunk.length = n;
> > +        i->per_type.memblock_info.chunk.memblock =
> pa_memblock_ref(chunk->memblock);
> >
> > -        i->channel = channel;
> > -        i->offset = offset;
> > -        i->seek_mode = seek_mode;
> > +        i->per_type.memblock_info.channel = channel;
> > +        i->per_type.memblock_info.offset = offset;
> > +        i->per_type.memblock_info.seek_mode = seek_mode;
> >   #ifdef HAVE_CREDS
> >           i->with_ancil_data = false;
> >   #endif
> > @@ -414,7 +427,7 @@ void pa_pstream_send_release(pa_pstream *p, uint32_t
> block_id) {
> >       if (!(item = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> >           item = pa_xnew(struct item_info, 1);
> >       item->type = PA_PSTREAM_ITEM_SHMRELEASE;
> > -    item->block_id = block_id;
> > +    item->per_type.block_id = block_id;
> >   #ifdef HAVE_CREDS
> >       item->with_ancil_data = false;
> >   #endif
> > @@ -451,7 +464,7 @@ void pa_pstream_send_revoke(pa_pstream *p, uint32_t
> block_id) {
> >       if (!(item = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> >           item = pa_xnew(struct item_info, 1);
> >       item->type = PA_PSTREAM_ITEM_SHMREVOKE;
> > -    item->block_id = block_id;
> > +    item->per_type.block_id = block_id;
> >   #ifdef HAVE_CREDS
> >       item->with_ancil_data = false;
> >   #endif
> > @@ -495,9 +508,9 @@ static void prepare_next_write_item(pa_pstream *p) {
> >       if (p->write.current->type == PA_PSTREAM_ITEM_PACKET) {
> >           size_t plen;
> >
> > -        pa_assert(p->write.current->packet);
> > +        pa_assert(p->write.current->per_type.packet);
> >
> > -        p->write.data = (void *) pa_packet_data(p->write.current->packet,
> &plen);
> > +        p->write.data = (void *)
> pa_packet_data(p->write.current->per_type.packet, &plen);
> >           p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] =
> htonl((uint32_t) plen);
> >
> >           if (plen <= MINIBUF_SIZE - PA_PSTREAM_DESCRIPTOR_SIZE) {
> > @@ -508,32 +521,32 @@ static void prepare_next_write_item(pa_pstream *p) {
> >       } else if (p->write.current->type == PA_PSTREAM_ITEM_SHMRELEASE) {
> >
> >           p->write.descriptor[PA_PSTREAM_DESCRIPTOR_FLAGS] =
> htonl(PA_FLAG_SHMRELEASE);
> > -        p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl(p->write.current->block_id);
> > +        p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl(p->write.current->per_type.block_id);
> >
> >       } else if (p->write.current->type == PA_PSTREAM_ITEM_SHMREVOKE) {
> >
> >           p->write.descriptor[PA_PSTREAM_DESCRIPTOR_FLAGS] =
> htonl(PA_FLAG_SHMREVOKE);
> > -        p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl(p->write.current->block_id);
> > +        p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl(p->write.current->per_type.block_id);
> >
> >       } else {
> >           uint32_t flags;
> >           bool send_payload = true;
> >
> >           pa_assert(p->write.current->type == PA_PSTREAM_ITEM_MEMBLOCK);
> > -        pa_assert(p->write.current->chunk.memblock);
> > +        pa_assert(p->write.current->per_type.memblock_info.chunk.memblock);
> >
> > -        p->write.descriptor[PA_PSTREAM_DESCRIPTOR_CHANNEL] =
> htonl(p->write.current->channel);
> > -        p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl((uint32_t) (((uint64_t) p->write.current->offset) >> 32));
> > -        p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_LO] =
> htonl((uint32_t) ((uint64_t) p->write.current->offset));
> > +        p->write.descriptor[PA_PSTREAM_DESCRIPTOR_CHANNEL] =
> htonl(p->write.current->per_type.memblock_info.channel);
> > +        p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] =
> htonl((uint32_t) (((uint64_t) p->write.current->per_type.memblock_info.offset)
> >> 32));
> > +        p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_LO] =
> htonl((uint32_t) ((uint64_t)
> p->write.current->per_type.memblock_info.offset));
> >
> > -        flags = (uint32_t) (p->write.current->seek_mode &
> PA_FLAG_SEEKMASK);
> > +        flags = (uint32_t)
> (p->write.current->per_type.memblock_info.seek_mode & PA_FLAG_SEEKMASK);
> >
> >           if (p->use_shm) {
> >               uint32_t block_id, shm_id;
> >               size_t offset, length;
> >               uint32_t *shm_info = (uint32_t *)
> &p->write.minibuf[PA_PSTREAM_DESCRIPTOR_SIZE];
> >               size_t shm_size = sizeof(uint32_t) * PA_PSTREAM_SHM_MAX;
> > -            pa_mempool *current_pool =
> pa_memblock_get_pool(p->write.current->chunk.memblock);
> > +            pa_mempool *current_pool =
> pa_memblock_get_pool(p->write.current->per_type.memblock_info.chunk.memblock);
> >               pa_memexport *current_export;
> >
> >               if (p->mempool == current_pool)
> > @@ -542,7 +555,7 @@ static void prepare_next_write_item(pa_pstream *p) {
> >                   pa_assert_se(current_export =
> pa_memexport_new(current_pool, memexport_revoke_cb, p));
> >
> >               if (pa_memexport_put(current_export,
> > -                                 p->write.current->chunk.memblock,
> > +
> p->write.current->per_type.memblock_info.chunk.memblock,
> >                                    &block_id,
> >                                    &shm_id,
> >                                    &offset,
> > @@ -555,8 +568,8 @@ static void prepare_next_write_item(pa_pstream *p) {
> >
> >                   shm_info[PA_PSTREAM_SHM_BLOCKID] = htonl(block_id);
> >                   shm_info[PA_PSTREAM_SHM_SHMID] = htonl(shm_id);
> > -                shm_info[PA_PSTREAM_SHM_INDEX] = htonl((uint32_t) (offset +
> p->write.current->chunk.index));
> > -                shm_info[PA_PSTREAM_SHM_LENGTH] = htonl((uint32_t)
> p->write.current->chunk.length);
> > +                shm_info[PA_PSTREAM_SHM_INDEX] = htonl((uint32_t) (offset +
> p->write.current->per_type.memblock_info.chunk.index));
> > +                shm_info[PA_PSTREAM_SHM_LENGTH] = htonl((uint32_t)
> p->write.current->per_type.memblock_info.chunk.length);
> >
> >                   p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] =
> htonl(shm_size);
> >                   p->write.minibuf_validsize = PA_PSTREAM_DESCRIPTOR_SIZE +
> shm_size;
> > @@ -569,8 +582,8 @@ static void prepare_next_write_item(pa_pstream *p) {
> >           }
> >
> >           if (send_payload) {
> > -            p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] =
> htonl((uint32_t) p->write.current->chunk.length);
> > -            p->write.memchunk = p->write.current->chunk;
> > +            p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] =
> htonl((uint32_t) p->write.current->per_type.memblock_info.chunk.length);
> > +            p->write.memchunk =
> p->write.current->per_type.memblock_info.chunk;
> >               pa_memblock_ref(p->write.memchunk.memblock);
> >               p->write.data = NULL;
> >           }
> >
> 
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux