Re: [PATCH v5 1/2] eal: add API for bus close

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

On Mon, Jan 10, 2022 at 6:26 AM <rohit.raj@nxp.com> wrote:
>
> From: Rohit Raj <rohit.raj@nxp.com>
>
> As per the current code we have API for bus probe, but the
> bus close API is missing. This breaks the multi process
> scenarios as objects are not cleaned while terminating the
> secondary processes.

After an application crash, how does this bus resets the associated resources?


>
> This patch adds a new API rte_bus_close() for cleanup of
> bus objects which were acquired during probe.

The patch in its current form breaks the ABI on rte_bus object.
This can be seen in GHA, or calling DPDK_ABI_REF_VERSION=v21.11
./devtools/test-meson-builds.sh.


[snip]

> diff --git a/doc/guides/rel_notes/release_22_03.rst b/doc/guides/rel_notes/release_22_03.rst
> index 6d99d1eaa9..7417606a2a 100644
> --- a/doc/guides/rel_notes/release_22_03.rst
> +++ b/doc/guides/rel_notes/release_22_03.rst
> @@ -55,6 +55,11 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =======================================================
>
> +    * **Added support to close bus.**
> +
> +      Added capability to allow a user to do cleanup of bus objects which
> +      were acquired during bus probe.
> +

Wrongly indented.


