[v5,3/5] eal/interrupts: add IRQ count in interrupt handle

  Have total number of IRQ count support in interrupt handle.
In case of VFIO this IRQ count is returned when
VFIO_DEVICE_GET_IRQ_INFO ioctl is invoked. This IRQ_count can
used by the devices to store/provide total number of interrupts
available and to enable or disable these interrupts.

Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
---
 lib/eal/common/eal_common_interrupts.c | 21 +++++++++++++++++
 lib/eal/common/eal_interrupts.h        |  1 +
 lib/eal/include/rte_interrupts.h       | 32 ++++++++++++++++++++++++++
 lib/eal/version.map                    |  2 ++
 4 files changed, 56 insertions(+)
  

On Thu, May 25, 2023 at 12:08 PM Nipun Gupta <nipun.gupta@amd.com> wrote:
>
> Have total number of IRQ count support in interrupt handle.
> In case of VFIO this IRQ count is returned when
> VFIO_DEVICE_GET_IRQ_INFO ioctl is invoked. This IRQ_count can
> used by the devices to store/provide total number of interrupts
> available and to enable or disable these interrupts.
>
> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> ---
>  lib/eal/common/eal_common_interrupts.c | 21 +++++++++++++++++
>  lib/eal/common/eal_interrupts.h        |  1 +
>  lib/eal/include/rte_interrupts.h       | 32 ++++++++++++++++++++++++++
>  lib/eal/version.map                    |  2 ++
>  4 files changed, 56 insertions(+)
>
> diff --git a/lib/eal/common/eal_common_interrupts.c b/lib/eal/common/eal_common_interrupts.c
> index 97b64fed58..a0167d9ad4 100644
> --- a/lib/eal/common/eal_common_interrupts.c
> +++ b/lib/eal/common/eal_common_interrupts.c
> @@ -398,6 +398,27 @@ int rte_intr_elist_index_set(struct rte_intr_handle *intr_handle,
>         return -rte_errno;
>  }
>
> +int rte_intr_irq_count_set(struct rte_intr_handle *intr_handle,
> +       int irq_count)
> +{
> +       CHECK_VALID_INTR_HANDLE(intr_handle);
> +
> +       intr_handle->irq_count = irq_count;
> +
> +       return 0;
> +fail:
> +       return -rte_errno;
> +}
> +
> +int rte_intr_irq_count_get(const struct rte_intr_handle *intr_handle)
> +{
> +       CHECK_VALID_INTR_HANDLE(intr_handle);
> +
> +       return intr_handle->irq_count;
> +fail:
> +       return -rte_errno;
> +}
> +
>  int rte_intr_vec_list_alloc(struct rte_intr_handle *intr_handle,
>         const char *name, int size)
>  {
> diff --git a/lib/eal/common/eal_interrupts.h b/lib/eal/common/eal_interrupts.h
> index 482781b862..eaf8e20187 100644
> --- a/lib/eal/common/eal_interrupts.h
> +++ b/lib/eal/common/eal_interrupts.h
> @@ -16,6 +16,7 @@ struct rte_intr_handle {
>         };
>         uint32_t alloc_flags;   /**< flags passed at allocation */
>         enum rte_intr_handle_type type;  /**< handle type */
> +       uint32_t irq_count;             /**< Total IRQ count */
>         uint32_t max_intr;             /**< max interrupt requested */
>         uint32_t nb_efd;               /**< number of available efd(event fd) */
>         uint8_t efd_counter_size;      /**< size of efd counter, used for vdev */
> diff --git a/lib/eal/include/rte_interrupts.h b/lib/eal/include/rte_interrupts.h
> index 487e3c8875..415d1fcac0 100644
> --- a/lib/eal/include/rte_interrupts.h
> +++ b/lib/eal/include/rte_interrupts.h
> @@ -506,6 +506,38 @@ __rte_internal
>  int
>  rte_intr_max_intr_get(const struct rte_intr_handle *intr_handle);
>
> +/**
> + * @internal
> + * Set the IRQ count field of interrupt handle with user
> + * provided IRQ count value.

I am intrigued by this new notion.
We already have different sizes in the intr_handle, why do we need a new one?

Plus, in the cdx patch using this new API, I see that an fd array is
filled based on nb_efd.
So it seems to me that this new irq_count is just a duplicate of nb_efd.


> + * @param intr_handle
> + *  pointer to the interrupt handle.
> + * @param irq_count
> + *  IRQ count
> + *
> + * @return
> + *  - On success, zero.
> + *  - On failure, a negative value and rte_errno is set.
> + */
> +__rte_internal
> +int
> +rte_intr_irq_count_set(struct rte_intr_handle *intr_handle, int irq_count);
> +
  

Hi David,

On 6/1/2023 8:55 PM, David Marchand wrote:
> 
> On Thu, May 25, 2023 at 12:08 PM Nipun Gupta <nipun.gupta@amd.com> wrote:
>>
>> Have total number of IRQ count support in interrupt handle.
>> In case of VFIO this IRQ count is returned when
>> VFIO_DEVICE_GET_IRQ_INFO ioctl is invoked. This IRQ_count can
>> used by the devices to store/provide total number of interrupts
>> available and to enable or disable these interrupts.
>>
>> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
>> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
>> ---
>>   lib/eal/common/eal_common_interrupts.c | 21 +++++++++++++++++
>>   lib/eal/common/eal_interrupts.h        |  1 +
>>   lib/eal/include/rte_interrupts.h       | 32 ++++++++++++++++++++++++++
>>   lib/eal/version.map                    |  2 ++
>>   4 files changed, 56 insertions(+)
>>
>> diff --git a/lib/eal/common/eal_common_interrupts.c b/lib/eal/common/eal_common_interrupts.c
>> index 97b64fed58..a0167d9ad4 100644
>> --- a/lib/eal/common/eal_common_interrupts.c
>> +++ b/lib/eal/common/eal_common_interrupts.c
>> @@ -398,6 +398,27 @@ int rte_intr_elist_index_set(struct rte_intr_handle *intr_handle,
>>          return -rte_errno;
>>   }
>>
>> +int rte_intr_irq_count_set(struct rte_intr_handle *intr_handle,
>> +       int irq_count)
>> +{
>> +       CHECK_VALID_INTR_HANDLE(intr_handle);
>> +
>> +       intr_handle->irq_count = irq_count;
>> +
>> +       return 0;
>> +fail:
>> +       return -rte_errno;
>> +}
>> +
>> +int rte_intr_irq_count_get(const struct rte_intr_handle *intr_handle)
>> +{
>> +       CHECK_VALID_INTR_HANDLE(intr_handle);
>> +
>> +       return intr_handle->irq_count;
>> +fail:
>> +       return -rte_errno;
>> +}
>> +
>>   int rte_intr_vec_list_alloc(struct rte_intr_handle *intr_handle,
>>          const char *name, int size)
>>   {
>> diff --git a/lib/eal/common/eal_interrupts.h b/lib/eal/common/eal_interrupts.h
>> index 482781b862..eaf8e20187 100644
>> --- a/lib/eal/common/eal_interrupts.h
>> +++ b/lib/eal/common/eal_interrupts.h
>> @@ -16,6 +16,7 @@ struct rte_intr_handle {
>>          };
>>          uint32_t alloc_flags;   /**< flags passed at allocation */
>>          enum rte_intr_handle_type type;  /**< handle type */
>> +       uint32_t irq_count;             /**< Total IRQ count */
>>          uint32_t max_intr;             /**< max interrupt requested */
>>          uint32_t nb_efd;               /**< number of available efd(event fd) */
>>          uint8_t efd_counter_size;      /**< size of efd counter, used for vdev */
>> diff --git a/lib/eal/include/rte_interrupts.h b/lib/eal/include/rte_interrupts.h
>> index 487e3c8875..415d1fcac0 100644
>> --- a/lib/eal/include/rte_interrupts.h
>> +++ b/lib/eal/include/rte_interrupts.h
>> @@ -506,6 +506,38 @@ __rte_internal
>>   int
>>   rte_intr_max_intr_get(const struct rte_intr_handle *intr_handle);
>>
>> +/**
>> + * @internal
>> + * Set the IRQ count field of interrupt handle with user
>> + * provided IRQ count value.
> 
> I am intrigued by this new notion.
> We already have different sizes in the intr_handle, why do we need a new one?
> 
> Plus, in the cdx patch using this new API, I see that an fd array is
> filled based on nb_efd.
> So it seems to me that this new irq_count is just a duplicate of nb_efd.

The API rte_intr_efd_enable() sets the nb_efd and max_intr to values 
provided by the caller (+ NB_OTHER_INTR for max_intr). These values are 
dependent on the number of interrupts which are enabled by any DPDK 
driver rather than the actual number of interrupts supported or provided 
by the Linux driver.

With CDX bus the devices can have interrupts which are not really bound 
by the number of queues, and as these devices are programmable (FPGA 
based), users are free to implement the interrupts as per the need. I 
need to provide the total number of interrupts supported by the device 
to the drivers.

nb_intr is also there, which is default initialized by 
RTE_MAX_RXTX_INTR_VEC_ID, due to which this does not seem to be best 
fit. Though as per the meaning it shall represent the total number of 
interrupts supported by the device. Maybe we can remove the default 
value and use this as a total interrupt count supported for the device?

w.r.t. comments on the other patches, I will fix them in the next respin.

Regards,
Nipun

> 
> 
>> + * @param intr_handle
>> + *  pointer to the interrupt handle.
>> + * @param irq_count
>> + *  IRQ count
>> + *
>> + * @return
>> + *  - On success, zero.
>> + *  - On failure, a negative value and rte_errno is set.
>> + */
>> +__rte_internal
>> +int
>> +rte_intr_irq_count_set(struct rte_intr_handle *intr_handle, int irq_count);
>> +
> 
> 
> --
> David Marchand
>
  

> -----Original Message-----
> From: Gupta, Nipun
> Sent: Thursday, June 1, 2023 11:35 PM
> To: David Marchand <david.marchand@redhat.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; hkalra@marvell.com;
> anatoly.burakov@intel.com; stephen@networkplumber.org; Yigit, Ferruh
> <Ferruh.Yigit@amd.com>; Anand, Harpreet <harpreet.anand@amd.com>;
> Agarwal, Nikhil <nikhil.agarwal@amd.com>
> Subject: Re: [PATCH v5 3/5] eal/interrupts: add IRQ count in interrupt handle
> 
> Hi David,
> 
> On 6/1/2023 8:55 PM, David Marchand wrote:
> >
> > On Thu, May 25, 2023 at 12:08 PM Nipun Gupta <nipun.gupta@amd.com>
> wrote:
> >>
> >> Have total number of IRQ count support in interrupt handle.
> >> In case of VFIO this IRQ count is returned when
> >> VFIO_DEVICE_GET_IRQ_INFO ioctl is invoked. This IRQ_count can
> >> used by the devices to store/provide total number of interrupts
> >> available and to enable or disable these interrupts.
> >>
> >> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> >> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> >> ---
> >>   lib/eal/common/eal_common_interrupts.c | 21 +++++++++++++++++
> >>   lib/eal/common/eal_interrupts.h        |  1 +
> >>   lib/eal/include/rte_interrupts.h       | 32 ++++++++++++++++++++++++++
> >>   lib/eal/version.map                    |  2 ++
> >>   4 files changed, 56 insertions(+)
> >>
> >> diff --git a/lib/eal/common/eal_common_interrupts.c
> b/lib/eal/common/eal_common_interrupts.c
> >> index 97b64fed58..a0167d9ad4 100644
> >> --- a/lib/eal/common/eal_common_interrupts.c
> >> +++ b/lib/eal/common/eal_common_interrupts.c
> >> @@ -398,6 +398,27 @@ int rte_intr_elist_index_set(struct rte_intr_handle
> *intr_handle,
> >>          return -rte_errno;
> >>   }
> >>
> >> +int rte_intr_irq_count_set(struct rte_intr_handle *intr_handle,
> >> +       int irq_count)
> >> +{
> >> +       CHECK_VALID_INTR_HANDLE(intr_handle);
> >> +
> >> +       intr_handle->irq_count = irq_count;
> >> +
> >> +       return 0;
> >> +fail:
> >> +       return -rte_errno;
> >> +}
> >> +
> >> +int rte_intr_irq_count_get(const struct rte_intr_handle *intr_handle)
> >> +{
> >> +       CHECK_VALID_INTR_HANDLE(intr_handle);
> >> +
> >> +       return intr_handle->irq_count;
> >> +fail:
> >> +       return -rte_errno;
> >> +}
> >> +
> >>   int rte_intr_vec_list_alloc(struct rte_intr_handle *intr_handle,
> >>          const char *name, int size)
> >>   {
> >> diff --git a/lib/eal/common/eal_interrupts.h
> b/lib/eal/common/eal_interrupts.h
> >> index 482781b862..eaf8e20187 100644
> >> --- a/lib/eal/common/eal_interrupts.h
> >> +++ b/lib/eal/common/eal_interrupts.h
> >> @@ -16,6 +16,7 @@ struct rte_intr_handle {
> >>          };
> >>          uint32_t alloc_flags;   /**< flags passed at allocation */
> >>          enum rte_intr_handle_type type;  /**< handle type */
> >> +       uint32_t irq_count;             /**< Total IRQ count */
> >>          uint32_t max_intr;             /**< max interrupt requested */
> >>          uint32_t nb_efd;               /**< number of available efd(event fd) */
> >>          uint8_t efd_counter_size;      /**< size of efd counter, used for vdev */
> >> diff --git a/lib/eal/include/rte_interrupts.h b/lib/eal/include/rte_interrupts.h
> >> index 487e3c8875..415d1fcac0 100644
> >> --- a/lib/eal/include/rte_interrupts.h
> >> +++ b/lib/eal/include/rte_interrupts.h
> >> @@ -506,6 +506,38 @@ __rte_internal
> >>   int
> >>   rte_intr_max_intr_get(const struct rte_intr_handle *intr_handle);
> >>
> >> +/**
> >> + * @internal
> >> + * Set the IRQ count field of interrupt handle with user
> >> + * provided IRQ count value.
> >
> > I am intrigued by this new notion.
> > We already have different sizes in the intr_handle, why do we need a new one?
> >
> > Plus, in the cdx patch using this new API, I see that an fd array is
> > filled based on nb_efd.
> > So it seems to me that this new irq_count is just a duplicate of nb_efd.
> 
> The API rte_intr_efd_enable() sets the nb_efd and max_intr to values
> provided by the caller (+ NB_OTHER_INTR for max_intr). These values are
> dependent on the number of interrupts which are enabled by any DPDK
> driver rather than the actual number of interrupts supported or provided
> by the Linux driver.
> 
> With CDX bus the devices can have interrupts which are not really bound
> by the number of queues, and as these devices are programmable (FPGA
> based), users are free to implement the interrupts as per the need. I
> need to provide the total number of interrupts supported by the device
> to the drivers.
> 
> nb_intr is also there, which is default initialized by
> RTE_MAX_RXTX_INTR_VEC_ID, due to which this does not seem to be best
> fit. Though as per the meaning it shall represent the total number of
> interrupts supported by the device. Maybe we can remove the default
> value and use this as a total interrupt count supported for the device?

On more look nb_intr seems to be more aligned towards the number of efd's
allocated, rather than the total number of interrupts supported on the device.
So, it should have a default value, but yes then I am not sure it this can be used
to represent total number of interrupt count. Please let me know if your views
on this.

Regards,
Nipun
  

> -----Original Message-----
> From: Nipun Gupta <nipun.gupta@amd.com>
> Sent: Thursday, May 25, 2023 3:38 PM
> To: dev@dpdk.org; thomas@monjalon.net; david.marchand@redhat.com;
> Harman Kalra <hkalra@marvell.com>; anatoly.burakov@intel.com;
> stephen@networkplumber.org
> Cc: ferruh.yigit@amd.com; harpreet.anand@amd.com;
> nikhil.agarwal@amd.com; Nipun Gupta <nipun.gupta@amd.com>
> Subject: [EXT] [PATCH v5 3/5] eal/interrupts: add IRQ count in interrupt
> handle
> 
> External Email
> 
> ----------------------------------------------------------------------
> Have total number of IRQ count support in interrupt handle.
> In case of VFIO this IRQ count is returned when
> VFIO_DEVICE_GET_IRQ_INFO ioctl is invoked. This IRQ_count can used by
> the devices to store/provide total number of interrupts available and to
> enable or disable these interrupts.
> 

Hi Nipun,

We already have "max_intr" field for the same purpose and its respective APIs
plt_intr_max_intr_set()/plt_intr_max_intr_get()

Thanks
Harman
  

On 6/6/2023 12:48 PM, Harman Kalra wrote:
> 
>> -----Original Message-----
>> From: Nipun Gupta <nipun.gupta@amd.com>
>> Sent: Thursday, May 25, 2023 3:38 PM
>> To: dev@dpdk.org; thomas@monjalon.net; david.marchand@redhat.com;
>> Harman Kalra <hkalra@marvell.com>; anatoly.burakov@intel.com;
>> stephen@networkplumber.org
>> Cc: ferruh.yigit@amd.com; harpreet.anand@amd.com;
>> nikhil.agarwal@amd.com; Nipun Gupta <nipun.gupta@amd.com>
>> Subject: [EXT] [PATCH v5 3/5] eal/interrupts: add IRQ count in interrupt
>> handle
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> Have total number of IRQ count support in interrupt handle.
>> In case of VFIO this IRQ count is returned when
>> VFIO_DEVICE_GET_IRQ_INFO ioctl is invoked. This IRQ_count can used by
>> the devices to store/provide total number of interrupts available and to
>> enable or disable these interrupts.
>>
> 
> Hi Nipun,
> 
> We already have "max_intr" field for the same purpose and its respective APIs
> plt_intr_max_intr_set()/plt_intr_max_intr_get()

Hi Harman,

If we have a look into rte_intr_efd_enable() API, 'max_intr' being set 
in this API. So once a driver is using the interrupts the 'max_intr' 
would be overwritten. 'nb_intr' which is described as "Max vector count" 
seems more relevant to me here and I have used 'nb_intr' to have the 
total interrupt count available and sent out the updated series for CDX 
bus. Please let me know in case you have separate thoughts on this.

Thanks,
Nipun

> 
> Thanks
> Harman
  

> -----Original Message-----
> From: Nipun Gupta <nipun.gupta@amd.com>
> Sent: Tuesday, June 6, 2023 12:57 PM
> To: Harman Kalra <hkalra@marvell.com>; dev@dpdk.org;
> thomas@monjalon.net; david.marchand@redhat.com;
> anatoly.burakov@intel.com; stephen@networkplumber.org
> Cc: ferruh.yigit@amd.com; harpreet.anand@amd.com;
> nikhil.agarwal@amd.com
> Subject: Re: [EXT] [PATCH v5 3/5] eal/interrupts: add IRQ count in interrupt
> handle
> 
> 
> 
> On 6/6/2023 12:48 PM, Harman Kalra wrote:
> >
> >> -----Original Message-----
> >> From: Nipun Gupta <nipun.gupta@amd.com>
> >> Sent: Thursday, May 25, 2023 3:38 PM
> >> To: dev@dpdk.org; thomas@monjalon.net;
> david.marchand@redhat.com;
> >> Harman Kalra <hkalra@marvell.com>; anatoly.burakov@intel.com;
> >> stephen@networkplumber.org
> >> Cc: ferruh.yigit@amd.com; harpreet.anand@amd.com;
> >> nikhil.agarwal@amd.com; Nipun Gupta <nipun.gupta@amd.com>
> >> Subject: [EXT] [PATCH v5 3/5] eal/interrupts: add IRQ count in
> >> interrupt handle
> >>
> >> External Email
> >>
> >> ---------------------------------------------------------------------
> >> - Have total number of IRQ count support in interrupt handle.
> >> In case of VFIO this IRQ count is returned when
> >> VFIO_DEVICE_GET_IRQ_INFO ioctl is invoked. This IRQ_count can used by
> >> the devices to store/provide total number of interrupts available and
> >> to enable or disable these interrupts.
> >>
> >
> > Hi Nipun,
> >
> > We already have "max_intr" field for the same purpose and its
> > respective APIs
> > plt_intr_max_intr_set()/plt_intr_max_intr_get()
> 
> Hi Harman,
> 
> If we have a look into rte_intr_efd_enable() API, 'max_intr' being set in this
> API. So once a driver is using the interrupts the 'max_intr'
> would be overwritten. 'nb_intr' which is described as "Max vector count"
> seems more relevant to me here and I have used 'nb_intr' to have the total
> interrupt count available and sent out the updated series for CDX bus. Please
> let me know in case you have separate thoughts on this.
> 

Hi Nipun,

Got your point, we are aligned on this.
Just to bring to your notice, In pci_vfio_setup_interrupts(), if irq.count > nb_intr 
we are updating nb_intr as part of rte_intr_event_list_update()

Thanks	
Harman



> Thanks,
> Nipun
> 
> >
> > Thanks
> > Harman