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

[Dmitry Kozlyuk]

2021-10-19 08:32 (UTC+0000), Harman Kalra:
> > -----Original Message-----
> > From: Stephen Hemminger <stephen at networkplumber.org>
> > Sent: Tuesday, October 19, 2021 4:27 AM
> > To: Harman Kalra <hkalra at marvell.com>
> > Cc: dev at dpdk.org; Thomas Monjalon <thomas at monjalon.net>; Ray Kinsella
> > <mdr at ashroe.eu>; david.marchand at redhat.com;
> > dmitry.kozliuk at 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 at 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.

Just as a comment, bus scanning is the real issue, not the alarms.
Alarms could be initialized after the memory management
(but it's irrelevant because their handle is not accessed from the outside).
However, MM needs to know bus IOVA requirements to initialize,
which is usually determined by at least bus device requirements.

>  I think this should not cause any issue in primary/secondary model as all interrupt
> instance pointer will be shared.

What do you mean? Aren't we discussing the issue
that those allocated early are not shared?

> Infact to avoid any surprises of primary/secondary
> not working we thought of making all allocations via rte_malloc. 

I don't see why anyone would not make them shared.
In order to only use rte_malloc(), we need:
1. In bus drivers, move handle allocation from scan to probe stage.
2. In EAL, move alarm initialization to after the MM.
It all can be done later with v3 design---but there are out-of-tree drivers.
We need to force them to make step 1 at some point.
I see two options:
a) Right now have an external API that only works with rte_malloc()
   and internal API with autodetection. Fix DPDK and drop internal API.
b) Have external API with autodetection. Fix DPDK.
   At the next ABI breakage drop autodetection and libc-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.

I support v3 design with no options and autodetection,
because that's the interface we want in the end.
Implementation can be improved later.