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

["Wang, Haiyue" <haiyue.wang@intel.com>]

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Friday, March 25, 2022 21:14
> To: Gaoxiang Liu <gaoxiangliu0@163.com>; chas3@att.com; humin29@huawei.com
> Cc: dev@dpdk.org; liugaoxiang@huawei.com; olivier.matz@6wind.com; andrew.rybchenko@oktetlabs.ru
> Subject: RE: [PATCH v5] net/bonding: another fix to LACP mempool size
> 
> +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 ++++++++---


> >
> > +/* 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.

Then, introduce some kind of public API macro, like
RTE_MEMPOOL_CACHE_MAX_FLUSHTHRESH_MULTIPLIER as RTE_MEMPOOL_CACHE_MAX_SIZE does ?
So that when calling mempool create API, it can do other kind of calculation, like
RTE_MIN(user's new flush multiper, RTE_MEMPOOL_CACHE_MAX_FLUSHTHRESH_MULTIPLIER).

Just a suggestion, so that no need add strange comments like BONDING_8023AD in an
lib.

> 
> > +
> > +	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
> >