Re: [dpdk-dev] [PATCH v2] mempool: rename functions with confusing names

[Bruce Richardson <bruce.richardson@intel.com>]

+ Correct email for Thomas :-(

On Wed, Jun 29, 2016 at 05:27:15PM +0100, Bruce Richardson wrote:
> The mempool_count and mempool_free_count behaved contrary to what their
> names suggested. The free_count function actually returned the number of
> elements that were allocated from the pool, not the number unallocated as
> the name implied.
> 
> Fix this by introducing two new functions to replace the old ones,
> * rte_mempool_avail_count to replace rte_mempool_count
> * rte_mempool_in_use_count to replace rte_mempool_free_count
> 
> In this patch, the new functions are added, and the old ones are marked
> as deprecated. All apps and examples that use the old functions are
> updated to use the new functions.
> 
> Fixes: af75078fece3 ("first public release")
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> 
> v2 changes
> * original patch used allocated/unallocated for new functions. Change
>   those to in_use/avail.
> 
> ---
>  app/test/test_cryptodev.c                  |  6 ++---
>  app/test/test_cryptodev_perf.c             |  6 ++---
>  app/test/test_mbuf.c                       |  4 +--
>  app/test/test_mempool.c                    |  4 +--
>  app/test/test_mempool_perf.c               |  2 +-
>  doc/guides/rel_notes/deprecation.rst       |  6 +++++
>  drivers/net/qede/qede_rxtx.c               |  4 +--
>  examples/multi_process/l2fwd_fork/main.c   |  3 ++-
>  examples/qos_sched/main.c                  |  2 --
>  lib/librte_mempool/rte_mempool.c           | 10 +++++--
>  lib/librte_mempool/rte_mempool.h           | 42 +++++++++++++++++++++++++++---
>  lib/librte_mempool/rte_mempool_version.map |  1 +
>  12 files changed, 69 insertions(+), 21 deletions(-)
> 
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> index 9dfe34f..fbfe1d0 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -316,12 +316,12 @@ testsuite_teardown(void)
>  
>  	if (ts_params->mbuf_pool != NULL) {
>  		RTE_LOG(DEBUG, USER1, "CRYPTO_MBUFPOOL count %u\n",
> -		rte_mempool_count(ts_params->mbuf_pool));
> +		rte_mempool_avail_count(ts_params->mbuf_pool));
>  	}
>  
>  	if (ts_params->op_mpool != NULL) {
>  		RTE_LOG(DEBUG, USER1, "CRYPTO_OP_POOL count %u\n",
> -		rte_mempool_count(ts_params->op_mpool));
> +		rte_mempool_avail_count(ts_params->op_mpool));
>  	}
>  
>  }
> @@ -412,7 +412,7 @@ ut_teardown(void)
>  
>  	if (ts_params->mbuf_pool != NULL)
>  		RTE_LOG(DEBUG, USER1, "CRYPTO_MBUFPOOL count %u\n",
> -				rte_mempool_count(ts_params->mbuf_pool));
> +			rte_mempool_avail_count(ts_params->mbuf_pool));
>  
>  	rte_cryptodev_stats_get(ts_params->valid_devs[0], &stats);
>  
> diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c
> index e484cbb..d728211 100644
> --- a/app/test/test_cryptodev_perf.c
> +++ b/app/test/test_cryptodev_perf.c
> @@ -343,10 +343,10 @@ testsuite_teardown(void)
>  
>  	if (ts_params->mbuf_mp != NULL)
>  		RTE_LOG(DEBUG, USER1, "CRYPTO_PERF_MBUFPOOL count %u\n",
> -		rte_mempool_count(ts_params->mbuf_mp));
> +		rte_mempool_avail_count(ts_params->mbuf_mp));
>  	if (ts_params->op_mpool != NULL)
>  		RTE_LOG(DEBUG, USER1, "CRYPTO_PERF_OP POOL count %u\n",
> -		rte_mempool_count(ts_params->op_mpool));
> +		rte_mempool_avail_count(ts_params->op_mpool));
>  }
>  
>  static int
> @@ -395,7 +395,7 @@ ut_teardown(void)
>  
>  	if (ts_params->mbuf_mp != NULL)
>  		RTE_LOG(DEBUG, USER1, "CRYPTO_PERF_MBUFPOOL count %u\n",
> -			rte_mempool_count(ts_params->mbuf_mp));
> +			rte_mempool_avail_count(ts_params->mbuf_mp));
>  
>  	rte_cryptodev_stats_get(ts_params->dev_id, &stats);
>  
> diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> index 1835acc..8664885 100644
> --- a/app/test/test_mbuf.c
> +++ b/app/test/test_mbuf.c
> @@ -717,7 +717,7 @@ test_refcnt_iter(unsigned lcore, unsigned iter)
>  	 * - increment it's reference up to N+1,
>  	 * - enqueue it N times into the ring for slave cores to free.
>  	 */
> -	for (i = 0, n = rte_mempool_count(refcnt_pool);
> +	for (i = 0, n = rte_mempool_avail_count(refcnt_pool);
>  	    i != n && (m = rte_pktmbuf_alloc(refcnt_pool)) != NULL;
>  	    i++) {
>  		ref = RTE_MAX(rte_rand() % REFCNT_MAX_REF, 1UL);
> @@ -745,7 +745,7 @@ test_refcnt_iter(unsigned lcore, unsigned iter)
>  
>  	/* check that all mbufs are back into mempool by now */
>  	for (wn = 0; wn != REFCNT_MAX_TIMEOUT; wn++) {
> -		if ((i = rte_mempool_count(refcnt_pool)) == n) {
> +		if ((i = rte_mempool_avail_count(refcnt_pool)) == n) {
>  			refcnt_lcore[lcore] += tref;
>  			printf("%s(lcore=%u, iter=%u) completed, "
>  			    "%u references processed\n",
> diff --git a/app/test/test_mempool.c b/app/test/test_mempool.c
> index 31582d8..db159aa 100644
> --- a/app/test/test_mempool.c
> +++ b/app/test/test_mempool.c
> @@ -210,7 +210,7 @@ test_mempool_basic(struct rte_mempool *mp)
>  
>  	/* tests that improve coverage */
>  	printf("get object count\n");
> -	if (rte_mempool_count(mp) != MEMPOOL_SIZE - 1)
> +	if (rte_mempool_avail_count(mp) != MEMPOOL_SIZE - 1)
>  		RET_ERR();
>  
>  	printf("get private data\n");
> @@ -470,7 +470,7 @@ test_mempool_basic_ex(struct rte_mempool *mp)
>  		return ret;
>  	}
>  	printf("test_mempool_basic_ex now mempool (%s) has %u free entries\n",
> -		mp->name, rte_mempool_free_count(mp));
> +		mp->name, rte_mempool_in_use_count(mp));
>  	if (rte_mempool_full(mp) != 1) {
>  		printf("test_mempool_basic_ex the mempool should be full\n");
>  		goto fail_mp_basic_ex;
> diff --git a/app/test/test_mempool_perf.c b/app/test/test_mempool_perf.c
> index c5f8455..e638a4a 100644
> --- a/app/test/test_mempool_perf.c
> +++ b/app/test/test_mempool_perf.c
> @@ -201,7 +201,7 @@ launch_cores(unsigned cores)
>  	       "n_put_bulk=%u n_keep=%u ",
>  	       (unsigned) mp->cache_size, cores, n_get_bulk, n_put_bulk, n_keep);
>  
> -	if (rte_mempool_count(mp) != MEMPOOL_SIZE) {
> +	if (rte_mempool_avail_count(mp) != MEMPOOL_SIZE) {
>  		printf("mempool is not full\n");
>  		return -1;
>  	}
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 80f873d..1472db2 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -34,6 +34,12 @@ Deprecation Notices
>    compact API. The ones that remain are backwards compatible and use the
>    per-lcore default cache if available. This change targets release 16.07.
>  
> +* The APIs rte_mempool_count and rte_mempool_free_count are being deprecated
> +  on the basis that they are confusing to use - free_count actually returns
> +  the number of allocated entries, not the number of free entries as expected.
> +  They are being replaced by rte_mempool_avail_count and
> +  rte_mempool_in_use_count respectively.
> +
>  * The mbuf flags PKT_RX_VLAN_PKT and PKT_RX_QINQ_PKT are deprecated and
>    are respectively replaced by PKT_RX_VLAN_STRIPPED and
>    PKT_RX_QINQ_STRIPPED, that are better described. The old flags and
> diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
> index ccce5fd..b5a40fe 100644
> --- a/drivers/net/qede/qede_rxtx.c
> +++ b/drivers/net/qede/qede_rxtx.c
> @@ -23,8 +23,8 @@ static inline int qede_alloc_rx_buffer(struct qede_rx_queue *rxq)
>  			   "Failed to allocate rx buffer "
>  			   "sw_rx_prod %u sw_rx_cons %u mp entries %u free %u",
>  			   idx, rxq->sw_rx_cons & NUM_RX_BDS(rxq),
> -			   rte_mempool_count(rxq->mb_pool),
> -			   rte_mempool_free_count(rxq->mb_pool));
> +			   rte_mempool_avail_count(rxq->mb_pool),
> +			   rte_mempool_in_use_count(rxq->mb_pool));
>  		return -ENOMEM;
>  	}
>  	rxq->sw_rx_ring[idx].mbuf = new_mb;
> diff --git a/examples/multi_process/l2fwd_fork/main.c b/examples/multi_process/l2fwd_fork/main.c
> index 368b309..2d951d9 100644
> --- a/examples/multi_process/l2fwd_fork/main.c
> +++ b/examples/multi_process/l2fwd_fork/main.c
> @@ -442,7 +442,8 @@ reset_slave_all_ports(unsigned slaveid)
>  		pool = rte_mempool_lookup(buf_name);
>  		if (pool)
>  			printf("Port %d mempool free object is %u(%u)\n", slave->port[i],
> -				rte_mempool_count(pool), (unsigned)NB_MBUF);
> +				rte_mempool_avail_count(pool),
> +				(unsigned int)NB_MBUF);
>  		else
>  			printf("Can't find mempool %s\n", buf_name);
>  
> diff --git a/examples/qos_sched/main.c b/examples/qos_sched/main.c
> index e16b164..e10cfd4 100644
> --- a/examples/qos_sched/main.c
> +++ b/examples/qos_sched/main.c
> @@ -201,8 +201,6 @@ app_stat(void)
>  				stats.oerrors - tx_stats[i].oerrors);
>  		memcpy(&tx_stats[i], &stats, sizeof(stats));
>  
> -		//printf("MP = %d\n", rte_mempool_count(conf->app_pktmbuf_pool));
> -
>  #if APP_COLLECT_STAT
>  		printf("-------+------------+------------+\n");
>  		printf("       |  received  |   dropped  |\n");
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index e6a83d0..c86d3fd 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -905,8 +905,8 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
>  }
>  
>  /* Return the number of entries in the mempool */
> -unsigned
> -rte_mempool_count(const struct rte_mempool *mp)
> +unsigned int
> +rte_mempool_avail_count(const struct rte_mempool *mp)
>  {
>  	unsigned count;
>  	unsigned lcore_id;
> @@ -928,6 +928,12 @@ rte_mempool_count(const struct rte_mempool *mp)
>  	return count;
>  }
>  
> +unsigned int
> +rte_mempool_count(const struct rte_mempool *mp)
> +{
> +	return rte_mempool_avail_count(mp);
> +}
> +
>  /* dump the cache status */
>  static unsigned
>  rte_mempool_dump_cache(FILE *f, const struct rte_mempool *mp)
> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> index 0a1777c..389ef16 100644
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -1368,9 +1368,44 @@ rte_mempool_get(struct rte_mempool *mp, void **obj_p)
>   * @return
>   *   The number of entries in the mempool.
>   */
> +unsigned int rte_mempool_avail_count(const struct rte_mempool *mp);
> +
> +/**
> + * @deprecated
> + * Return the number of entries in the mempool.
> + *
> + * When cache is enabled, this function has to browse the length of
> + * all lcores, so it should not be used in a data path, but only for
> + * debug purposes.
> + *
> + * @param mp
> + *   A pointer to the mempool structure.
> + * @return
> + *   The number of entries in the mempool.
> + */
> +__rte_deprecated
>  unsigned rte_mempool_count(const struct rte_mempool *mp);
>  
>  /**
> + * Return the number of elements which have been allocated from the mempool
> + *
> + * When cache is enabled, this function has to browse the length of
> + * all lcores, so it should not be used in a data path, but only for
> + * debug purposes.
> + *
> + * @param mp
> + *   A pointer to the mempool structure.
> + * @return
> + *   The number of free entries in the mempool.
> + */
> +static inline unsigned int
> +rte_mempool_in_use_count(const struct rte_mempool *mp)
> +{
> +	return mp->size - rte_mempool_avail_count(mp);
> +}
> +
> +/**
> + * @deprecated
>   * Return the number of free entries in the mempool ring.
>   * i.e. how many entries can be freed back to the mempool.
>   *
> @@ -1387,10 +1422,11 @@ unsigned rte_mempool_count(const struct rte_mempool *mp);
>   * @return
>   *   The number of free entries in the mempool.
>   */
> +__rte_deprecated
>  static inline unsigned
>  rte_mempool_free_count(const struct rte_mempool *mp)
>  {
> -	return mp->size - rte_mempool_count(mp);
> +	return rte_mempool_in_use_count(mp);
>  }
>  
>  /**
> @@ -1409,7 +1445,7 @@ rte_mempool_free_count(const struct rte_mempool *mp)
>  static inline int
>  rte_mempool_full(const struct rte_mempool *mp)
>  {
> -	return !!(rte_mempool_count(mp) == mp->size);
> +	return !!(rte_mempool_avail_count(mp) == mp->size);
>  }
>  
>  /**
> @@ -1428,7 +1464,7 @@ rte_mempool_full(const struct rte_mempool *mp)
>  static inline int
>  rte_mempool_empty(const struct rte_mempool *mp)
>  {
> -	return !!(rte_mempool_count(mp) == 0);
> +	return !!(rte_mempool_avail_count(mp) == 0);
>  }
>  
>  /**
> diff --git a/lib/librte_mempool/rte_mempool_version.map b/lib/librte_mempool/rte_mempool_version.map
> index 9bcbf17..ffd0daf 100644
> --- a/lib/librte_mempool/rte_mempool_version.map
> +++ b/lib/librte_mempool/rte_mempool_version.map
> @@ -32,5 +32,6 @@ DPDK_16.07 {
>  	rte_mempool_populate_virt;
>  	rte_mempool_register_ops;
>  	rte_mempool_set_ops_byname;
> +	rte_mempool_avail_count;
>  
>  } DPDK_2.0;
> -- 
> 2.5.5
>