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 B703A58C3 for ; Wed, 10 Feb 2016 21:06:20 +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 1aTb31-0006h4-T8; Wed, 10 Feb 2016 21:07:36 +0100 To: "Wiles, Keith" , "dev@dpdk.org" References: <1454454177-26743-1-git-send-email-keith.wiles@intel.com> <1455039006-86816-1-git-send-email-keith.wiles@intel.com> <56BB6C77.8080808@6wind.com> From: Olivier MATZ Message-ID: <56BB9832.10302@6wind.com> Date: Wed, 10 Feb 2016 21:06:10 +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: Content-Type: text/plain; charset=utf-8 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 20:06:20 -0000 Hi Keith, On 02/10/2016 07:35 PM, Wiles, Keith wrote: >>> @@ -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 > > Started looking at this change and the problem is I want to remove the ifdef RTE_MEMPOOL.. As well as the #else/#endif code. If I do as you suggest it does not appear to be clearer when someone goes back to remove the code, they may think the #ifdef RTE_MEMPOOL/#else/#endif are still required. OK, makes sense. >>> @@ -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) > > Yes I agree the ifndef looks strange, but it should work as we want to remove the #ifdef RTE_MEMPOOL/#endif lines. This was the reason for the comment that it was a ifndef. It's not only strange, it's also probably not what you want to do: #ifndef RTE_NEXT_ABI /* Note: ifndef */ #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0 #endif /* RTE_NEXT_ABI */ ... Here, the #endif corresponds to the second #if, not the first #ifdef. Regards, Olivier