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

[Harman Kalra <hkalra@marvell.com>]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra
> Sent: Wednesday, October 13, 2021 11:24 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; 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
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Tuesday, October 12, 2021 8:53 PM
> > To: Harman Kalra <hkalra@marvell.com>
> > Cc: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; 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
> >
> > 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.
> > > > > Wdyt?
> > > > >
> > > > > Plus "from_hugepage" is misleading, you could be in --no-huge
> > > > > mode, rte_zmalloc still works.
> > > >
> > > > <HK> In mellanox 5 driver interrupt handle instance is freed in
> > > > destructor " mlx5_pmd_interrupt_handler_uninstall()" while DPDK
> > > > memory allocators are already cleaned up in "rte_eal_cleanup".
> > > > Hence I allocated interrupt instances for such cases from normal heap.
> > > > There could be other such cases so I think its ok to keep this support.
> > >
> > > This is surprising.
> > > Why would the mlx5 driver wait to release in a destructor?
> > > It should be done once no interrupt handler is necessary (like when
> > > stopping all ports), and that would be before rte_eal_cleanup().
> >
> > 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.
> > The mlx5 PMD could be reworked to match this requirement.
> > In any case we should not any memory management in
> > constructors/destructors.
> 
> 
> Hi Thomas, David
> 
> 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.
> 
> 2. Currently interrupt handle field inside struct rte_pci_device is static which
> is changed to a pointer in this series(as struct rte_intr_handle is hidden inside
> a c file and size is unknown outside).
> I am allocating the memory for this interrupt instance inside
> "rte_pci_probe_one_driver()" just before "pci_vfio_map_resource()" which
> sets up vfio resources and calls " pci_vfio_setup_interrupts()" which setups
> the interrupt support. Here challenge is "rte_bus_probe()" also gets called
> before "rte_eal_memory_init()".


Sorry my bad, bus probing happens after "rte_eal_memory_init()", so second point
Is invalid.


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