09/02/2022 12:04, David Marchand:
> On Mon, Jan 10, 2022 at 6:26 AM <rohit....@nxp.com> wrote:
> >
> > From: Rohit Raj <rohit....@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.
