[1/2] eal: make max interrupt vector id configurable

  From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Make RTE_MAX_RXTX_INTR_VEC_ID configurable as MSI-X support a
maximum of 2048 vectors.
The default value is unchanged and set to 512.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 config/meson.build                          | 1 +
 lib/librte_eal/include/rte_eal_interrupts.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
  

On Fri, Feb 26, 2021 at 12:31 AM <pbhagavatula@marvell.com> wrote:
>
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Make RTE_MAX_RXTX_INTR_VEC_ID configurable as MSI-X support a
> maximum of 2048 vectors.
> The default value is unchanged and set to 512.


IMO, We dont need to make it configurable and each platform sets its
value. That scheme won't work as generic distribution build will fail
to run.
Since PCIe specification defines this value and there is no
performance impact on increasing this,
IMO, We can change to 2048 as default.

Thought from others?

>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
>  config/meson.build                          | 1 +
>  lib/librte_eal/include/rte_eal_interrupts.h | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/config/meson.build b/config/meson.build
> index 3cf560b8a..0fe4d0689 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -247,6 +247,7 @@ dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet'))
>  dpdk_conf.set('RTE_ENABLE_TRACE_FP', get_option('enable_trace_fp'))
>  # values which have defaults which may be overridden
>  dpdk_conf.set('RTE_MAX_VFIO_GROUPS', 64)
> +dpdk_conf.set('RTE_MAX_RXTX_INTR_VEC_ID', 512)
>  dpdk_conf.set('RTE_DRIVER_MEMPOOL_BUCKET_SIZE_KB', 64)
>  dpdk_conf.set('RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', true)
>  if dpdk_conf.get('RTE_ARCH_64')
> diff --git a/lib/librte_eal/include/rte_eal_interrupts.h b/lib/librte_eal/include/rte_eal_interrupts.h
> index 00bcc19b6..e9af1a4c2 100644
> --- a/lib/librte_eal/include/rte_eal_interrupts.h
> +++ b/lib/librte_eal/include/rte_eal_interrupts.h
> @@ -17,7 +17,6 @@
>  #ifndef _RTE_EAL_INTERRUPTS_H_
>  #define _RTE_EAL_INTERRUPTS_H_
>
> -#define RTE_MAX_RXTX_INTR_VEC_ID      512
>  #define RTE_INTR_VEC_ZERO_OFFSET      0
>  #define RTE_INTR_VEC_RXTX_OFFSET      1
>
> --
> 2.17.1
>
  

On Wed, Mar 24, 2021 at 12:01 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Fri, Feb 26, 2021 at 12:31 AM <pbhagavatula@marvell.com> wrote:
> >
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >
> > Make RTE_MAX_RXTX_INTR_VEC_ID configurable as MSI-X support a
> > maximum of 2048 vectors.
> > The default value is unchanged and set to 512.
>
>
> IMO, We dont need to make it configurable and each platform sets its
> value. That scheme won't work as generic distribution build will fail
> to run.
> Since PCIe specification defines this value and there is no
> performance impact on increasing this,
> IMO, We can change to 2048 as default.

It probably breaks rte_intr_* ABI.

struct rte_intr_handle {
...
        int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
        struct rte_epoll_event elist[RTE_MAX_RXTX_INTR_VEC_ID];
                                       /**< intr vector epoll event */
...


I see you need this for octeontx2, so wondering if you could handle
this differently in octeontx2 drivers?
  

On Wed, Mar 24, 2021 at 4:45 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Wed, Mar 24, 2021 at 12:01 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Fri, Feb 26, 2021 at 12:31 AM <pbhagavatula@marvell.com> wrote:
> > >
> > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > >
> > > Make RTE_MAX_RXTX_INTR_VEC_ID configurable as MSI-X support a
> > > maximum of 2048 vectors.
> > > The default value is unchanged and set to 512.
> >
> >
> > IMO, We dont need to make it configurable and each platform sets its
> > value. That scheme won't work as generic distribution build will fail
> > to run.
> > Since PCIe specification defines this value and there is no
> > performance impact on increasing this,
> > IMO, We can change to 2048 as default.
>
> It probably breaks rte_intr_* ABI.

Yes. Even though all APIs are used as a pointer (ie. "struct
rte_intr_handle *"), the definition
kept in the header file.


> struct rte_intr_handle {
> ...
>         int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
>         struct rte_epoll_event elist[RTE_MAX_RXTX_INTR_VEC_ID];
>                                        /**< intr vector epoll event */
> ...
>
>
> I see you need this for octeontx2, so wondering if you could handle
> this differently in octeontx2 drivers?

This is an issue with any PCIe device that has more than 512 MSIX interrupts.

The PCI spec the max is defined as 2K.

CN10K drivers have 1K interrupt lines per PCIe device.

I think, following are the options.
1) To avoid ABI breakage  in default configuration use the existing patch
2) In 21.11 break ABI and Either change to
a) RTE_MAX_RXTX_INTR_VEC_ID as 1024
or
b) Make it full dynamic allocation based on PCI device MSIX size on probe time.
That brings some kind of dependency rte_intr with PCI device. Need to
understand,
How it can clearly be abstracted out and Is it worth trouble for the
amount of memory.
Looks like the cost of one entry is 40B. So additional 512 is 40B *
512 =  21KB virtual memory.


