Re: [dpdk-dev] [PATCH v5 0/6] make rte_intr_handle internal

[David Marchand <david.marchand@redhat.com>]

On Mon, Oct 25, 2021 at 3:04 PM Raslan Darawsheh <rasland@nvidia.com> wrote:
>
> Hi,
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra
> > Sent: Friday, October 22, 2021 11:49 PM
> > To: dev@dpdk.org
> > Cc: david.marchand@redhat.com; dmitry.kozliuk@gmail.com;
> > mdr@ashroe.eu; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> > Harman Kalra <hkalra@marvell.com>
> > Subject: [dpdk-dev] [PATCH v5 0/6] make rte_intr_handle internal
> >
> > Moving struct rte_intr_handle as an internal structure to
> > avoid any ABI breakages in future. Since this structure defines
> > some static arrays and changing respective macros breaks the ABI.
> > Eg:
> > Currently RTE_MAX_RXTX_INTR_VEC_ID imposes a limit of maximum 512
> > MSI-X interrupts that can be defined for a PCI device, while PCI
> > specification allows maximum 2048 MSI-X interrupts that can be used.
> > If some PCI device requires more than 512 vectors, either change the
> > RTE_MAX_RXTX_INTR_VEC_ID limit or dynamically allocate based on
> > PCI device MSI-X size on probe time. Either way its an ABI breakage.
> >
> > Change already included in 21.11 ABI improvement spreadsheet (item 42):
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Furld
> > efense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps-
> > 3A__docs.google.com_s&amp;data=04%7C01%7Crasland%40nvidia.com%7C
> > 567d8ee2e3c842a9e59808d9959d822e%7C43083d15727340c1b7db39efd9ccc1
> > 7a%7C0%7C0%7C637705326003996997%7CUnknown%7CTWFpbGZsb3d8eyJ
> > WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> > 7C1000&amp;sdata=7UgxpkEtH%2Fnjk7xo9qELjqWi58XLzzCH2pimeDWLzvc%
> > 3D&amp;reserved=0
> > preadsheets_d_1betlC000ua5SsSiJIcC54mCCCJnW6voH5Dqv9UxeyfE_edit-
> > 23gid-
> > 3D0&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=5ESHPj7V-
> > 7JdkxT_Z_SU6RrS37ys4U
> > XudBQ_rrS5LRo&m=7dl3OmXU7QHMmWYB6V1hYJtq1cUkjfhXUwze2Si_48c
> > &s=lh6DEGhR
> > Bg1shODpAy3RQk-H-0uQx5icRfUBf9dtCp4&e=
> >
> > This series makes struct rte_intr_handle totally opaque to the outside
> > world by wrapping it inside a .c file and providing get set wrapper APIs
> > to read or manipulate its fields.. Any changes to be made to any of the
> > fields should be done via these get set APIs.
> > Introduced a new eal_common_interrupts.c where all these APIs are
> > defined
> > and also hides struct rte_intr_handle definition.
> >
> > Details on each patch of the series:
> > Patch 1: eal/interrupts: implement get set APIs
> > This patch provides prototypes and implementation of all the new
> > get set APIs. Alloc APIs are implemented to allocate memory for
> > interrupt handle instance. Currently most of the drivers defines
> > interrupt handle instance as static but now it cant be static as
> > size of rte_intr_handle is unknown to all the drivers. Drivers are
> > expected to allocate interrupt instances during initialization
> > and free these instances during cleanup phase.
> > This patch also rearranges the headers related to interrupt
> > framework. Epoll related definitions prototypes are moved into a
> > new header i.e. rte_epoll.h and APIs defined in rte_eal_interrupts.h
> > which were driver specific are moved to rte_interrupts.h (as anyways
> > it was accessible and used outside DPDK library. Later in the series
> > rte_eal_interrupts.h is removed.
> >
> > Patch 2: eal/interrupts: avoid direct access to interrupt handle
> > Modifying the interrupt framework for linux and freebsd to use these
> > get set alloc APIs as per requirement and avoid accessing the fields
> > directly.
> >
> > Patch 3: test/interrupt: apply get set interrupt handle APIs
> > Updating interrupt test suite to use interrupt handle APIs.
> >
> > Patch 4: drivers: remove direct access to interrupt handle fields
> > Modifying all the drivers and libraries which are currently directly
> > accessing the interrupt handle fields. Drivers are expected to
> > allocated the interrupt instance, use get set APIs with the allocated
> > interrupt handle and free it on cleanup.
> >
> > Patch 5: eal/interrupts: make interrupt handle structure opaque
> > In this patch rte_eal_interrupt.h is removed, struct rte_intr_handle
> > definition is moved to c file to make it completely opaque. As part of
> > interrupt handle allocation, array like efds and elist(which are currently
> > static) are dynamically allocated with default size
> > (RTE_MAX_RXTX_INTR_VEC_ID). Later these arrays can be reallocated as per
> > device requirement using new API rte_intr_handle_event_list_update().
> > Eg, on PCI device probing MSIX size can be queried and these arrays can
> > be reallocated accordingly.
> >
> > Patch 6: eal/alarm: introduce alarm fini routine
> > Introducing alarm fini routine, as the memory allocated for alarm interrupt
> > instance can be freed in alarm fini.
> >
> > Testing performed:
> > 1. Validated the series by running interrupts and alarm test suite.
> > 2. Validate l3fwd power functionality with octeontx2 and i40e intel cards,
> >    where interrupts are expected on packet arrival.
> >
> > v1:
> > * Fixed freebsd compilation failure
> > * Fixed seg fault in case of memif
> >
> > v2:
> > * Merged the prototype and implementation patch to 1.
> > * Restricting allocation of single interrupt instance.
> > * Removed base APIs, as they were exposing internally
> > allocated memory information.
> > * Fixed some memory leak issues.
> > * Marked some library specific APIs as internal.
> >
> > v3:
> > * Removed flag from instance alloc API, rather auto detect
> > if memory should be allocated using glibc malloc APIs or
> > rte_malloc*
> > * Added APIs for get/set windows handle.
> > * Defined macros for repeated checks.
> >
> > v4:
> > * Rectified some typo in the APIs documentation.
> > * Better names for some internal variables.
> >
> > v5:
> > * Reverted back to passing flag to instance alloc API, as
> > with auto detect some multiprocess issues existing in the
> > library were causing tests failure.
> > * Rebased to top of tree.
> >
> > Harman Kalra (6):
> >   eal/interrupts: implement get set APIs
> >   eal/interrupts: avoid direct access to interrupt handle
> >   test/interrupt: apply get set interrupt handle APIs
> >   drivers: remove direct access to interrupt handle
> >   eal/interrupts: make interrupt handle structure opaque
> >   eal/alarm: introduce alarm fini routine
> >
> >  MAINTAINERS                                   |   1 +
> >  app/test/test_interrupts.c                    | 163 +++--
> >  drivers/baseband/acc100/rte_acc100_pmd.c      |  18 +-
> >  .../fpga_5gnr_fec/rte_fpga_5gnr_fec.c         |  21 +-
> >  drivers/baseband/fpga_lte_fec/fpga_lte_fec.c  |  21 +-
> >  drivers/bus/auxiliary/auxiliary_common.c      |   2 +
> >  drivers/bus/auxiliary/linux/auxiliary.c       |  10 +
> >  drivers/bus/auxiliary/rte_bus_auxiliary.h     |   2 +-
> >  drivers/bus/dpaa/dpaa_bus.c                   |  28 +-
> >  drivers/bus/dpaa/rte_dpaa_bus.h               |   2 +-
> >  drivers/bus/fslmc/fslmc_bus.c                 |  16 +-
> >  drivers/bus/fslmc/fslmc_vfio.c                |  32 +-
> >  drivers/bus/fslmc/portal/dpaa2_hw_dpio.c      |  20 +-
> >  drivers/bus/fslmc/portal/dpaa2_hw_pvt.h       |   2 +-
> >  drivers/bus/fslmc/rte_fslmc.h                 |   2 +-
> >  drivers/bus/ifpga/ifpga_bus.c                 |  15 +-
> >  drivers/bus/ifpga/rte_bus_ifpga.h             |   2 +-
> >  drivers/bus/pci/bsd/pci.c                     |  21 +-
> >  drivers/bus/pci/linux/pci.c                   |   4 +-
> >  drivers/bus/pci/linux/pci_uio.c               |  73 +-
> >  drivers/bus/pci/linux/pci_vfio.c              | 115 ++-
> >  drivers/bus/pci/pci_common.c                  |  29 +-
> >  drivers/bus/pci/pci_common_uio.c              |  21 +-
> >  drivers/bus/pci/rte_bus_pci.h                 |   4 +-
> >  drivers/bus/vmbus/linux/vmbus_bus.c           |   6 +
> >  drivers/bus/vmbus/linux/vmbus_uio.c           |  37 +-
> >  drivers/bus/vmbus/rte_bus_vmbus.h             |   2 +-
> >  drivers/bus/vmbus/vmbus_common_uio.c          |  24 +-
> >  drivers/common/cnxk/roc_cpt.c                 |   8 +-
> >  drivers/common/cnxk/roc_dev.c                 |  14 +-
> >  drivers/common/cnxk/roc_irq.c                 | 108 +--
> >  drivers/common/cnxk/roc_nix_inl_dev_irq.c     |   8 +-
> >  drivers/common/cnxk/roc_nix_irq.c             |  36 +-
> >  drivers/common/cnxk/roc_npa.c                 |   2 +-
> >  drivers/common/cnxk/roc_platform.h            |  49 +-
> >  drivers/common/cnxk/roc_sso.c                 |   4 +-
> >  drivers/common/cnxk/roc_tim.c                 |   4 +-
> >  drivers/common/octeontx2/otx2_dev.c           |  14 +-
> >  drivers/common/octeontx2/otx2_irq.c           | 117 +--
> >  .../octeontx2/otx2_cryptodev_hw_access.c      |   4 +-
> >  drivers/event/octeontx2/otx2_evdev_irq.c      |  12 +-
> >  drivers/mempool/octeontx2/otx2_mempool.c      |   2 +-
> >  drivers/net/atlantic/atl_ethdev.c             |  20 +-
> >  drivers/net/avp/avp_ethdev.c                  |   8 +-
> >  drivers/net/axgbe/axgbe_ethdev.c              |  12 +-
> >  drivers/net/axgbe/axgbe_mdio.c                |   6 +-
> >  drivers/net/bnx2x/bnx2x_ethdev.c              |  10 +-
> >  drivers/net/bnxt/bnxt_ethdev.c                |  33 +-
> >  drivers/net/bnxt/bnxt_irq.c                   |   4 +-
> >  drivers/net/dpaa/dpaa_ethdev.c                |  47 +-
> >  drivers/net/dpaa2/dpaa2_ethdev.c              |  10 +-
> >  drivers/net/e1000/em_ethdev.c                 |  23 +-
> >  drivers/net/e1000/igb_ethdev.c                |  79 +--
> >  drivers/net/ena/ena_ethdev.c                  |  35 +-
> >  drivers/net/enic/enic_main.c                  |  26 +-
> >  drivers/net/failsafe/failsafe.c               |  23 +-
> >  drivers/net/failsafe/failsafe_intr.c          |  43 +-
> >  drivers/net/failsafe/failsafe_ops.c           |  19 +-
> >  drivers/net/failsafe/failsafe_private.h       |   2 +-
> >  drivers/net/fm10k/fm10k_ethdev.c              |  32 +-
> >  drivers/net/hinic/hinic_pmd_ethdev.c          |  10 +-
> >  drivers/net/hns3/hns3_ethdev.c                |  57 +-
> >  drivers/net/hns3/hns3_ethdev_vf.c             |  64 +-
> >  drivers/net/hns3/hns3_rxtx.c                  |   2 +-
> >  drivers/net/i40e/i40e_ethdev.c                |  53 +-
> >  drivers/net/iavf/iavf_ethdev.c                |  42 +-
> >  drivers/net/iavf/iavf_vchnl.c                 |   4 +-
> >  drivers/net/ice/ice_dcf.c                     |  10 +-
> >  drivers/net/ice/ice_dcf_ethdev.c              |  21 +-
> >  drivers/net/ice/ice_ethdev.c                  |  49 +-
> >  drivers/net/igc/igc_ethdev.c                  |  45 +-
> >  drivers/net/ionic/ionic_ethdev.c              |  17 +-
> >  drivers/net/ixgbe/ixgbe_ethdev.c              |  66 +-
> >  drivers/net/memif/memif_socket.c              | 111 ++-
> >  drivers/net/memif/memif_socket.h              |   4 +-
> >  drivers/net/memif/rte_eth_memif.c             |  61 +-
> >  drivers/net/memif/rte_eth_memif.h             |   2 +-
> >  drivers/net/mlx4/mlx4.c                       |  19 +-
> >  drivers/net/mlx4/mlx4.h                       |   2 +-
> >  drivers/net/mlx4/mlx4_intr.c                  |  47 +-
> >  drivers/net/mlx5/linux/mlx5_os.c              |  53 +-
> >  drivers/net/mlx5/linux/mlx5_socket.c          |  25 +-
> >  drivers/net/mlx5/mlx5.h                       |   6 +-
> >  drivers/net/mlx5/mlx5_rxq.c                   |  42 +-
> >  drivers/net/mlx5/mlx5_trigger.c               |   4 +-
> >  drivers/net/mlx5/mlx5_txpp.c                  |  26 +-
> >  drivers/net/netvsc/hn_ethdev.c                |   4 +-
> >  drivers/net/nfp/nfp_common.c                  |  34 +-
> >  drivers/net/nfp/nfp_ethdev.c                  |  13 +-
> >  drivers/net/nfp/nfp_ethdev_vf.c               |  13 +-
> >  drivers/net/ngbe/ngbe_ethdev.c                |  29 +-
> >  drivers/net/octeontx2/otx2_ethdev_irq.c       |  35 +-
> >  drivers/net/qede/qede_ethdev.c                |  16 +-
> >  drivers/net/sfc/sfc_intr.c                    |  30 +-
> >  drivers/net/tap/rte_eth_tap.c                 |  36 +-
> >  drivers/net/tap/rte_eth_tap.h                 |   2 +-
> >  drivers/net/tap/tap_intr.c                    |  32 +-
> >  drivers/net/thunderx/nicvf_ethdev.c           |  12 +
> >  drivers/net/thunderx/nicvf_struct.h           |   2 +-
> >  drivers/net/txgbe/txgbe_ethdev.c              |  38 +-
> >  drivers/net/txgbe/txgbe_ethdev_vf.c           |  33 +-
> >  drivers/net/vhost/rte_eth_vhost.c             |  76 +-
> >  drivers/net/virtio/virtio_ethdev.c            |  21 +-
> >  .../net/virtio/virtio_user/virtio_user_dev.c  |  48 +-
> >  drivers/net/vmxnet3/vmxnet3_ethdev.c          |  43 +-
> >  drivers/raw/ifpga/ifpga_rawdev.c              |  62 +-
> >  drivers/raw/ntb/ntb.c                         |   9 +-
> >  .../regex/octeontx2/otx2_regexdev_hw_access.c |   4 +-
> >  drivers/vdpa/ifc/ifcvf_vdpa.c                 |   5 +-
> >  drivers/vdpa/mlx5/mlx5_vdpa.c                 |  10 +
> >  drivers/vdpa/mlx5/mlx5_vdpa.h                 |   4 +-
> >  drivers/vdpa/mlx5/mlx5_vdpa_event.c           |  22 +-
> >  drivers/vdpa/mlx5/mlx5_vdpa_virtq.c           |  45 +-
> >  lib/bbdev/rte_bbdev.c                         |   4 +-
> >  lib/eal/common/eal_common_interrupts.c        | 588 +++++++++++++++
> >  lib/eal/common/eal_private.h                  |  11 +
> >  lib/eal/common/meson.build                    |   1 +
> >  lib/eal/freebsd/eal.c                         |   1 +
> >  lib/eal/freebsd/eal_alarm.c                   |  53 +-
> >  lib/eal/freebsd/eal_interrupts.c              | 112 ++-
> >  lib/eal/include/meson.build                   |   2 +-
> >  lib/eal/include/rte_eal_interrupts.h          | 269 -------
> >  lib/eal/include/rte_eal_trace.h               |  24 +-
> >  lib/eal/include/rte_epoll.h                   | 118 ++++
> >  lib/eal/include/rte_interrupts.h              | 668 +++++++++++++++++-
> >  lib/eal/linux/eal.c                           |   1 +
> >  lib/eal/linux/eal_alarm.c                     |  37 +-
> >  lib/eal/linux/eal_dev.c                       |  63 +-
> >  lib/eal/linux/eal_interrupts.c                | 303 +++++---
> >  lib/eal/version.map                           |  46 +-
> >  lib/ethdev/ethdev_pci.h                       |   2 +-
> >  lib/ethdev/rte_ethdev.c                       |  14 +-
> >  132 files changed, 3631 insertions(+), 1713 deletions(-)
> >  create mode 100644 lib/eal/common/eal_common_interrupts.c
> >  delete mode 100644 lib/eal/include/rte_eal_interrupts.h
> >  create mode 100644 lib/eal/include/rte_epoll.h
> >
> > --
> > 2.18.0
>
> This series is causing this seg fault with MLX5 pmd:
> Thread 1 "dpdk-l3fwd-powe" received signal SIGSEGV, Segmentation fault.
> rte_intr_free_epoll_fd (intr_handle=0x0) at ../lib/eal/linux/eal_interrupts.c:1512
> 1512                    if (__atomic_load_n(&rev->status,
> (gdb) bt
> #0  rte_intr_free_epoll_fd (intr_handle=0x0) at ../lib/eal/linux/eal_interrupts.c:1512
> #1  0x0000555556de7814 in mlx5_rx_intr_vec_disable (dev=0x55555b554a40 <rte_eth_devices>) at ../drivers/net/mlx5/mlx5_rxq.c:934
> #2  0x0000555556de73da in mlx5_rx_intr_vec_enable (dev=0x55555b554a40 <rte_eth_devices>) at ../drivers/net/mlx5/mlx5_rxq.c:836
> #3  0x0000555556e04012 in mlx5_dev_start (dev=0x55555b554a40 <rte_eth_devices>) at ../drivers/net/mlx5/mlx5_trigger.c:1146
> #4  0x0000555555b82da7 in rte_eth_dev_start (port_id=0) at ../lib/ethdev/rte_ethdev.c:1823
> #5  0x000055555575e66d in main (argc=7, argv=0x7fffffffe3f0) at ../examples/l3fwd-power/main.c:2811
> (gdb) f 1
> #1  0x0000555556de7814 in mlx5_rx_intr_vec_disable (dev=0x55555b554a40 <rte_eth_devices>) at ../drivers/net/mlx5/mlx5_rxq.c:934
> 934             rte_intr_free_epoll_fd(intr_handle);
>
>
> It can be easily reproduced as following:
> dpdk-l3fwd-power -n 4 -a 0000:08:00.0,txq_inline_mpw=439,rx_vec_en=1 -a 0000:08:00.,txq_inline_mpw=439,rx_vec_en=1 -c 0xfffffff -- -p 0x3 -P --interrupt-only --parse-ptype --config='(0, 0, 0)(1, 0, 1)(0, 1, 2)(1, 1, 3)(0, 2, 4)(1, 2, 5)(0, 3, 6)(1, 3, 7)'
>

That confirms my suspicion on pci bus update that look at
RTE_PCI_DRV_NEED_MAPPING.
v7 incoming.


-- 
David Marchand