Date: Sun, 24 Nov 2019 12:42:26 -0700 From: Warner Losh <imp@bsdimp.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r355037 - head/sys/dev/pci Message-ID: <CANCZdfqh5jEZE03ZGDdPnOQ6RR-%2Bu%2B4CSo=DOvoGko0hNeHkmw@mail.gmail.com> In-Reply-To: <20191124164536.GC2707@kib.kiev.ua> References: <201911232343.xANNhqkQ097797@repo.freebsd.org> <20191124131010.GB2707@kib.kiev.ua> <CANCZdfrtKQmGiB=w5Q2x7sTCh4JO63mdJn6fg%2BAXfPfeyQGkfA@mail.gmail.com> <20191124164536.GC2707@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Nov 24, 2019, 9:45 AM Konstantin Belousov <kostikbel@gmail.com> wrote: > On Sun, Nov 24, 2019 at 07:37:01AM -0700, Warner Losh wrote: > > On Sun, Nov 24, 2019, 6:10 AM Konstantin Belousov <kostikbel@gmail.com> > > wrote: > > > > > On Sat, Nov 23, 2019 at 11:43:52PM +0000, Warner Losh wrote: > > > > Author: imp > > > > Date: Sat Nov 23 23:43:52 2019 > > > > New Revision: 355037 > > > > URL: https://svnweb.freebsd.org/changeset/base/355037 > > > > > > > > Log: > > > > Push Giant down one layer > > > > > > > > The /dev/pci device doesn't need GIANT, per se. However, one > routine > > > > that it calls, pci_find_dbsf implicitly does. It walks a list that > can > > > > change when PCI scans a new bus. With hotplug, this means we could > > > > have a race with that scanning. To prevent that, take out Giant > around > > > > scanning the list. > > > > > > > > However, given that we have places in the tree that drop giant, if > > > > held when we call into them, the whole use of Giant to protect > newbus > > > > may be less effective that we desire, so add a comment about why > we're > > > > talking it out, and we'll address the issue when we lock newbus > with > > > > something other than Giant. > > > > > > > > Modified: > > > > head/sys/dev/pci/pci.c > > > > head/sys/dev/pci/pci_user.c > > > > > > > > Modified: head/sys/dev/pci/pci.c > > > > > > > > ============================================================================== > > > > --- head/sys/dev/pci/pci.c Sat Nov 23 23:41:21 2019 > (r355036) > > > > +++ head/sys/dev/pci/pci.c Sat Nov 23 23:43:52 2019 > (r355037) > > > > @@ -445,18 +445,21 @@ pci_find_bsf(uint8_t bus, uint8_t slot, uint8_t > > > func) > > > > device_t > > > > pci_find_dbsf(uint32_t domain, uint8_t bus, uint8_t slot, uint8_t > func) > > > > { > > > > - struct pci_devinfo *dinfo; > > > > + struct pci_devinfo *dinfo = NULL; > > > > > > > > + /* Giant because newbus is Giant locked revisit with newbus > > > locking */ > > > > + mtx_lock(&Giant); > > > > STAILQ_FOREACH(dinfo, &pci_devq, pci_links) { > > > > if ((dinfo->cfg.domain == domain) && > > > > (dinfo->cfg.bus == bus) && > > > > (dinfo->cfg.slot == slot) && > > > > (dinfo->cfg.func == func)) { > > > > - return (dinfo->cfg.dev); > > > > + break; > > > > } > > > > } > > > > + mtx_unlock(&Giant); > > > > > > > > - return (NULL); > > > > + return (dinfo != NULL ? dinfo->cfg.dev : NULL); > > > I do not think this change is correct. If the parallel hotplug, or > > > rather, hot-unplug event occurs, then dinfo potentially becomes invalid > > > right after the Giant unlock, which makes both this function and its > > > callers to access freed memory. Having caller to lock a newbus lock > > > around both the call and consumption of the returned data is required. > > > > > > There are many data lifetime issues. If anything the PCI user device > calls > > drops Giant and then picks it back up again we are in the same boat... I > > totally agree this is a bad situation, but can only really be fixed by > > locking newbus with a different lock than Giant and likely using some > kind > > of reference count for device_t that are handed out... > > > > In the mean time, I'll move giant back up into the ioctl routine and hope > > it isn't dropped by things it calls..m > > I think we can start at least by marking the Giant acqusitions that are > related to newbus. > > I never saw anybody talking publically why the naive translation of newbus > Giant into a sleepable lock, e.g. sx in exclusive mode, cannot work. From > my memory, one of the big issues is that many sleeps done at probe/attach, > need to drop the newbus lock. > If we are holding a topology lock, we can't drop it... otherwise someone else can acquire it and change things out from us... Also, locks are useless for device_t lifetimes. Too many places cache them in non obvious ways, so we need to add some kind of ref count. And since these relationships are often not parent / child, we'd likely need a spoilage call to release refs. You shouldn't complete your detaching if others holding references. Finally, we walk both up and down the tree. When I tried a super naive lock in the past, I ran into a bunch of LORs due to this. And what do you do if multiple nodes in the tree want to start a detach/delete at the same time? Or if there is a suspend going on at the same time... These are all solvable problems, but simply replacing giant with a sx lock won't solve them and may introduce issues because giant is special in many ways... it would, however, fix the one aspect of giant we don't want: random parts of the kernel dropping it. Warner >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqh5jEZE03ZGDdPnOQ6RR-%2Bu%2B4CSo=DOvoGko0hNeHkmw>