Re: [dpdk-dev] [PATCH] mempool: limit cache_size

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: Zoltan Kiss [mailto:zoltan.kiss@linaro.org]
> Sent: Monday, May 18, 2015 2:31 PM
> To: Ananyev, Konstantin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] mempool: limit cache_size
> 
> 
> 
> On 18/05/15 14:14, Ananyev, Konstantin wrote:
> >
> >
> >> -----Original Message-----
> >> From: Zoltan Kiss [mailto:zoltan.kiss@linaro.org]
> >> Sent: Monday, May 18, 2015 1:50 PM
> >> To: Ananyev, Konstantin; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH] mempool: limit cache_size
> >>
> >>
> >>
> >> On 18/05/15 13:41, Ananyev, Konstantin wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zoltan Kiss
> >>>> Sent: Monday, May 18, 2015 1:28 PM
> >>>> To: dev@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH] mempool: limit cache_size
> >>>>
> >>>> Hi,
> >>>>
> >>>> Any opinion on this patch?
> >>>>
> >>>> Regards,
> >>>>
> >>>> Zoltan
> >>>>
> >>>> On 13/05/15 19:59, Zoltan Kiss wrote:
> >>>>> Otherwise cache_flushthresh can be bigger than n, and
> >>>>> a consumer can starve others by keeping every element
> >>>>> either in use or in the cache.
> >>>>>
> >>>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> >>>>> ---
> >>>>>     lib/librte_mempool/rte_mempool.c | 3 ++-
> >>>>>     lib/librte_mempool/rte_mempool.h | 2 +-
> >>>>>     2 files changed, 3 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> >>>>> index cf7ed76..ca6cd9c 100644
> >>>>> --- a/lib/librte_mempool/rte_mempool.c
> >>>>> +++ b/lib/librte_mempool/rte_mempool.c
> >>>>> @@ -440,7 +440,8 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
> >>>>>     	mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list);
> >>>>>
> >>>>>     	/* asked cache too big */
> >>>>> -	if (cache_size > RTE_MEMPOOL_CACHE_MAX_SIZE) {
> >>>>> +	if (cache_size > RTE_MEMPOOL_CACHE_MAX_SIZE ||
> >>>>> +	    (uint32_t) cache_size * CACHE_FLUSHTHRESH_MULTIPLIER > n) {
> >>>>>     		rte_errno = EINVAL;
> >>>>>     		return NULL;
> >>>>>     	}
> >>>
> >>> Why just no 'cache_size > n' then?
> >>
> >> The commit message says: "Otherwise cache_flushthresh can be bigger than
> >> n, and a consumer can starve others by keeping every element either in
> >> use or in the cache."
> >
> > Ah yes, you right - your condition is more restrictive, which is better.
> > Though here you implicitly convert cache_size and n to floats and compare 2 floats :
> > (uint32_t) cache_size * CACHE_FLUSHTHRESH_MULTIPLIER > n)
> > Shouldn't it be:
> > (uint32_t)(cache_size * CACHE_FLUSHTHRESH_MULTIPLIER) > n)
> > So we do conversion back to uint32_t compare to unsigned integers instead?
> > Same as below:
> > mp->cache_flushthresh = (uint32_t)
> >                  (cache_size * CACHE_FLUSHTHRESH_MULTIPLIER);
> 
> To bring it further: how about ditching the whole cache_flushthresh
> member of the mempool structure, and use this:
> 
> #define CACHE_FLUSHTHRESH(mp) (uint32_t)((mp)->cache_size * 1.5)

That's quite expensive and I think would slow down mempool_put() quite a lot .
So I'd suggest we keep cache_flushthresh as it is.

> 
> Furthermore, do we want to expose the flush threshold multiplier through
> the config file?

Hmm, my opinion is no - so far no one ask for that,
and as general tendency - we trying to reduce number of options in config file.
Do you have any good justification when current value is not good enough?
Anyway, that probably could be a subject of another patch/discussion.
Konstantin

> 
> > ?
> >
> > In fact, as we use it more than once, it probably makes sense to create a macro for it,
> > something like:
> > #define CALC_CACHE_FLUSHTHRESH(c)	((uint32_t)((c) *  CACHE_FLUSHTHRESH_MULTIPLIER)
> >
> > Or even
> >
> > #define CALC_CACHE_FLUSHTHRESH(c)	((typeof (c))((c) *  CACHE_FLUSHTHRESH_MULTIPLIER)
> >
> >
> > Konstantin
> >
> >>
> >>> Konstantin
> >>>
> >>>>> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> >>>>> index 9001312..a4a9610 100644
> >>>>> --- a/lib/librte_mempool/rte_mempool.h
> >>>>> +++ b/lib/librte_mempool/rte_mempool.h
> >>>>> @@ -468,7 +468,7 @@ typedef void (rte_mempool_ctor_t)(struct rte_mempool *, void *);
> >>>>>      *   If cache_size is non-zero, the rte_mempool library will try to
> >>>>>      *   limit the accesses to the common lockless pool, by maintaining a
> >>>>>      *   per-lcore object cache. This argument must be lower or equal to
> >>>>> - *   CONFIG_RTE_MEMPOOL_CACHE_MAX_SIZE. It is advised to choose
> >>>>> + *   CONFIG_RTE_MEMPOOL_CACHE_MAX_SIZE and n / 1.5. It is advised to choose
> >>>>>      *   cache_size to have "n modulo cache_size == 0": if this is
> >>>>>      *   not the case, some elements will always stay in the pool and will
> >>>>>      *   never be used. The access to the per-lcore table is of course
> >>>>>