RE: [PATCH v5] net/bonding: another fix to LACP mempool size

["Morten Brørup" <mb@smartsharesystems.com>]

+CC mempool maintainers

> From: Gaoxiang Liu [mailto:gaoxiangliu0@163.com]
> Sent: Friday, 25 March 2022 14.02
> 
> The following log message may appear after a slave is idle(or nearly
> idle)
> for a few minutes:"PMD: Failed to allocate LACP packet from pool".
> And bond mode 4 negotiation may fail.
> 
> Problem:When bond mode 4 has been chosed and delicated queue has
> not been enable, all mbufs from a slave' private pool(used
> exclusively for transmitting LACPDUs) have been allocated in
> interrupt thread, and are still sitting in the device's tx
> descriptor ring and other cores' mempool caches in fwd thread.
> Thus the interrupt thread can not alloc LACP packet from pool.
> 
> Solution: Ensure that each slave'tx (LACPDU) mempool owns more than
> n-tx-queues * n-tx-descriptor + fwd_core_num *
> per-core-mmempool-flush-threshold mbufs.
> 
> Note that the LACP tx machine fuction is the only code that allocates
> from a slave's private pool. It runs in the context of the interrupt
> thread, and thus it has no mempool cache of its own.
> 
> Signed-off-by: Gaoxiang Liu <liugaoxiang@huawei.com>
> 
> ---
> v2:
> * Fixed compile issues.
> 
> v3:
> * delete duplicate code.
> 
> v4;
> * Fixed some issues.
> 1. total_tx_desc should use +=
> 2. add detailed logs
> 
> v5:
> * Fixed some issues.
> 1. move CACHE_FLUSHTHRESH_MULTIPLIER to rte_eth_bond-8023ad.c
> 2. use RTE_MIN
> ---
>  drivers/net/bonding/rte_eth_bond_8023ad.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.c
> b/drivers/net/bonding/rte_eth_bond_8023ad.c
> index ca50583d62..2c39b0d062 100644
> --- a/drivers/net/bonding/rte_eth_bond_8023ad.c
> +++ b/drivers/net/bonding/rte_eth_bond_8023ad.c
> @@ -1050,6 +1050,7 @@ bond_mode_8023ad_activate_slave(struct
> rte_eth_dev *bond_dev,
>  	uint32_t total_tx_desc;
>  	struct bond_tx_queue *bd_tx_q;
>  	uint16_t q_id;
> +	uint32_t cache_size;
> 
>  	/* Given slave mus not be in active list */
>  	RTE_ASSERT(find_slave_by_id(internals->active_slaves,
> @@ -1100,11 +1101,15 @@ bond_mode_8023ad_activate_slave(struct
> rte_eth_dev *bond_dev,
>  		total_tx_desc += bd_tx_q->nb_tx_desc;
>  	}
> 
> +/* BONDING_8023AD_CACHE_FLUSHTHRESH_MULTIPLIER  is the same as
> + * CACHE_FLUSHTHRESH_MULTIPLIER already defined in rte_mempool.c */
> +#define BONDING_8023AD_CACHE_FLUSHTHRESH_MULTIPLIER 1.5

Very important comment. Thank you!

May I suggest that a similar comment is added to the rte_mempool.c file, so if CACHE_FLUSHTHRESH_MULTIPLIER is changed there, we don't forget to change the copy-pasted code in the rte_eth_bond_8023ad.c file too. It has previously been discussed changing it from 1.5 to 2 for symmetry reasons.

> +
> +	cache_size = RTE_MIN(RTE_MEMPOOL_CACHE_MAX_SIZE, 32);
> +	total_tx_desc += rte_lcore_count() * cache_size *
> BONDING_8023AD_CACHE_FLUSHTHRESH_MULTIPLIER;
>  	snprintf(mem_name, RTE_DIM(mem_name), "slave_port%u_pool",
> slave_id);
>  	port->mbuf_pool = rte_pktmbuf_pool_create(mem_name,
> total_tx_desc,
> -		RTE_MEMPOOL_CACHE_MAX_SIZE >= 32 ?
> -			32 : RTE_MEMPOOL_CACHE_MAX_SIZE,
> -		0, element_size, socket_id);
> +		cache_size, 0, element_size, socket_id);
> 
>  	/* Any memory allocation failure in initialization is critical
> because
>  	 * resources can't be free, so reinitialization is impossible. */
> --
> 2.32.0
>