re: config_mounroot - spinout while attaching nouveaufb0 on amd64 with LOCKDEBUG
Jarom?r Dole?ek writes: > Le lun. 17 f=C3=A9vr. 2020 =C3=A0 17:55, matthew green = > a =C3=A9crit : > > > > FWIW, i've been running my radeon with a patch that exlicitly drops > > kernel lock around the "real attach" function (the one that config > > mountroot ends up calling.) > > > > we really need to MPSAFE-ify the autoconf subsystem. right now, it > > is expected that autoconf runs with kernel lock... i am not sure of > > the path we should take for this -- but let's actually have a design > > in place we are happy with, while my change below works, it's ugly > > and wrong. > > Would it make sense to simply require all callers of > config_mountroot() to have MPSAFE config routines and call the > callbacks for them without kernel lock? > > There are just few of those, and they can simply be changed to do the > KERNEL_LOCK()/UNLOCK themselves. this is a tempting idea, but i'm loathe to have different parts of autoconf run with different locking semantics. eg, my hack has the possibility of attempting to update the autoconf private data which is currently protected by the kernel lock, and i suspect it's actually possible because these routines attach sub-devices: radeon0 at pci10 dev 0 function 0: ATI Technologies Mobility Radeon HD 4500 (rev. 0x00) radeondrmkmsfb0 at radeon0 wsdisplay0 at radeondrmkmsfb0 kbdmux 1 wsmux1: connecting to wsdisplay0 wskbd0: connecting to wsdisplay0 right now with my change, all that runs with no protection. it seems non-trivial to fix that all up for only config_mountroot(). this is why i wrote "let's actually have a design" :-) .mrg.
Re: config_mounroot - spinout while attaching nouveaufb0 on amd64 with LOCKDEBUG
Le lun. 17 févr. 2020 à 17:55, matthew green a écrit : > > FWIW, i've been running my radeon with a patch that exlicitly drops > kernel lock around the "real attach" function (the one that config > mountroot ends up calling.) > > we really need to MPSAFE-ify the autoconf subsystem. right now, it > is expected that autoconf runs with kernel lock... i am not sure of > the path we should take for this -- but let's actually have a design > in place we are happy with, while my change below works, it's ugly > and wrong. Would it make sense to simply require all callers of config_mountroot() to have MPSAFE config routines and call the callbacks for them without kernel lock? There are just few of those, and they can simply be changed to do the KERNEL_LOCK()/UNLOCK themselves. Jaromir
Re: config_mounroot - spinout while attaching nouveaufb0 on amd64 with LOCKDEBUG
jaromir.dole...@gmail.com (=?UTF-8?B?SmFyb23DrXIgRG9sZcSNZWs=?=) writes: >I confirmed via ddb that this happens due to config_mountroot_thread() >holding the kernel lock for too long Probably because some driver is busy-waiting while holding the lock. I had the same with lots of console output plus long delay() calls in sdmmc. -- -- Michael van Elst Internet: mlel...@serpens.de "A potential Snark may lurk in every tree."
re: config_mounroot - spinout while attaching nouveaufb0 on amd64 with LOCKDEBUG
FWIW, i've been running my radeon with a patch that exlicitly drops kernel lock around the "real attach" function (the one that config mountroot ends up calling.) we really need to MPSAFE-ify the autoconf subsystem. right now, it is expected that autoconf runs with kernel lock... i am not sure of the path we should take for this -- but let's actually have a design in place we are happy with, while my change below works, it's ugly and wrong. .mrg. Index: sys/external/bsd/drm2/radeon/radeon_pci.c === RCS file: /cvsroot/src/sys/external/bsd/drm2/radeon/radeon_pci.c,v retrieving revision 1.14 diff -p -u -r1.14 radeon_pci.c --- sys/external/bsd/drm2/radeon/radeon_pci.c 24 Jan 2020 11:44:27 - 1.14 +++ sys/external/bsd/drm2/radeon/radeon_pci.c 17 Feb 2020 16:54:05 - @@ -229,6 +229,9 @@ radeon_attach_real(device_t self) unsigned long flags; int error; + /* XXXSMP autoconf */ + KERNEL_UNLOCK_ONE(NULL); + ok = radeon_pci_lookup(pa, ); KASSERT(ok); @@ -274,6 +277,9 @@ radeon_attach_real(device_t self) } out: sc->sc_dev = self; + + /* XXXSMP autoconf */ + KERNEL_LOCK(1, NULL); } static int
Re: config_mounroot - spinout while attaching nouveaufb0 on amd64 with LOCKDEBUG
> Date: Mon, 17 Feb 2020 00:29:03 +0100 > From: Jaromír Doleček > > while debugging the MSI attachment for nouveaufb0, I've got several > times spinout panic like one below. It doesn't happen on every boot, > but on almost every one. > > I confirmed via ddb that this happens due to config_mountroot_thread() > holding the kernel lock for too long - that's where backtrack for cpu > 0 (which holds the lock) leads. > > Would it be worth it to spend some efford to run that thread without > kernel lock, or postpone the driver initialization even further? Maybe we could have an MP-safe variant of config_mountroot which nouveau could use. Just a matter of teaching config_mountroot_thread to KERNEL_LOCK/UNLOCK around the callback according to a flag stored somewhere, much like we do elsewhere. Deferring it some other way would be suboptimal because we really want this as soon as possible once the file system is available to load firmware from. (It would be even better if we could get the firmware earlier from the botloader somehow, like the bootloader can load modules, but it's not entirely clear how to structure the logic to decide _which_ firmware to load early on.)
