On Wed, Feb 26, 2025 at 05:34:01PM -0800, John Hubbard wrote: > On Wed Feb 26, 2025 at 5:02 PM PST, Greg KH wrote: > > On Wed, Feb 26, 2025 at 07:47:30PM -0400, Jason Gunthorpe wrote: > >> The way misc device works you can't unload the module until all the > >> FDs are closed and the misc code directly handles races with opening > >> new FDs while modules are unloading. It is quite a different scheme > >> than discussed in this thread. > > > > And I would argue that is it the _right_ scheme to be following overall > > here. Removing modules with in-flight devices/drivers is to me is odd, > > and only good for developers doing work, not for real systems, right? > > Right...I think. I'm not experienced with misc, but I do know that the > "run driver code after driver release" is very, very concerning. > > I'm quite new to drivers/gpu/drm, so this is the first time I've learned > about this DRM behavior... It's really, really complicated, and not well documented. Probably should fix that. The issue is that you have at least 4 different lifetimes involved here, and people mix them up all the time and get confused. I discuss 3 of those here: https://lists.freedesktop.org/archives/intel-xe/2024-April/034195.html The 4th is the lifetime of the module .text section, for which we need try_module_get. Now the issue with that is that developers much prefer convenience over correctness on this, and routinely complain _very_ loudly about "unnecessary" module references. They're not, but to break the cycle that Jason points out a bit earlier you need 2 steps: - Nuke the driver binding manually through sysfs with the unbind files. - Nuke all userspace that might beholding files and other resources open. - At this point the module refcount should be zero and you can unload it. Except developers really don't like the manual unbind step, and so we're missing try_module_get() in a bunch of places where it really should be. Now wrt why you can't just solve this all at the subsystem level and guarantee that after drm_dev_unplug no code is running in driver callbacks anymore: In really, really simple subsystems like backlight this is doable. In drm with arbitrary ioctl this isn't, and you get to make a choice: - You wait until all driver code finishes, which could be never if there's ioctl that wait for render to complete and don't handle hotunplug correctly. This is a deadlock. In my experience this is theorically possible, practically no one gets this right and defacto means that actual hotunplug under load has a good chance of just hanging forever. Which is why drm doesn't do this. - You make the revoceable critical sections as small as possible, which makes hotunplug much, much less likely do deadlock. But means that after revoking hw access you still have arbitrary driver code running in callbacks, and you need to deal with. This is why I like the rust Revocable so much, because it's a normal rcu section, so disallows all sleeping. You might still deadlock on a busy loop waiting for hw without having a timeout. But that's generally fairly easy to spot, and good drivers have macros/helpers for this so that there is always a timeout. drm_dev_unplug uses sleepable rcu for practicality reasons and so has a much, much higher chance of deadlocks. Note that strictly speaking drm_device should hold a module reference on the driver, but see above for why we don't have that - developers prefer convenience over correctness in this area. > > Yes, networking did add that functionality to allow modules to be > > unloaded with network connections open, and I'm guessing RDMA followed > > that, but really, why? > > > > What is the requirement that means that you have to do this for function > > pointers? I can understand the disconnect issue between devices and > > drivers and open file handles (or sockets), as that is a normal thing, > > but not removing code from the system, that is not normal. > > > > I really hope that this "run after release" is something that Rust for > Linux drivers, and in particular, the gpu/nova*, gpu/drm/nova* drivers, > can *leave behind*. > > DRM may have had ${reasons} for this approach, but this nova effort is > rebuilding from the ground up. So we should avoid just blindly following > this aspect of the original DRM design. We can and should definitely try to make this much better. I think we can get to full correctness wrt the first 3 lifetime things in rust. I'm not sure whether handling module unload/.text lifetime is worth the bother, it's probably only going to upset developers if we try. At least that's been my experience. Cheers, Sima -- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
