Re: [PATCH v9 1/7] eal: add static per-lcore memory allocation facility

[Thomas Monjalon <thomas@monjalon.net>]

11/10/2024 10:09, Morten Brørup:
> > > +static void *
> > > +lcore_var_alloc(size_t size, size_t align)
> > > +{
> > > +	void *handle;
> > > +	unsigned int lcore_id;
> > > +	void *value;
> > > +
> > > +	offset = RTE_ALIGN_CEIL(offset, align);
> > > +
> > > +	if (offset + size > RTE_MAX_LCORE_VAR) {
> > > +#ifdef RTE_EXEC_ENV_WINDOWS
> > > +		lcore_buffer = _aligned_malloc(LCORE_BUFFER_SIZE,
> > > +					       RTE_CACHE_LINE_SIZE);
> > > +#else
> > > +		lcore_buffer = aligned_alloc(RTE_CACHE_LINE_SIZE,
> > > +					     LCORE_BUFFER_SIZE);
> > > +#endif
> > > +		RTE_VERIFY(lcore_buffer != NULL);
> > 
> > Please no panic in a lib.
> > You can return NULL.
> 
> I agree with Mattias design here.
> Lcore variables are like RTE_PER_LCORE variables and simple "static" variables.
> If the system does not have enough memory for those, the application will not be able to deal with it.
> Panic early (in this lib) is the correct way to deal with it.

There were discussions in the past where we agreed to remove
as much panic as possible in our libs and drivers.
We want to allow the application to have a chance to cleanup.

I don't think returning NULL in an allocation is something disruptive.

I understand you don't want to manage an error return
in variable declarations, so can we have RTE_VERIFY in declaration macros?


> > > + * Lcore variables cannot and need not be freed.
> > 
> > I'm curious about that.
> > If EAL is closed, and the application continues its life,
> > then we want all this memory to be cleaned as well.
> > Do you know rte_eal_cleanup()?
> 
> Good catch, Thomas! I missed that in my review.
> Mattias, it seems you need a chained list of lcore_buffer allocations for this.

Yes


> > > + * The size of an lcore variable's value must be less than the DPDK
> > 
> > size of variable, not size of value
> 
> Initially, I thought the same as Thomas...
> It is confusing considering the handle the variable, and its instances having values.
> 
> However, during the review, Mattias convinced me of its correctness.
> 
> And by the way, RTE_PER_LCORE also does it:
> https://elixir.bootlin.com/dpdk/v24.07/source/lib/eal/include/rte_per_lcore.h#L48

I understand your point of view and I accept it.