Re: [dpdk-dev] [PATCH] acl: use rte_calloc for temporary memory allocation

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

Hi Vlad, 

> 
> Hi Konstantin,
> 
> Thanks for your feedback.
> 
> Would you accept this change as config file compile-time parameter with libc calloc as default?
> It is one line change only so it is easy to ifdef.

It is an option, but the main requirements from the community
is to minimize number of build time config options, and instead provide
ability to the user to configure things at runtime.
That's why I thought about something like:

+ /* use EAL hugepages for temporary memory allocations,
+  * might improve build time, build increases hugepages
+  * demand significantly.
+  */
#define	RTE_ACL_CFG_FLAG_HMEM	1

struct rte_acl_config {
	uint32_t num_categories; /**< Number of categories to build with. */
	uint32_t num_fields;     /**< Number of field definitions. */
	struct rte_acl_field_def defs[RTE_ACL_MAX_FIELDS];
	/**< array of field definitions. */
	size_t max_size;
	/**< max memory limit for internal run-time structures. */
+	uint64_t flags;
}; 

And then change tb_pool() to use either calloc() or rte_alloc() based on the flags value.
Another, probably even better and more flexible way is to allow user to specify his own alloc routine:

struct rte_acl_config {
	uint32_t num_categories; /**< Number of categories to build with. */
	uint32_t num_fields;     /**< Number of field definitions. */
	struct rte_acl_field_def defs[RTE_ACL_MAX_FIELDS];
	/**< array of field definitions. */
	size_t max_size;
	/**< max memory limit for internal run-time structures. */
+	void *(tballoc)( (size_t, size_t);
+	void (*tbfree)(void *);
};

In that case user can provide his own alloc/free based on rte_calloc/rte_free
or even on something else.
The only drawback I see with that approaches, that in that case, you'll need to follow
ABI compliance rules, which probably means that your change might not make 16.11.

Konstantin 

> 
> Regards,
> Vlad
> 
> -----Original Message-----
> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> Sent: Wednesday, August 31, 2016 4:28 AM
> To: Vladyslav Buslov
> Cc: dev@dpdk.org
> Subject: RE: [PATCH] acl: use rte_calloc for temporary memory allocation
> 
> Hi Vladyslav,
> 
> > -----Original Message-----
> > From: Vladyslav Buslov [mailto:vladyslav.buslov@harmonicinc.com]
> > Sent: Tuesday, August 16, 2016 3:01 PM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Cc: dev@dpdk.org
> > Subject: [PATCH] acl: use rte_calloc for temporary memory allocation
> >
> > Acl build process uses significant amount of memory which degrades
> > performance by causing page walks when memory is allocated on regular heap using libc calloc.
> >
> > This commit changes tb_mem to allocate temporary memory on huge pages with rte_calloc.
> 
> We deliberately used standard system memory allocation routines (calloc/free) here.
> With current design build phase was never considered to be an 'RT' phase operation.
> It is pretty cpu and memory expensive.
> So if we'll use RTE memory for build phase it could easily consume all (or most) of it, and might cause DPDK process failure or degradation.
> If you really feel that you (and other users) would benefit from using rte_calloc here (I personally still in doubt), then at least it should be a
> new field inside rte_acl_config, that would allow user to control that behavior.
> Though, as I said above, librte_acl was never designed to ' to allocate tens of thousands of ACLs at runtime'.
> To add ability to add/delete rules at runtime while keeping lookup time reasonably low some new approach need to be introduced.
> Konstantin
> 
> >
> > Signed-off-by: Vladyslav Buslov <vladyslav.buslov@harmonicinc.com>
> > ---
> >  lib/librte_acl/tb_mem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_acl/tb_mem.c b/lib/librte_acl/tb_mem.c index
> > 157e608..c373673 100644
> > --- a/lib/librte_acl/tb_mem.c
> > +++ b/lib/librte_acl/tb_mem.c
> > @@ -52,7 +52,7 @@ tb_pool(struct tb_mem_pool *pool, size_t sz)
> >  	size_t size;
> >
> >  	size = sz + pool->alignment - 1;
> > -	block = calloc(1, size + sizeof(*pool->block));
> > +	block = rte_calloc("ACL_TBMEM_BLOCK", 1, size +
> > +sizeof(*pool->block), 0);
> >  	if (block == NULL) {
> >  		RTE_LOG(ERR, MALLOC, "%s(%zu)\n failed, currently allocated "
> >  			"by pool: %zu bytes\n", __func__, sz, pool->alloc);
> > --
> > 2.8.3