Re: [dpdk-dev] [EXT] Re: [PATCH v3 2/7] eal/interrupts: implement get set APIs

[Thomas Monjalon <thomas@monjalon.net>]

19/10/2021 10:32, Harman Kalra:
> From: Stephen Hemminger <stephen@networkplumber.org>
> > On Tue, 19 Oct 2021 01:07:02 +0530
> > Harman Kalra <hkalra@marvell.com> wrote:
> > > +	/* Detect if DPDK malloc APIs are ready to be used. */
> > > +	mem_allocator = rte_malloc_is_ready();
> > > +	if (mem_allocator)
> > > +		intr_handle = rte_zmalloc(NULL, sizeof(struct
> > rte_intr_handle),
> > > +					  0);
> > > +	else
> > > +		intr_handle = calloc(1, sizeof(struct rte_intr_handle));
> > 
> > This is problematic way to do this.
> > The reason to use rte_malloc vs malloc should be determined by usage.
> > 
> > If the pointer will be shared between primary/secondary process then it has
> > to be in hugepages (ie rte_malloc). If it is not shared then then use regular
> > malloc.
> > 
> > But what you have done is created a method which will be a latent bug for
> > anyone using primary/secondary process.
> > 
> > Either:
> >     intr_handle is not allowed to be used in secondary.
> >       Then always use malloc().
> > Or.
> >     intr_handle can be used by both primary and secondary.
> >     Then always use rte_malloc().
> >     Any code path that allocates intr_handle before pool is
> >     ready is broken.
> 
> Hi Stephan,
> 
> Till V2, I implemented this API in a way where user of the API can choose
> If he wants intr handle to be allocated using malloc or rte_malloc by passing
> a flag arg to the rte_intr_instanc_alloc API. User of the API will best know if
> the intr handle is to be shared with secondary or not.

Yes the caller should know, but it makes usage more difficult.
Using rte_malloc always is simpler.
 
> But after some discussions and suggestions from the community we decided
> to drop that flag argument and auto detect on whether rte_malloc APIs are
> ready to be used and thereafter make all further allocations via rte_malloc.
> Currently alarm subsystem (or any driver doing allocation in constructor) gets
> interrupt instance allocated using glibc malloc that too because rte_malloc*
> is not ready by rte_eal_alarm_init(), while all further consumers gets instance
> allocated via rte_malloc.

Yes the general case is to allocate after rte_malloc is ready.
Anyway a constructor should not allocate complicate things.

>  I think this should not cause any issue in primary/secondary model as all interrupt
> instance pointer will be shared. Infact to avoid any surprises of primary/secondary
> not working we thought of making all allocations via rte_malloc.

Yes

> David, Thomas, Dmitry, please add if I missed anything.

I understand Stephen's concern but I think this choice is a good compromise.
Ideally we should avoid doing real stuff in constructors.

> Can we please conclude on this series APIs as API freeze deadline (rc1) is very near.

I vote for keeping this design.