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

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

2021-10-14 09:31 (UTC+0000), Harman Kalra:
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Thursday, October 14, 2021 1:53 PM
> > To: Harman Kalra <hkalra@marvell.com>
> > Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Ray Kinsella
> > <mdr@ashroe.eu>; Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>; David
> > Marchand <david.marchand@redhat.com>; viacheslavo@nvidia.com;
> > matan@nvidia.com
> > Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v1 2/7] eal/interrupts: implement
> > get set APIs
> > 
> > 13/10/2021 20:52, Thomas Monjalon:  
> > > 13/10/2021 19:57, Harman Kalra:  
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra  
> > > > > From: Thomas Monjalon <thomas@monjalon.net>  
> > > > > > 04/10/2021 11:57, David Marchand:  
> > > > > > > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra
> > > > > > > <hkalra@marvell.com>  
> > > > > > wrote:  
> > > > > > > > > > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int  
> > size,  
> > > > > > > > > > +
> > > > > > > > > > +bool
> > > > > > > > > > +from_hugepage) {
> > > > > > > > > > +       struct rte_intr_handle *intr_handle;
> > > > > > > > > > +       int i;
> > > > > > > > > > +
> > > > > > > > > > +       if (from_hugepage)
> > > > > > > > > > +               intr_handle = rte_zmalloc(NULL,
> > > > > > > > > > +                                         size * sizeof(struct rte_intr_handle),
> > > > > > > > > > +                                         0);
> > > > > > > > > > +       else
> > > > > > > > > > +               intr_handle = calloc(1, size *
> > > > > > > > > > + sizeof(struct rte_intr_handle));  
> > > > > > > > >
> > > > > > > > > We can call DPDK allocator in all cases.
> > > > > > > > > That would avoid headaches on why multiprocess does not
> > > > > > > > > work in some rarely tested cases.  
> > [...]  
> > > > > > I agree with David.
> > > > > > I prefer a simpler API which always use rte_malloc, and make
> > > > > > sure interrupts are always handled between rte_eal_init and  
> > rte_eal_cleanup.
> > [...]  
> > > > > There are couple of more dependencies on glibc heap APIs:
> > > > > 1. "rte_eal_alarm_init()" allocates an interrupt instance which is
> > > > > used for timerfd, is called before "rte_eal_memory_init()" which
> > > > > does the memseg init.
> > > > > Not sure what all challenges we may face in moving alarm_init
> > > > > after memory_init as it might break some subsystem inits.
> > > > > Other option could be to allocate interrupt instance for timerfd
> > > > > on first alarm_setup call.  
> > >
> > > Indeed it is an issue.
> > >
> > > [...]
> > >  
> > > > > There are many other drivers which statically declares the
> > > > > interrupt handles inside their respective private structures and
> > > > > memory for those structure was allocated from heap. For such
> > > > > drivers I allocated interrupt instances also using glibc heap APIs.  
> > >
> > > Could you use rte_malloc in these drivers?  
> > 
> > If we take the direction of 2 different allocations mode for the interrupts, I
> > suggest we make it automatic without any API parameter.
> > We don't have any function to check rte_malloc readiness I think.
> > But we can detect whether shared memory is ready with this check:
> > rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC This check
> > is true at the end of rte_eal_init, so it is false during probing.
> > Would it be enough? Or should we implement rte_malloc_is_ready()?  
> 
> Hi Thomas,
> 
> It's a very good suggestion. Let's implement "rte_malloc_is_ready()" which could be as
> simple as " rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC" check.
> There may be more consumers for this API in future.

I doubt it should be public. How it is supposed to be used? 
Any application code for DPDK necessarily calls rte_eal_init() first,
after that this function would always return true.

> 
> If we are making it automatic detection, shall we now even have argument to this alloc API?
> I added a flags argument (32 bit) in latest series where each bit of this flag can be an allocation capability.
> I used two bits for discriminating between glibc malloc and rte_malloc. Shall we keep it or drop it?
> 
> David, Dmitry please share your thoughts.

I'd drop it, but no strong opinion.
Since allocation type is automatic and all other properties can be set later,
there are no use cases for any options here.
And if they appear, flags may be insufficient as well.