From: Panu Matilainen <pmatilai@redhat.com>
To: Keith Wiles <keith.wiles@intel.com>, dev@dpdk.org
Subject: Re: [PATCH v3] mempool: reduce rte_mempool structure size
Date: Fri, 12 Feb 2016 13:23:45 +0200 [thread overview]
Message-ID: <56BDC0C1.9010506@redhat.com> (raw)
In-Reply-To: <1455139134-9826-1-git-send-email-keith.wiles@intel.com>
On 02/10/2016 11:18 PM, Keith Wiles wrote:
> The rte_mempool structure is changed, which will cause an ABI change
> for this structure. Providing backward compat is not reasonable
> here as this structure is used in multiple defines/inlines.
>
> Allow mempool cache support to be dynamic depending on if the
> mempool being created needs cache support. Saves about 1.5M of
> memory used by the rte_mempool structure.
>
> Allocating small mempools which do not require cache can consume
> larges amounts of memory if you have a number of these mempools.
>
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> ---
> * Patch v3 fix up the ifdefs to correct some problems in removing ifdef
> lines. Added the ABI deprecation notice to the document file.
> * Patch v2 to add some comments and setup for RTE_NEXT_ABI changes.
>
> app/test/test_mempool.c | 5 +++
> doc/guides/rel_notes/deprecation.rst | 7 +++
> lib/librte_mempool/rte_mempool.c | 82 +++++++++++++++++++++++++++++++++---
> lib/librte_mempool/rte_mempool.h | 46 ++++++++++++++++----
> 4 files changed, 127 insertions(+), 13 deletions(-)
>
> diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
> index f0f823b..f3fba50 100644
> --- a/app/test/test_mempool.c
> +++ b/app/test/test_mempool.c
> @@ -122,8 +122,13 @@ test_mempool_basic(void)
> return -1;
>
> printf("get private data\n");
> +#ifdef RTE_NEXT_ABI
> + if (rte_mempool_get_priv(mp) != (char *)mp +
> + MEMPOOL_HEADER_SIZE(mp, mp->pg_num, mp->cache_size))
> +#else
> if (rte_mempool_get_priv(mp) !=
> (char*) mp + MEMPOOL_HEADER_SIZE(mp, mp->pg_num))
> +#endif
> return -1;
>
> printf("get physical address of an object\n");
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index e94d4a2..1b9d25e 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -49,3 +49,10 @@ Deprecation Notices
> commands (such as RETA update in testpmd). This should impact
> CMDLINE_PARSE_RESULT_BUFSIZE, STR_TOKEN_SIZE and RDLINE_BUF_SIZE.
> It should be integrated in release 2.3.
> +
> +* ABI change is planned for the rte_mempool structure to allow mempool
> + cache support to be dynamic depending on the mempool being created
> + needing cache support. Saves about 1.5M of memory per rte_mempool structure
> + by removing the per lcore cache memory. Change will occur after DPDK 16.04
> + release.
> +
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index aff5f6d..5f21eaa 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -452,12 +452,17 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
> /* compilation-time checks */
> RTE_BUILD_BUG_ON((sizeof(struct rte_mempool) &
> RTE_CACHE_LINE_MASK) != 0);
> +#ifdef RTE_NEXT_ABI
> + RTE_BUILD_BUG_ON((sizeof(struct rte_mempool_cache) &
> + RTE_CACHE_LINE_MASK) != 0);
> +#else
> #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
> RTE_BUILD_BUG_ON((sizeof(struct rte_mempool_cache) &
> RTE_CACHE_LINE_MASK) != 0);
> RTE_BUILD_BUG_ON((offsetof(struct rte_mempool, local_cache) &
> RTE_CACHE_LINE_MASK) != 0);
> #endif
> +#endif /* RTE_NEXT_ABI */
> #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> RTE_BUILD_BUG_ON((sizeof(struct rte_mempool_debug_stats) &
> RTE_CACHE_LINE_MASK) != 0);
> @@ -527,9 +532,8 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
> */
> int head = sizeof(struct rte_mempool);
> int new_size = (private_data_size + head) % page_size;
> - if (new_size) {
> + if (new_size)
> private_data_size += page_size - new_size;
> - }
> }
>
> /* try to allocate tailq entry */
> @@ -544,7 +548,12 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
> * store mempool objects. Otherwise reserve a memzone that is large
> * enough to hold mempool header and metadata plus mempool objects.
> */
> +#ifdef RTE_NEXT_ABI
> + mempool_size = MEMPOOL_HEADER_SIZE(mp, pg_num, cache_size);
> + mempool_size += private_data_size;
> +#else
> mempool_size = MEMPOOL_HEADER_SIZE(mp, pg_num) + private_data_size;
> +#endif /* RTE_NEXT_ABI */
> mempool_size = RTE_ALIGN_CEIL(mempool_size, RTE_MEMPOOL_ALIGN);
> if (vaddr == NULL)
> mempool_size += (size_t)objsz.total_size * n;
> @@ -598,9 +607,22 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
> mp->cache_flushthresh = CALC_CACHE_FLUSHTHRESH(cache_size);
> mp->private_data_size = private_data_size;
>
> +#ifdef RTE_NEXT_ABI
> + /*
> + * local_cache pointer is set even if cache_size is zero.
> + * The local_cache points to just past the elt_pa[] array.
> + */
> + mp->local_cache = (struct rte_mempool_cache *)
> + ((char *)mp + MEMPOOL_HEADER_SIZE(mp, pg_num, 0));
> +
> + /* calculate address of the first element for continuous mempool. */
> + obj = (char *)mp + MEMPOOL_HEADER_SIZE(mp, pg_num, cache_size) +
> + private_data_size;
> +#else
> /* calculate address of the first element for continuous mempool. */
> obj = (char *)mp + MEMPOOL_HEADER_SIZE(mp, pg_num) +
> private_data_size;
> +#endif /* RTE_NEXT_ABI */
> obj = RTE_PTR_ALIGN_CEIL(obj, RTE_MEMPOOL_ALIGN);
>
> /* populate address translation fields. */
> @@ -613,9 +635,8 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
> mp->elt_va_start = (uintptr_t)obj;
> mp->elt_pa[0] = mp->phys_addr +
> (mp->elt_va_start - (uintptr_t)mp);
> -
> - /* mempool elements in a separate chunk of memory. */
> } else {
> + /* mempool elements in a separate chunk of memory. */
> mp->elt_va_start = (uintptr_t)vaddr;
> memcpy(mp->elt_pa, paddr, sizeof (mp->elt_pa[0]) * pg_num);
> }
> @@ -645,10 +666,21 @@ unsigned
> rte_mempool_count(const struct rte_mempool *mp)
> {
> unsigned count;
> +#ifdef RTE_NEXT_ABI
> + unsigned lcore_id;
>
> count = rte_ring_count(mp->ring);
>
> + if (mp->cache_size == 0)
> + return count;
> +
> + for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
> + count += mp->local_cache[lcore_id].len;
> +#else
> #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
> +
> + count = rte_ring_count(mp->ring);
> +
> {
> unsigned lcore_id;
> if (mp->cache_size == 0)
> @@ -658,6 +690,7 @@ rte_mempool_count(const struct rte_mempool *mp)
> count += mp->local_cache[lcore_id].len;
> }
> #endif
> +#endif /* RTE_NEXT_ABI */
>
> /*
> * due to race condition (access to len is not locked), the
> @@ -672,6 +705,24 @@ rte_mempool_count(const struct rte_mempool *mp)
> static unsigned
> rte_mempool_dump_cache(FILE *f, const struct rte_mempool *mp)
> {
> +#ifdef RTE_NEXT_ABI
> + unsigned lcore_id;
> + unsigned count = 0;
> + unsigned cache_count;
> +
> + fprintf(f, " cache infos:\n");
> + fprintf(f, " cache_size=%"PRIu32"\n", mp->cache_size);
> + if (mp->cache_size == 0)
> + return count;
> +
> + for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> + cache_count = mp->local_cache[lcore_id].len;
> + fprintf(f, " cache_count[%u]=%u\n", lcore_id, cache_count);
> + count += cache_count;
> + }
> + fprintf(f, " total_cache_count=%u\n", count);
> + return count;
> +#else
> #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
> unsigned lcore_id;
> unsigned count = 0;
> @@ -691,6 +742,7 @@ rte_mempool_dump_cache(FILE *f, const struct rte_mempool *mp)
> fprintf(f, " cache disabled\n");
> return 0;
> #endif
> +#endif /* RTE_NEXT_ABI */
> }
>
> #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> @@ -755,6 +807,26 @@ mempool_audit_cookies(const struct rte_mempool *mp)
> #define mempool_audit_cookies(mp) do {} while(0)
> #endif
>
> +#ifdef RTE_NEXT_ABI
> +/* check cookies before and after objects */
> +static void
> +mempool_audit_cache(const struct rte_mempool *mp)
> +{
> + /* check cache size consistency */
> + unsigned lcore_id;
> +
> + if (mp->cache_size == 0)
> + return;
> +
> + for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
> + if (mp->local_cache[lcore_id].len > mp->cache_flushthresh) {
> + RTE_LOG(CRIT, MEMPOOL, "badness on cache[%u]\n",
> + lcore_id);
> + rte_panic("MEMPOOL: invalid cache len\n");
> + }
> + }
> +}
> +#else
> #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
> /* check cookies before and after objects */
> static void
> @@ -773,7 +845,7 @@ mempool_audit_cache(const struct rte_mempool *mp)
> #else
> #define mempool_audit_cache(mp) do {} while(0)
> #endif
> -
> +#endif /* RTE_NEXT_ABI */
>
> /* check the consistency of mempool (size, cookies, ...) */
> void
> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> index 9745bf0..b12d6a9 100644
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -95,7 +95,7 @@ struct rte_mempool_debug_stats {
> } __rte_cache_aligned;
> #endif
>
> -#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
> +#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 || defined(RTE_NEXT_ABI) /* Remove line */
> /**
> * A structure that stores a per-core object cache.
> */
> @@ -107,7 +107,7 @@ struct rte_mempool_cache {
> */
> void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 3]; /**< Cache objects */
> } __rte_cache_aligned;
> -#endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */
> +#endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */ /* Remove line RTE_NEXT_ABI */
>
> /**
> * A structure that stores the size of mempool elements.
> @@ -194,10 +194,14 @@ struct rte_mempool {
>
> unsigned private_data_size; /**< Size of private data. */
>
> +#ifdef RTE_NEXT_ABI
> + struct rte_mempool_cache *local_cache; /**< Per-lcore local cache */
> +#else
> #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
> /** Per-lcore local cache. */
> struct rte_mempool_cache local_cache[RTE_MAX_LCORE];
> #endif
> +#endif /* RTE_NEXT_ABI */
>
> #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> /** Per-lcore statistics. */
> @@ -246,6 +250,26 @@ struct rte_mempool {
> #define __MEMPOOL_STAT_ADD(mp, name, n) do {} while(0)
> #endif
>
> +#ifdef RTE_NEXT_ABI
> +/**
> + * Size of elt_pa array size based on number of pages. (Internal use)
> + */
> +#define __PA_SIZE(mp, pgn) \
> + RTE_ALIGN_CEIL((((pgn) - RTE_DIM((mp)->elt_pa)) * \
> + sizeof((mp)->elt_pa[0])), RTE_CACHE_LINE_SIZE)
> +
> +/**
> + * Calculate the size of the mempool header.
> + *
> + * @param mp
> + * Pointer to the memory pool.
> + * @param pgn
> + * Number of pages used to store mempool objects.
> + */
> +#define MEMPOOL_HEADER_SIZE(mp, pgn, cs) \
> + (sizeof(*(mp)) + __PA_SIZE(mp, pgn) + (((cs) == 0) ? 0 : \
> + (sizeof(struct rte_mempool_cache) * RTE_MAX_LCORE)))
> +#else
> /**
> * Calculate the size of the mempool header.
> *
> @@ -257,6 +281,7 @@ struct rte_mempool {
> #define MEMPOOL_HEADER_SIZE(mp, pgn) (sizeof(*(mp)) + \
> RTE_ALIGN_CEIL(((pgn) - RTE_DIM((mp)->elt_pa)) * \
> sizeof ((mp)->elt_pa[0]), RTE_CACHE_LINE_SIZE))
> +#endif /* RTE_NEXT_ABI */
>
> /**
> * Return true if the whole mempool is in contiguous memory.
> @@ -755,19 +780,19 @@ static inline void __attribute__((always_inline))
> __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table,
> unsigned n, int is_mp)
> {
> -#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
> +#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 || defined(RTE_NEXT_ABI) /* Remove line */
> struct rte_mempool_cache *cache;
> uint32_t index;
> void **cache_objs;
> unsigned lcore_id = rte_lcore_id();
> uint32_t cache_size = mp->cache_size;
> uint32_t flushthresh = mp->cache_flushthresh;
> -#endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */
> +#endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */ /* Remove line RTE_NEXT_ABI */
>
> /* increment stat now, adding in mempool always success */
> __MEMPOOL_STAT_ADD(mp, put, n);
>
> -#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
> +#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 || defined(RTE_NEXT_ABI) /* Remove line */
> /* cache is not enabled or single producer or non-EAL thread */
> if (unlikely(cache_size == 0 || is_mp == 0 ||
> lcore_id >= RTE_MAX_LCORE))
> @@ -802,7 +827,7 @@ __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table,
> return;
>
> ring_enqueue:
> -#endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */
> +#endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */ /* Remove line RTE_NEXT_ABI */
>
> /* push remaining objects in ring */
> #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> @@ -946,7 +971,7 @@ __mempool_get_bulk(struct rte_mempool *mp, void **obj_table,
> unsigned n, int is_mc)
> {
> int ret;
> -#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
> +#if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 || defined(RTE_NEXT_ABI) /* Remove line */
> struct rte_mempool_cache *cache;
> uint32_t index, len;
> void **cache_objs;
> @@ -992,7 +1017,7 @@ __mempool_get_bulk(struct rte_mempool *mp, void **obj_table,
> return 0;
>
> ring_dequeue:
> -#endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */
> +#endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */ /* Remove line RTE_NEXT_ABI */
>
> /* get remaining objects from ring */
> if (is_mc)
> @@ -1293,7 +1318,12 @@ void rte_mempool_audit(const struct rte_mempool *mp);
> */
> static inline void *rte_mempool_get_priv(struct rte_mempool *mp)
> {
> +#ifdef RTE_NEXT_ABI
> + return (char *)mp +
> + MEMPOOL_HEADER_SIZE(mp, mp->pg_num, mp->cache_size);
> +#else
> return (char *)mp + MEMPOOL_HEADER_SIZE(mp, mp->pg_num);
> +#endif /* RTE_NEXT_ABI */
> }
>
> /**
>
This is not RTE_NEXT_ABI material IMO, the added ifdef clutter is just
too much.
I'd suggest adding a deprecation notice for the change now and after
16.04 is released, just resend the patch without messing with RTE_NEXT_ABI.
- Pnau -
next prev parent reply other threads:[~2016-02-12 11:23 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-02 23:02 [PATCH] mempool: Reduce rte_mempool structure size Keith Wiles
2016-02-03 17:11 ` Ananyev, Konstantin
2016-02-08 11:02 ` Olivier MATZ
2016-02-08 15:57 ` Wiles, Keith
2016-02-09 17:30 ` [PATCH v2] mempool: reduce " Keith Wiles
2016-02-10 16:59 ` Olivier MATZ
2016-02-10 17:22 ` Wiles, Keith
2016-02-10 18:35 ` Wiles, Keith
2016-02-10 20:06 ` Olivier MATZ
2016-02-10 21:18 ` [PATCH v3] " Keith Wiles
2016-02-12 11:23 ` Panu Matilainen [this message]
2016-02-12 13:57 ` Thomas Monjalon
2016-02-12 14:19 ` Panu Matilainen
2016-02-12 15:07 ` Wiles, Keith
2016-02-12 15:38 ` Thomas Monjalon
2016-02-12 15:50 ` Olivier MATZ
2016-02-12 15:58 ` Wiles, Keith
2016-02-15 9:58 ` Hunt, David
2016-02-15 10:15 ` Olivier MATZ
2016-02-15 10:21 ` Hunt, David
2016-02-15 12:31 ` Olivier MATZ
2016-02-12 15:54 ` Wiles, Keith
2016-02-12 18:36 ` [PATCH v4] " Keith Wiles
2016-02-15 9:20 ` Olivier MATZ
2016-04-14 9:42 ` [PATCH v5] " Olivier Matz
2016-04-14 13:28 ` Wiles, Keith
2016-04-14 13:43 ` Olivier MATZ
2016-04-14 13:53 ` Wiles, Keith
2016-05-17 5:31 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56BDC0C1.9010506@redhat.com \
--to=pmatilai@redhat.com \
--cc=dev@dpdk.org \
--cc=keith.wiles@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.