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

[Harman Kalra <hkalra@marvell.com>]

> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Sent: Monday, October 4, 2021 4:48 PM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: dev@dpdk.org; Ray Kinsella <mdr@ashroe.eu>; David Marchand
> <david.marchand@redhat.com>
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v1 2/7] eal/interrupts: implement
> get set APIs
> 
> 2021-10-04 10:37 (UTC+0000), Harman Kalra:
> > [...]
> > > > +struct rte_intr_handle *rte_intr_handle_instance_index_get(
> > > > +				struct rte_intr_handle *intr_handle, int
> > > index)
> > >
> > > If rte_intr_handle_instance_alloc() returns a pointer to an array,
> > > this function is useless since the user can simply manipulate a pointer.
> >
> > <HK> User wont be able to manipulate the pointer as he is not aware of
> size of struct rte_intr_handle.
> > He will observe "dereferencing pointer to incomplete type" compilation
> error.
> 
> Sorry, my bad.
> 
> > > If we want to make a distinction between a single struct
> > > rte_intr_handle and a commonly allocated bunch of such (but why?),
> > > then they should be represented by distinct types.
> >
> > <HK> Do you mean, we should have separate APIs for single allocation
> > and batch allocation? As get API will be useful only in case of batch
> > allocation. Currently interrupt autotests and ifpga_rawdev driver makes
> batch allocation.
> > I think common API for single and batch is fine, get API is required for
> returning a particular intr_handle instance.
> > But one problem I see in current implementation is there should be
> > upper limit check for index in get/set API, which I will fix.
> 
> I don't think we need different APIs, I was asking if it was your intention.
> Now I understand it and agree with you.
> 
> > > > +int rte_intr_handle_instance_index_set(struct rte_intr_handle
> > > *intr_handle,
> > > > +				       const struct rte_intr_handle *src,
> > > > +				       int index)
> > >
> > > See above regarding the "index" parameter. If it can be removed, a
> > > better name for this function would be rte_intr_handle_copy().
> >
> > <HK> I think get API is required.
> 
> Maybe index is still not needed: "intr_handle" can just be a pointer to the
> right item obtained with rte_intr_handle_instance_index_get(). This way you
> also don't need to duplicate the index-checking logic.

In the current implementation, batch allocation of interrupt handle may lead to mem leak while
freeing efds and elist array. I am only freeing efds/elist for intr_handle[0] in rte_intr_handle_instance_free().
To free efds/elist of all the intr_handles[], either I should cache the size parameter passed during alloc. But
where should I store it? In first instance of struct rte_intr_handle. I don't think it will be a good idea.

Since batch allocation is only done in test suite and ifpga_rawdev.c, to keep things simpler let's restrict
rte_intr_handle_instance_alloc() to single instance allocation and user can call this API in a loop and
maintain array of handles locally. 
With this approach get_index API is not required and set_index API can be renamed to rte_intr_handle_copy()

Thoughts? 

Thanks
Harman