> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozl...@gmail.com>
> Sent: Monday, October 4, 2021 4:48 PM
> To: Harman Kalra <hka...@marvell.com>
> Cc: dev@dpdk.org; Ray Kinsella <m...@ashroe.eu>; David Marchand
> <david.march...@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
