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

[Harman Kalra <hkalra@marvell.com>]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, October 19, 2021 4:27 AM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Ray Kinsella
> <mdr@ashroe.eu>; david.marchand@redhat.com;
> dmitry.kozliuk@gmail.com
> Subject: [EXT] Re: [dpdk-dev] [PATCH v3 2/7] eal/interrupts: implement get
> set APIs
> 
> External Email
> 
> ----------------------------------------------------------------------
> 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.

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.
 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. 

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

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

Thanks
Harman