Thoughts?

>
>
> --
> David Marchand
>
  

On Wed, Mar 24, 2021 at 1:55 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > IMO, We dont need to make it configurable and each platform sets its
> > > value. That scheme won't work as generic distribution build will fail
> > > to run.
> > > Since PCIe specification defines this value and there is no
> > > performance impact on increasing this,
> > > IMO, We can change to 2048 as default.
> >
> > It probably breaks rte_intr_* ABI.
>
> Yes. Even though all APIs are used as a pointer (ie. "struct
> rte_intr_handle *"), the definition
> kept in the header file.
>
>
> > struct rte_intr_handle {
> > ...
> >         int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
> >         struct rte_epoll_event elist[RTE_MAX_RXTX_INTR_VEC_ID];
> >                                        /**< intr vector epoll event */
> > ...
> >
> >
> > I see you need this for octeontx2, so wondering if you could handle
> > this differently in octeontx2 drivers?
>
> This is an issue with any PCIe device that has more than 512 MSIX interrupts.
>
> The PCI spec the max is defined as 2K.
>
> CN10K drivers have 1K interrupt lines per PCIe device.
>
> I think, following are the options.
> 1) To avoid ABI breakage  in default configuration use the existing patch
> 2) In 21.11 break ABI and Either change to
> a) RTE_MAX_RXTX_INTR_VEC_ID as 1024
> or
> b) Make it full dynamic allocation based on PCI device MSIX size on probe time.
> That brings some kind of dependency rte_intr with PCI device. Need to
> understand,
> How it can clearly be abstracted out and Is it worth trouble for the
> amount of memory.
> Looks like the cost of one entry is 40B. So additional 512 is 40B *
> 512 =  21KB virtual memory.

Since you mentioned performance is not impacted, I guess this is
control path only.
And there is no need to expose this.
So:

c) Rework API so that we don't expose such details.
  

On Wed, Mar 24, 2021 at 8:56 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Wed, Mar 24, 2021 at 1:55 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > IMO, We dont need to make it configurable and each platform sets its
> > > > value. That scheme won't work as generic distribution build will fail
> > > > to run.
> > > > Since PCIe specification defines this value and there is no
> > > > performance impact on increasing this,
> > > > IMO, We can change to 2048 as default.
> > >
> > > It probably breaks rte_intr_* ABI.
> >
> > Yes. Even though all APIs are used as a pointer (ie. "struct
> > rte_intr_handle *"), the definition
> > kept in the header file.
> >
> >
> > > struct rte_intr_handle {
> > > ...
> > >         int efds[RTE_MAX_RXTX_INTR_VEC_ID];  /**< intr vectors/efds mapping */
> > >         struct rte_epoll_event elist[RTE_MAX_RXTX_INTR_VEC_ID];
> > >                                        /**< intr vector epoll event */
> > > ...
> > >
> > >
> > > I see you need this for octeontx2, so wondering if you could handle
> > > this differently in octeontx2 drivers?
> >
> > This is an issue with any PCIe device that has more than 512 MSIX interrupts.
> >
> > The PCI spec the max is defined as 2K.
> >
> > CN10K drivers have 1K interrupt lines per PCIe device.
> >
> > I think, following are the options.
> > 1) To avoid ABI breakage  in default configuration use the existing patch
> > 2) In 21.11 break ABI and Either change to
> > a) RTE_MAX_RXTX_INTR_VEC_ID as 1024
> > or
> > b) Make it full dynamic allocation based on PCI device MSIX size on probe time.
> > That brings some kind of dependency rte_intr with PCI device. Need to
> > understand,
> > How it can clearly be abstracted out and Is it worth trouble for the
> > amount of memory.
> > Looks like the cost of one entry is 40B. So additional 512 is 40B *
> > 512 =  21KB virtual memory.
>
> Since you mentioned performance is not impacted, I guess this is
> control path only.

Yes.

> And there is no need to expose this.
> So:
>
> c) Rework API so that we don't expose such details.

Yes. That's what I meant by option (b).("Make it fully dynamic
allocation based on PCI device MSIX size on probe time.")

>
>
> --
> David Marchand
>