[dpdk-dev] [PATCH] bus/vdev: fix probe same device twice

[Gaëtan Rivet]

On Wed, Nov 07, 2018 at 04:53:50PM +0000, Zhang, Qi Z wrote:
> 
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet at 6wind.com]
> > Sent: Tuesday, November 6, 2018 4:34 PM
> > To: Thomas Monjalon <thomas at monjalon.net>
> > Cc: Zhang, Qi Z <qi.z.zhang at intel.com>; dev at dpdk.org; Yigit, Ferruh
> > <ferruh.yigit at intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice
> > 
> > On Tue, Nov 06, 2018 at 09:36:22PM +0100, Thomas Monjalon wrote:
> > > 06/11/2018 16:46, Zhang, Qi Z:
> > > > From: Thomas Monjalon [mailto:thomas at monjalon.net]
> > > > >
> > > > > Hi,
> > > > >
> > > > > 06/11/2018 01:31, Qi Zhang:
> > > > > > When probe the same device at second time
> > > > >
> > > > > Sorry I stop on this first sentence.
> > > > > How and why do you probe a vdev twice?
> > > >
> > > > if we do rte_dev_hotplug_add or rte_dev_proble on a probed device.
> > > > (yes, this is not usually what an application want, but it can
> > > > happen by miss-operation, and this is covered by our test case, it
> > > > make sense to me that hotplug API should be robust enough to handle
> > > > that situation.)
> > >
> > > Yes I agree we must handle this situation.
> > >
> > > > we will failed at the second time as expected, but will not able to
> > > > detach the device any more, since during the second scan, original
> > vdev->device.devargs is corrupted.
> > >
> > > The root cause is we remove a devargs which was referenced.
> > > Could we overwrite the first devargs instead of removing it?
> > >
> > >
> > 
> > It's also possible to add a back-reference to an rte_device in [1], but that can
> > only work if only one rte_device references a devargs.
> > It seems to be the case now, but it might be good to enforce explicitly that
> > when a bus scans its devices, it should do a 1-to-1 map to devargs.
> > 
> > If mapping rte_device to rte_devargs needs to respect rules, it could help bus
> > developpers to have a function that will do the job: verify that the devargs is
> > not currently used, add the back-reference to the rte_device.
> > 
> > With the proper back-reference, it is possible to clean-up the device when
> > removing the devargs 
> 
> This may still not work for vdev, since the old reference is used in vdev_find to find a exist device by name during scan.
> (For PCI device, we have pci_addr, but vdev we use devargs->name to identify device, anyway this can be fixed in vdev, but that required a clone on the device name also break the coupling somehow.)

A bus should keep device identifiers within a device, without relying on
objects belonging to the EAL.

> I just don't understand "why we must tight the tighten the device -> devargs coupling, not loosen it"
> 

My point is that we are seemingly having problems with loose pointers,
broken mappings, memory leaks. So managing seems already too
complicated. Adding clones and copies will only make it more difficult
to get right.

It seems we have identified in this thread problematic behaviors from
developpers, instead of giving them more tools to shoot feet we can
instead give helpers to do what they are trying to do, but properly.

The end-goal is not to have several devargs lying around, copies of each
other, it is to avoid breaking devargs references.

> (and also to add the rte_devargs_extract() function
> > that would allow keeping the original devargs and insert it back if the hotplug
> > fails, then the mapping must be restored).
> 
> > 
> > [1]: https://mails.dpdk.org/archives/dev/2018-November/118274.html
> > 
> > --
> > Gaëtan Rivet
> > 6WIND

-- 
Gaëtan Rivet
6WIND