>
>  Removed Items
>  -------------
> @@ -84,6 +89,9 @@ API Changes
>     Also, make sure to start the actual text at the margin.
>     =======================================================
>
> +   * eal: Added new API ``rte_bus_close`` to perform cleanup bus objects which
> +     were acquired during bus probe.
> +
>
>  ABI Changes
>  -----------
> diff --git a/lib/eal/common/eal_common_bus.c b/lib/eal/common/eal_common_bus.c
> index baa5b532af..2c3c0a90d2 100644
> --- a/lib/eal/common/eal_common_bus.c
> +++ b/lib/eal/common/eal_common_bus.c
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright 2016 NXP
> + * Copyright 2016,2022 NXP
>   */
>
>  #include <stdio.h>
> @@ -85,6 +85,37 @@ rte_bus_probe(void)
>         return 0;
>  }
>
> +/* Close all devices of all buses */
> +int
> +rte_bus_close(void)
> +{
> +       int ret;
> +       struct rte_bus *bus, *vbus = NULL;
> +
> +       TAILQ_FOREACH(bus, &rte_bus_list, next) {
> +               if (!strcmp(bus->name, "vdev")) {
> +                       vbus = bus;
> +                       continue;
> +               }
> +
> +               if (bus->close) {
> +                       ret = bus->close();
> +                       if (ret)
> +                               RTE_LOG(ERR, EAL, "Bus (%s) close failed.\n",
> +                                       bus->name);
> +               }
> +       }
> +
> +       if (vbus && vbus->close) {
> +               ret = vbus->close();
> +               if (ret)
> +                       RTE_LOG(ERR, EAL, "Bus (%s) close failed.\n",
> +                               vbus->name);
> +       }

The vdev bus is special in that some drivers can reference objects
from other buses (see f4ce209a8ce5 ("eal: postpone vdev
initialization") and da76cc02342b ("eal: probe new virtual bus after
other bus devices")).
For this reason, I would expect that the vdev bus is closed before the
other buses.


> +
> +       return 0;
> +}
> +
>  /* Dump information of a single bus */
>  static int
>  bus_dump_one(FILE *f, struct rte_bus *bus)
> diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
> index a1cd2462db..87d70c6898 100644
> --- a/lib/eal/freebsd/eal.c
> +++ b/lib/eal/freebsd/eal.c
> @@ -984,6 +984,7 @@ rte_eal_cleanup(void)
>  {
>         struct internal_config *internal_conf =
>                 eal_get_internal_configuration();
> +       rte_bus_close();
>         rte_service_finalize();
>         rte_mp_channel_cleanup();
>         /* after this point, any DPDK pointers will become dangling */
> diff --git a/lib/eal/include/rte_bus.h b/lib/eal/include/rte_bus.h
> index bbbb6efd28..c6211bbd95 100644
> --- a/lib/eal/include/rte_bus.h
> +++ b/lib/eal/include/rte_bus.h
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright 2016 NXP
> + * Copyright 2016,2022 NXP
>   */
>
>  #ifndef _RTE_BUS_H_
> @@ -66,6 +66,23 @@ typedef int (*rte_bus_scan_t)(void);
>   */
>  typedef int (*rte_bus_probe_t)(void);
>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Implementation specific close function which is responsible for resetting all
> + * detected devices on the bus to a default state, closing UIO nodes or VFIO
> + * groups and also freeing any memory allocated during rte_bus_probe like
> + * private resources for device list.
> + *
> + * This is called while iterating over each registered bus.
> + *
> + * @return
> + *     0 for successful close
> + *     !0 for any error while closing
> + */
> +typedef int (*rte_bus_close_t)(void);
> +
>  /**
>   * Device iterator to find a device on a bus.
>   *
> @@ -263,6 +280,7 @@ struct rte_bus {
>         const char *name;            /**< Name of the bus */
>         rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
>         rte_bus_probe_t probe;       /**< Probe devices on bus */
> +       rte_bus_close_t close;       /**< Close devices on bus */
>         rte_bus_find_device_t find_device; /**< Find a device on the bus */
>         rte_bus_plug_t plug;         /**< Probe single device for drivers */
>         rte_bus_unplug_t unplug;     /**< Remove single device from driver */
> @@ -317,6 +335,16 @@ int rte_bus_scan(void);
>   */
>  int rte_bus_probe(void);
>
> +/**
> + * For each device on the buses, call the device specific close.
> + *
> + * @return
> + *      0 for successful close
> + *     !0 otherwise
> + */
> +__rte_experimental
> +int rte_bus_close(void);
> +
>  /**
>   * Dump information of all the buses registered with EAL.
>   *
> diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
> index 60b4924838..5c60131e46 100644
> --- a/lib/eal/linux/eal.c
> +++ b/lib/eal/linux/eal.c
> @@ -1362,6 +1362,14 @@ rte_eal_cleanup(void)
>
>         if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>                 rte_memseg_walk(mark_freeable, NULL);
> +
> +       /* Close all the buses and devices/drivers on them */
> +       if (rte_bus_close()) {
> +               rte_eal_init_alert("Cannot close devices");

You can't call rte_eal_*init*_alert in rte_eal_cleanup.

There is not much to do if the bus close fails, I'd rather leave the
cleanup continue.


> +               rte_errno = ENOTSUP;
> +               return -1;
> +       }
> +
>         rte_service_finalize();
>         rte_mp_channel_cleanup();
>         /* after this point, any DPDK pointers will become dangling */
> diff --git a/lib/eal/version.map b/lib/eal/version.map
> index ab28c22791..39882dbbd5 100644
> --- a/lib/eal/version.map
> +++ b/lib/eal/version.map
> @@ -420,6 +420,9 @@ EXPERIMENTAL {
>         rte_intr_instance_free;
>         rte_intr_type_get;
>         rte_intr_type_set;
> +
> +       # added in 22.03
> +       rte_bus_close;
>  };
>
>  INTERNAL {
> diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
> index 67db7f099a..5915ab6291 100644
> --- a/lib/eal/windows/eal.c
> +++ b/lib/eal/windows/eal.c
> @@ -260,6 +260,7 @@ rte_eal_cleanup(void)
>         struct internal_config *internal_conf =
>                 eal_get_internal_configuration();
>
> +       rte_bus_close();
>         eal_intr_thread_cancel();
>         eal_mem_virt2iova_cleanup();
>         /* after this point, any DPDK pointers will become dangling */
> --
> 2.17.1
>


-- 
David Marchand