Thanks for review. Sent out v2.
Br, Zhike Wang JDCloud, Product Development, IaaS ------------------------------------------------------------------------------------------------ Mobile／+86 13466719566 E- mail／wangzh...@jd.com Address／5F Building A,North-Star Century Center,8 Beichen West Street,Chaoyang District Beijing Https://JDCloud.com ------------------------------------------------------------------------------------------------ -----Original Message----- From: Burakov, Anatoly [mailto:anatoly.bura...@intel.com] Sent: Monday, July 13, 2020 10:53 PM To: Andrew Rybchenko; 王志克; dev@dpdk.org Cc: olivier.m...@6wind.com Subject: Re: [dpdk-dev] [PATCH] mempool: fix memory allocation in memzones during retry. On 13-Jul-20 12:29 PM, Andrew Rybchenko wrote: > On 7/13/20 2:17 PM, Burakov, Anatoly wrote: >> On 13-Jul-20 4:40 AM, Zhike Wang wrote: >>> If allocation is successful on the first attempt, typically >>> there is no problem since we allocated everything required and >>> we'll terminate the loop (if memory chunk is really sufficient >>> to populate required number of mempool elements). >>> >>> If the first attempt fails, we try to allocate half >>> of mem_size and it succeed, we'll have one more iteration of >>> the for-loop to allocate memory for remaining elements and >>> should not try the next time with quarter of the mem_size. >>> >>> It is wrong that max_alloc_size is divided by 2 in the >>> case of successful allocation as well, or invalid memory >>> can be allocated, and leads to population failure, then errno >>> other than ENOMEM may be returned. >>> >>> Signed-off-by: Andrew Rybchenko <arybche...@solarflare.com> >>> Signed-off-by: Zhike Wang <wangzh...@jd.com> >>> --- >>> lib/librte_mempool/rte_mempool.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/lib/librte_mempool/rte_mempool.c >>> b/lib/librte_mempool/rte_mempool.c >>> index a2bd249..b8f2629 100644 >>> --- a/lib/librte_mempool/rte_mempool.c >>> +++ b/lib/librte_mempool/rte_mempool.c >>> @@ -635,7 +635,7 @@ struct pagesz_walk_arg { >>> RTE_MIN((size_t)mem_size, max_alloc_size), >>> mp->socket_id, mz_flags, align); >>> - if (mz == NULL && rte_errno != ENOMEM) >>> + if ((mz != NULL) || (mz == NULL && rte_errno != ENOMEM)) >> >> I think checking mz == NULL for the second time is redundant, as if >> we're hitting the second branch, we've already failed the "mz != NULL" >> test and can therefore assume that mz == NULL. > > Yes, of course. (Also parenthesis will be not required.) > >> >> That said, i'm struggling to think of circumstances where this would >> matter. Could you please provide an example? > > If the question about break in the case of mz != NULL, > it is important to avoid decreasing max_alloc_size to > try the same size once again if one more iteration is > needed to allocate remaining elements. Right, no further questions :) > >> >>> break; >>> max_alloc_size = RTE_MIN(max_alloc_size, >>> >> >> This should have a Fixes: tag. >> > > Yes, missed it. > > Many thanks for the review. > -- Thanks, Anatoly
