* [dpdk-dev] [PATCH] mempool: limit cache_size @ 2015-05-13 18:59 Zoltan Kiss 2015-05-18 12:27 ` Zoltan Kiss 0 siblings, 1 reply; 9+ messages in thread From: Zoltan Kiss @ 2015-05-13 18:59 UTC (permalink / raw) To: dev 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; } 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 -- 1.9.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: limit cache_size 2015-05-13 18:59 [dpdk-dev] [PATCH] mempool: limit cache_size Zoltan Kiss @ 2015-05-18 12:27 ` Zoltan Kiss 2015-05-18 12:31 ` Bruce Richardson 2015-05-18 12:41 ` Ananyev, Konstantin 0 siblings, 2 replies; 9+ messages in thread From: Zoltan Kiss @ 2015-05-18 12:27 UTC (permalink / raw) To: dev 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; > } > 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: limit cache_size 2015-05-18 12:27 ` Zoltan Kiss @ 2015-05-18 12:31 ` Bruce Richardson 2015-05-18 12:41 ` Ananyev, Konstantin 1 sibling, 0 replies; 9+ messages in thread From: Bruce Richardson @ 2015-05-18 12:31 UTC (permalink / raw) To: Zoltan Kiss; +Cc: dev On Mon, May 18, 2015 at 01:27:45PM +0100, Zoltan Kiss wrote: > 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> Seems reasonable enough to me. Acked-by: Bruce Richardson <bruce.richardson@intel.com> > >--- > > 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; > > } > >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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: limit cache_size 2015-05-18 12:27 ` Zoltan Kiss 2015-05-18 12:31 ` Bruce Richardson @ 2015-05-18 12:41 ` Ananyev, Konstantin 2015-05-18 12:50 ` Zoltan Kiss 1 sibling, 1 reply; 9+ messages in thread From: Ananyev, Konstantin @ 2015-05-18 12:41 UTC (permalink / raw) To: Zoltan Kiss, dev > -----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? 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: limit cache_size 2015-05-18 12:41 ` Ananyev, Konstantin @ 2015-05-18 12:50 ` Zoltan Kiss 2015-05-18 13:14 ` Ananyev, Konstantin 0 siblings, 1 reply; 9+ messages in thread From: Zoltan Kiss @ 2015-05-18 12:50 UTC (permalink / raw) To: Ananyev, Konstantin, dev 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." > 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 >>> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: limit cache_size 2015-05-18 12:50 ` Zoltan Kiss @ 2015-05-18 13:14 ` Ananyev, Konstantin 2015-05-18 13:31 ` Zoltan Kiss 0 siblings, 1 reply; 9+ messages in thread From: Ananyev, Konstantin @ 2015-05-18 13:14 UTC (permalink / raw) To: Zoltan Kiss, dev > -----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); ? 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 > >>> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: limit cache_size 2015-05-18 13:14 ` Ananyev, Konstantin @ 2015-05-18 13:31 ` Zoltan Kiss 2015-05-18 14:13 ` Ananyev, Konstantin 0 siblings, 1 reply; 9+ messages in thread From: Zoltan Kiss @ 2015-05-18 13:31 UTC (permalink / raw) To: Ananyev, Konstantin, dev 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) Furthermore, do we want to expose the flush threshold multiplier through the config file? > ? > > 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 >>>>> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: limit cache_size 2015-05-18 13:31 ` Zoltan Kiss @ 2015-05-18 14:13 ` Ananyev, Konstantin 2015-05-18 15:48 ` Zoltan Kiss 0 siblings, 1 reply; 9+ messages in thread From: Ananyev, Konstantin @ 2015-05-18 14:13 UTC (permalink / raw) To: Zoltan Kiss, dev > -----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 > >>>>> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dpdk-dev] [PATCH] mempool: limit cache_size 2015-05-18 14:13 ` Ananyev, Konstantin @ 2015-05-18 15:48 ` Zoltan Kiss 0 siblings, 0 replies; 9+ messages in thread From: Zoltan Kiss @ 2015-05-18 15:48 UTC (permalink / raw) To: Ananyev, Konstantin, dev On 18/05/15 15:13, Ananyev, Konstantin wrote: > > >> -----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. Ok, I have posted a v2 based on your suggestion. > >> >> 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? Nothing special, just the arbitrary value choice seemed a bit odd. > 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 >>>>>>> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-05-18 15:48 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-05-13 18:59 [dpdk-dev] [PATCH] mempool: limit cache_size Zoltan Kiss 2015-05-18 12:27 ` Zoltan Kiss 2015-05-18 12:31 ` Bruce Richardson 2015-05-18 12:41 ` Ananyev, Konstantin 2015-05-18 12:50 ` Zoltan Kiss 2015-05-18 13:14 ` Ananyev, Konstantin 2015-05-18 13:31 ` Zoltan Kiss 2015-05-18 14:13 ` Ananyev, Konstantin 2015-05-18 15:48 ` Zoltan Kiss
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).