Re: [RFC PATCH 0/3] gpu: nova-core: add basic timer subdevice implementation

[Simona Vetter <simona.vetter@ffwll.ch>]

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