> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Thursday, October 14, 2021 4:06 PM
> To: Harman Kalra <hka...@marvell.com>
> Cc: David Marchand <david.march...@redhat.com>; dev@dpdk.org; Raslan
> Darawsheh <rasl...@nvidia.com>; Ray Kinsella <m...@ashroe.eu>; Dmitry
> Kozlyuk <dmitry.kozl...@gmail.com>; viachesl...@nvidia.com;
> ma...@nvidia.com
> Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v1 2/7] eal/interrupts: implement
> get set APIs
>
> 14/10/2021 12:31, Harman Kalra:
> > From: Thomas Monjalon <tho...@monjalon.net>
> > > 14/10/2021 11:31, Harman Kalra:
> > > > From: Thomas Monjalon <tho...@monjalon.net>
> > > > > 13/10/2021 20:52, Thomas Monjalon:
> > > > > > 13/10/2021 19:57, Harman Kalra:
> > > > > > > From: dev <dev-boun...@dpdk.org> On Behalf Of Harman Kalra
> > > > > > > > From: Thomas Monjalon <tho...@monjalon.net>
> > > > > > > > > 04/10/2021 11:57, David Marchand:
> > > > > > > > > > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra
> > > > > > > > > > <hka...@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.
> > >
> > > You cannot rely on the magic because it is set only after probing.
> > > For such API you need to have another internal flag to check that
> > > malloc is setup.
> >
> > Yeah, got that. You mean in case of bus probing although rte_malloc is
> > setup but eal_mcfg_complete() is calledt done yet. So we should set
> > another malloc specific flag at the end of rte_eal_memory_init(). Correct?
>
> I think the new internal flag should be at the end of
> rte_eal_malloc_heap_init().
> Then a rte_internal function rte_malloc_is_ready() should check this flag.
Sure.
>
> > But just for understanding, as David suggested that we preserve keep
> > this flag then why not use it, have rte_malloc and malloc bits and make a
> decision.
> > Let driver has the flexibility to choose. Do you see any harm in this?
>
> Which flag?
In V2, I have replaced the bool arg with an 32bit flag in alloc api:
struct rte_intr_handle *rte_intr_instance_alloc(uint32_t flags);
Declared some flags which can be passed by the consumer
/** Interrupt instance allocation flags
* @see rte_intr_instance_alloc
*/
/** Allocate interrupt instance from traditional heap */
#define RTE_INTR_ALLOC_TRAD_HEAP 0x00000000
/** Allocate interrupt instance using DPDK memory management APIs */
#define RTE_INTR_ALLOC_DPDK_ALLOCATOR 0x00000001
As a future enhancement, if more options to the allocation is required by user,
new flags can be added.
Thanks
Harman
>
>
