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

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Wednesday, November 7, 2018 10:15 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] bus/vdev: fix probe same device twice
> 
> On Wed, Nov 07, 2018 at 04:53:50PM +0000, Zhang, Qi Z wrote:
> >
> >
> > > -----Original Message-----
> > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > > Sent: Tuesday, November 6, 2018 4:34 PM
> > > To: Thomas Monjalon <thomas@monjalon.net>
> > > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Yigit, Ferruh
> > > <ferruh.yigit@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@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.

Clone is not a problem if they are encapsulated well, what we need here is some API like
rte_dev_set_devargs/rte_dev_clear_devargs, and developer just need to remember to use them but not assign devargs directly. 

The point here is remove an item in devargs should not destroy the content in rte_device at the same time (it happens on vdev and I didn't see a fix base on exist proposal), I have no objection for other way to fix this, but clone is the only way I can figure out right now.

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