From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id D009B5963 for ; Wed, 10 Feb 2016 17:59:41 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84) (envelope-from ) id 1aTY8S-0006WY-E0; Wed, 10 Feb 2016 18:01:00 +0100 To: Keith Wiles , dev@dpdk.org References: <1454454177-26743-1-git-send-email-keith.wiles@intel.com> <1455039006-86816-1-git-send-email-keith.wiles@intel.com> From: Olivier MATZ X-Enigmail-Draft-Status: N1110 Message-ID: <56BB6C77.8080808@6wind.com> Date: Wed, 10 Feb 2016 17:59:35 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.3.0 MIME-Version: 1.0 In-Reply-To: <1455039006-86816-1-git-send-email-keith.wiles@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2] mempool: reduce rte_mempool structure size X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Feb 2016 16:59:41 -0000 Hi Keith, Thank you for adding the RTE_NEXT_ABI. I think this is the way described in the process. Your changes will be available in next version (16.4) for people compiling with RTE_NEXT_ABI=y, and in 16.7 without option (I'm just surprised that RTE_NEXT_ABI=y in default configs...). I think a deprecation notice should also be added in this commit in doc/guides/rel_notes/deprecation.rst. Please also find comments below. On 02/09/2016 06:30 PM, Keith Wiles wrote: > diff --git a/config/defconfig_x86_64-native-linuxapp-gcc b/config/defconfig_x86_64-native-linuxapp-gcc > index 60baf5b..02e9ace 100644 > --- a/config/defconfig_x86_64-native-linuxapp-gcc > +++ b/config/defconfig_x86_64-native-linuxapp-gcc > @@ -40,3 +40,8 @@ CONFIG_RTE_ARCH_64=y > > CONFIG_RTE_TOOLCHAIN="gcc" > CONFIG_RTE_TOOLCHAIN_GCC=y > +CONFIG_RTE_BUILD_SHARED_LIB=y > +CONFIG_RTE_NEXT_ABI=n > +CONFIG_RTE_EAL_IGB_UIO=n > +CONFIG_RTE_LIBRTE_KNI=n > +CONFIG_RTE_KNI_KMOD=n I think this should not be part of the patch. > @@ -672,6 +704,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; I think in this case we could avoid to duplicate the code without beeing unclear by using the proper #ifdefs: #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 || defined(RTE_NEXT_ABI) /* common code */ #ifdef RTE_NEXT_ABI if (mp->cache_size == 0) return count; #endif /* common code */ #else ... #endif > @@ -755,6 +806,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 same here > diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h > index 6e2390a..fc9b595 100644 > --- a/lib/librte_mempool/rte_mempool.h > +++ b/lib/librte_mempool/rte_mempool.h > @@ -95,6 +95,19 @@ struct rte_mempool_debug_stats { > } __rte_cache_aligned; > #endif > > +#ifdef RTE_NEXT_ABI > +/** > + * A structure that stores a per-core object cache. > + */ > +struct rte_mempool_cache { > + unsigned len; /**< Cache len */ > + /* > + * Cache is allocated to this size to allow it to overflow in certain > + * cases to avoid needless emptying of cache. > + */ > + void *objs[RTE_MEMPOOL_CACHE_MAX_SIZE * 3]; /**< Cache objects */ > +} __rte_cache_aligned; > +#else same here > @@ -755,19 +793,25 @@ static inline void __attribute__((always_inline)) > __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table, > unsigned n, int is_mp) > { > +#ifndef RTE_NEXT_ABI /* Note: ifndef */ > #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 > +#endif /* RTE_NEXT_ABI */ > 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; > +#ifndef RTE_NEXT_ABI /* Note: ifndef */ > #endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */ > +#endif /* RTE_NEXT_ABI */ this looks strange... I think it does not work properly. Why not #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 || defined(RTE_NEXT_ABI) > /* increment stat now, adding in mempool always success */ > __MEMPOOL_STAT_ADD(mp, put, n); > > +#ifndef RTE_NEXT_ABI /* Note: ifndef */ > #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 > +#endif /* RTE_NEXT_ABI */ > /* 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 +846,9 @@ __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table, > return; > > ring_enqueue: > +#ifndef RTE_NEXT_ABI /* Note: ifndef */ > #endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */ > +#endif /* RTE_NEXT_ABI */ > > /* push remaining objects in ring */ > #ifdef RTE_LIBRTE_MEMPOOL_DEBUG > @@ -946,7 +992,9 @@ __mempool_get_bulk(struct rte_mempool *mp, void **obj_table, > unsigned n, int is_mc) > { > int ret; > +#ifndef RTE_NEXT_ABI /* Note: ifndef */ > #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 > +#endif /* RTE_NEXT_ABI */ > struct rte_mempool_cache *cache; > uint32_t index, len; > void **cache_objs; > @@ -992,7 +1040,9 @@ __mempool_get_bulk(struct rte_mempool *mp, void **obj_table, > return 0; > > ring_dequeue: > +#ifndef RTE_NEXT_ABI /* Note: ifndef */ > #endif /* RTE_MEMPOOL_CACHE_MAX_SIZE > 0 */ > +#endif /* RTE_NEXT_ABI */ > > /* get remaining objects from ring */ > if (is_mc) Same in those cases. Regards, Olivier