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

[Thomas Monjalon <thomas@monjalon.net>]

09/02/2022 12:04, David Marchand:
> 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.

[...]
> > +/* 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")) {

Please do an explicit comparison "== 0".

> > +                       vbus = bus;
> > +                       continue;
> > +               }
> > +
> > +               if (bus->close) {

Please do an explicit comparison with "!= NULL".
We can also completely remove this check and implement the callback
in all buses. It should loop in all remaining devices and remove them.

> > +                       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.

Yes, good catch.

We don't have to expose this function as API.
This can be an internal function called only in rte_eal_cleanup().

Instead, it would be more useful to have a public function
to close a single bus by its name.

> > @@ -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 */

As David said, it is breaking the ABI.

[...]
> > @@ -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.

+1, just log and save the error code for return at the end.