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