Looks good mostly, just a few nitpicks.
On 2016-02-12 01:10, Ahmed S. Darwish wrote:
> Soon we're going to have three types of memory pools: POSIX shm_open()
> pools, memfd memfd_create() ones, and privately malloc()-ed pools.
>
> Due to such diversity of mempool types, transform pa_mempool_new()
> into a factory method that returns the required type of memory pool
> according to given parameters.
>
> Signed-off-by: Ahmed S. Darwish <darwish.07 at gmail.com>
> ---
> src/Makefile.am | 1 +
> src/pulse/context.c | 10 ++++++---
> src/pulsecore/core.c | 4 ++--
> src/pulsecore/mem.h | 50 +++++++++++++++++++++++++++++++++++++++++
> src/pulsecore/memblock.c | 19 +++++++++++++---
> src/pulsecore/memblock.h | 3 ++-
> src/pulsecore/protocol-native.c | 2 +-
> src/tests/cpu-mix-test.c | 2 +-
> src/tests/lfe-filter-test.c | 2 +-
> src/tests/mcalign-test.c | 2 +-
> src/tests/memblock-test.c | 6 ++---
> src/tests/memblockq-test.c | 2 +-
> src/tests/mix-test.c | 2 +-
> src/tests/remix-test.c | 2 +-
> src/tests/resampler-test.c | 2 +-
> src/tests/srbchannel-test.c | 2 +-
> 16 files changed, 90 insertions(+), 21 deletions(-)
> create mode 100644 src/pulsecore/mem.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index b0ca2bc..6cb5bd7 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -699,6 +699,7 @@ libpulsecommon_ at PA_MAJORMINOR@_la_SOURCES = \
> pulsecore/refcnt.h \
> pulsecore/srbchannel.c pulsecore/srbchannel.h \
> pulsecore/sample-util.c pulsecore/sample-util.h \
> + pulsecore/mem.h \
> pulsecore/shm.c pulsecore/shm.h \
> pulsecore/bitset.c pulsecore/bitset.h \
> pulsecore/socket-client.c pulsecore/socket-client.h \
> diff --git a/src/pulse/context.c b/src/pulse/context.c
> index 4f084e8..39e1ae8 100644
> --- a/src/pulse/context.c
> +++ b/src/pulse/context.c
> @@ -125,6 +125,7 @@ static void reset_callbacks(pa_context *c) {
>
> pa_context *pa_context_new_with_proplist(pa_mainloop_api *mainloop, const char *name, pa_proplist *p) {
> pa_context *c;
> + pa_mem_type_t type;
>
> pa_assert(mainloop);
>
> @@ -170,10 +171,13 @@ pa_context *pa_context_new_with_proplist(pa_mainloop_api *mainloop, const char *
> c->srb_template.readfd = -1;
> c->srb_template.writefd = -1;
>
> - if (!(c->mempool = pa_mempool_new(!c->conf->disable_shm, c->conf->shm_size))) {
> + type = !c->conf->disable_shm ? PA_MEM_TYPE_SHARED_POSIX : PA_MEM_TYPE_PRIVATE;
> + if (!(c->mempool = pa_mempool_new(type, c->conf->shm_size))) {
>
> - if (!c->conf->disable_shm)
> - c->mempool = pa_mempool_new(false, c->conf->shm_size);
> + if (!c->conf->disable_shm) {
> + pa_log_warn("Failed to allocate shared memory pool. Falling back to a normal private one.");
> + c->mempool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, c->conf->shm_size);
> + }
>
> if (!c->mempool) {
> context_free(c);
> diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
> index 1c3c3cd..4714932 100644
> --- a/src/pulsecore/core.c
> +++ b/src/pulsecore/core.c
> @@ -69,14 +69,14 @@ pa_core* pa_core_new(pa_mainloop_api *m, bool shared, size_t shm_size) {
> pa_assert(m);
>
> if (shared) {
> - if (!(pool = pa_mempool_new(shared, shm_size))) {
> + if (!(pool = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, shm_size))) {
> pa_log_warn("Failed to allocate shared memory pool. Falling back to a normal memory pool.");
> shared = false;
> }
> }
>
> if (!shared) {
> - if (!(pool = pa_mempool_new(shared, shm_size))) {
> + if (!(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, shm_size))) {
> pa_log("pa_mempool_new() failed.");
> return NULL;
> }
> diff --git a/src/pulsecore/mem.h b/src/pulsecore/mem.h
> new file mode 100644
> index 0000000..b6fdba6
> --- /dev/null
> +++ b/src/pulsecore/mem.h
> @@ -0,0 +1,50 @@
> +#ifndef foopulsememhfoo
> +#define foopulsememhfoo
> +
> +/***
> + This file is part of PulseAudio.
> +
> + Copyright 2016 Ahmed S. Darwish <darwish.07 at gmail.com>
> +
> + PulseAudio is free software; you can redistribute it and/or modify
> + it under the terms of the GNU Lesser General Public License as
> + published by the Free Software Foundation; either version 2.1 of the
> + License, or (at your option) any later version.
> +
> + PulseAudio is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +#include <stdbool.h>
> +
> +#include <pulsecore/macro.h>
> +
> +typedef enum pa_mem_type {
> + PA_MEM_TYPE_SHARED_POSIX, /* Data is shared and created using POSIX shm_open() */
> + PA_MEM_TYPE_SHARED_MEMFD, /* Data is shared and created using Linux memfd_create() */
> + PA_MEM_TYPE_PRIVATE, /* Data is private and created using classic memory allocation (malloc, etc.) */
Actually, it's created using either mmap, posix_memallign, or malloc.
> +} pa_mem_type_t;
> +
> +static inline bool pa_mem_type_is_shared(pa_mem_type_t t) {
> + return (t == PA_MEM_TYPE_SHARED_POSIX) || (t == PA_MEM_TYPE_SHARED_MEMFD);
> +}
> +
Is the reason for having this as an inline function just to avoid a mem.c ?
> +static inline const char *pa_mem_type_to_string(pa_mem_type_t type) {
> + switch (type) {
> + case PA_MEM_TYPE_SHARED_POSIX:
> + return "shared posix-shm";
> + case PA_MEM_TYPE_SHARED_MEMFD:
> + return "shared memfd";
> + case PA_MEM_TYPE_PRIVATE:
> + return "private";
> + }
> +
> + pa_assert_not_reached();
> +}
> +
> +#endif
> diff --git a/src/pulsecore/memblock.c b/src/pulsecore/memblock.c
> index 9b6810d..3a33a6f 100644
> --- a/src/pulsecore/memblock.c
> +++ b/src/pulsecore/memblock.c
> @@ -145,7 +145,9 @@ struct pa_mempool {
> pa_semaphore *semaphore;
> pa_mutex *mutex;
>
> + pa_mem_type_t type;
> pa_shm memory;
> +
> size_t block_size;
> unsigned n_blocks;
> bool is_remote_writable;
> @@ -744,11 +746,13 @@ static void memblock_replace_import(pa_memblock *b) {
> pa_mutex_unlock(import->mutex);
> }
>
> -pa_mempool* pa_mempool_new(bool shared, size_t size) {
> +pa_mempool *pa_mempool_new(pa_mem_type_t type, size_t size) {
> pa_mempool *p;
> + bool shared;
> char t1[PA_BYTES_SNPRINT_MAX], t2[PA_BYTES_SNPRINT_MAX];
>
> p = pa_xnew0(pa_mempool, 1);
> + p->type = type;
>
> p->block_size = PA_PAGE_ALIGN(PA_MEMPOOL_SLOT_SIZE);
> if (p->block_size < PA_PAGE_SIZE)
> @@ -763,13 +767,14 @@ pa_mempool* pa_mempool_new(bool shared, size_t size) {
> p->n_blocks = 2;
> }
>
> + shared = pa_mem_type_is_shared(type);
This will be changed in latter patches, looks a bit weird right now.
> if (pa_shm_create_rw(&p->memory, p->n_blocks * p->block_size, shared, 0700) < 0) {
> pa_xfree(p);
> return NULL;
> }
>
> pa_log_debug("Using %s memory pool with %u slots of size %s each, total size is %s, maximum usable slot size is %lu",
> - p->memory.shared ? "shared" : "private",
> + pa_mem_type_to_string(type),
> p->n_blocks,
> pa_bytes_snprint(t1, sizeof(t1), (unsigned) p->block_size),
> pa_bytes_snprint(t2, sizeof(t2), (unsigned) (p->n_blocks * p->block_size)),
> @@ -847,7 +852,15 @@ void pa_mempool_free(pa_mempool *p) {
> /* PA_DEBUG_TRAP; */
> }
>
> - pa_shm_free(&p->memory);
> + switch (p->type) {
> + case PA_MEM_TYPE_SHARED_POSIX:
> + case PA_MEM_TYPE_PRIVATE:
> + pa_shm_free(&p->memory);
> + break;
> + case PA_MEM_TYPE_SHARED_MEMFD:
> + pa_assert_not_reached();
> + break;
> + }
>
> pa_mutex_free(p->mutex);
> pa_semaphore_free(p->semaphore);
> diff --git a/src/pulsecore/memblock.h b/src/pulsecore/memblock.h
> index 4faef75..a760b6f 100644
> --- a/src/pulsecore/memblock.h
> +++ b/src/pulsecore/memblock.h
> @@ -30,6 +30,7 @@ typedef struct pa_memblock pa_memblock;
> #include <pulse/xmalloc.h>
> #include <pulsecore/atomic.h>
> #include <pulsecore/memchunk.h>
> +#include <pulsecore/mem.h>
>
> /* A pa_memblock is a reference counted memory block. PulseAudio
> * passes references to pa_memblocks around instead of copying
> @@ -121,7 +122,7 @@ pa_mempool * pa_memblock_get_pool(pa_memblock *b);
> pa_memblock *pa_memblock_will_need(pa_memblock *b);
>
> /* The memory block manager */
> -pa_mempool* pa_mempool_new(bool shared, size_t size);
> +pa_mempool *pa_mempool_new(pa_mem_type_t type, size_t size);
> void pa_mempool_free(pa_mempool *p);
> const pa_mempool_stat* pa_mempool_get_stat(pa_mempool *p);
> void pa_mempool_vacuum(pa_mempool *p);
> diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c
> index c6b3ca4..58f99c1 100644
> --- a/src/pulsecore/protocol-native.c
> +++ b/src/pulsecore/protocol-native.c
> @@ -2618,7 +2618,7 @@ static void setup_srbchannel(pa_native_connection *c) {
> return;
> }
>
> - if (!(c->rw_mempool = pa_mempool_new(true, c->protocol->core->shm_size))) {
> + if (!(c->rw_mempool = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, c->protocol->core->shm_size))) {
> pa_log_warn("Disabling srbchannel, reason: Failed to allocate shared "
> "writable memory pool.");
> return;
> diff --git a/src/tests/cpu-mix-test.c b/src/tests/cpu-mix-test.c
> index f3bc0cc..9726685 100644
> --- a/src/tests/cpu-mix-test.c
> +++ b/src/tests/cpu-mix-test.c
> @@ -76,7 +76,7 @@ static void run_mix_test(
> samples_ref = out_ref + (8 - align);
> nsamples = channels * (SAMPLES - (8 - align));
>
> - fail_unless((pool = pa_mempool_new(false, 0)) != NULL, NULL);
> + fail_unless((pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0)) != NULL, NULL);
>
> pa_random(samples0, nsamples * sizeof(int16_t));
> c0.memblock = pa_memblock_new_fixed(pool, samples0, nsamples * sizeof(int16_t), false);
> diff --git a/src/tests/lfe-filter-test.c b/src/tests/lfe-filter-test.c
> index 389a2b9..5b81e70 100644
> --- a/src/tests/lfe-filter-test.c
> +++ b/src/tests/lfe-filter-test.c
> @@ -136,7 +136,7 @@ START_TEST (lfe_filter_test) {
> a.format = PA_SAMPLE_S16NE;
>
> lft.ss = &a;
> - pa_assert_se(lft.pool = pa_mempool_new(false, 0));
> + pa_assert_se(lft.pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0));
>
> /* We prepare pseudo-random input audio samples for lfe-filter rewind testing*/
> ori_sample_ptr = pa_xmalloc(pa_frame_size(lft.ss) * TOTAL_SAMPLES);
> diff --git a/src/tests/mcalign-test.c b/src/tests/mcalign-test.c
> index 0d27dfd..57ac01c 100644
> --- a/src/tests/mcalign-test.c
> +++ b/src/tests/mcalign-test.c
> @@ -36,7 +36,7 @@ int main(int argc, char *argv[]) {
> pa_mcalign *a;
> pa_memchunk c;
>
> - p = pa_mempool_new(false, 0);
> + p = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0);
>
> a = pa_mcalign_new(11);
>
> diff --git a/src/tests/memblock-test.c b/src/tests/memblock-test.c
> index 2b51108..58eae7b 100644
> --- a/src/tests/memblock-test.c
> +++ b/src/tests/memblock-test.c
> @@ -80,11 +80,11 @@ START_TEST (memblock_test) {
>
> const char txt[] = "This is a test!";
>
> - pool_a = pa_mempool_new(true, 0);
> + pool_a = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
> fail_unless(pool_a != NULL);
> - pool_b = pa_mempool_new(true, 0);
> + pool_b = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
> fail_unless(pool_b != NULL);
> - pool_c = pa_mempool_new(true, 0);
> + pool_c = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
> fail_unless(pool_c != NULL);
>
> pa_mempool_get_shm_id(pool_a, &id_a);
> diff --git a/src/tests/memblockq-test.c b/src/tests/memblockq-test.c
> index eea6cfa..f9464db 100644
> --- a/src/tests/memblockq-test.c
> +++ b/src/tests/memblockq-test.c
> @@ -108,7 +108,7 @@ START_TEST (memblockq_test) {
>
> pa_log_set_level(PA_LOG_DEBUG);
>
> - p = pa_mempool_new(false, 0);
> + p = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0);
>
> silence.memblock = pa_memblock_new_fixed(p, (char*) "__", 2, 1);
> fail_unless(silence.memblock != NULL);
> diff --git a/src/tests/mix-test.c b/src/tests/mix-test.c
> index c8af600..117ad34 100644
> --- a/src/tests/mix-test.c
> +++ b/src/tests/mix-test.c
> @@ -286,7 +286,7 @@ START_TEST (mix_test) {
> if (!getenv("MAKE_CHECK"))
> pa_log_set_level(PA_LOG_DEBUG);
>
> - fail_unless((pool = pa_mempool_new(false, 0)) != NULL, NULL);
> + fail_unless((pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0)) != NULL, NULL);
>
> a.channels = 1;
> a.rate = 44100;
> diff --git a/src/tests/remix-test.c b/src/tests/remix-test.c
> index 6feb8dc..21e5f48 100644
> --- a/src/tests/remix-test.c
> +++ b/src/tests/remix-test.c
> @@ -51,7 +51,7 @@ int main(int argc, char *argv[]) {
>
> pa_log_set_level(PA_LOG_DEBUG);
>
> - pa_assert_se(pool = pa_mempool_new(false, 0));
> + pa_assert_se(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0));
>
> for (i = 0; maps[i].channels > 0; i++)
> for (j = 0; maps[j].channels > 0; j++) {
> diff --git a/src/tests/resampler-test.c b/src/tests/resampler-test.c
> index 9832a31..4796353 100644
> --- a/src/tests/resampler-test.c
> +++ b/src/tests/resampler-test.c
> @@ -404,7 +404,7 @@ int main(int argc, char *argv[]) {
> }
>
> ret = 0;
> - pa_assert_se(pool = pa_mempool_new(false, 0));
> + pa_assert_se(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0));
>
> if (!all_formats) {
>
> diff --git a/src/tests/srbchannel-test.c b/src/tests/srbchannel-test.c
> index cd4d397..59ce1ed 100644
> --- a/src/tests/srbchannel-test.c
> +++ b/src/tests/srbchannel-test.c
> @@ -85,7 +85,7 @@ START_TEST (srbchannel_test) {
> int pipefd[4];
>
> pa_mainloop *ml = pa_mainloop_new();
> - pa_mempool *mp = pa_mempool_new(true, 0);
> + pa_mempool *mp = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
> pa_iochannel *io1, *io2;
> pa_pstream *p1, *p2;
> pa_srbchannel *sr1, *sr2;
>
> Regards,
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
