Op 24-07-13 16:24, David Herrmann schreef: > Hi > > On Wed, Jul 24, 2013 at 4:03 PM, Maarten Lankhorst > <maarten.lankhorst@xxxxxxxxxxxxx> wrote: >> Op 24-07-13 15:43, David Herrmann schreef: >>> If we want to support real hotplugging, we need to block new syscalls from >>> entering any drm_* fops. We also need to wait for these to finish before >>> unregistering a device. >>> >>> This patch introduces drm_dev_get/put_active() which mark a device as >>> active during file-ops. If a device is unplugged, drm_dev_get_active() >>> will fail and prevent users from using this device. >>> >>> Internally, we use rwsems to implement this. It allows simultaneous users >>> (down_read) and we can block on them (down_write) to wait until they are >>> done. This way, a drm_dev_unregister() can be called at any time and does >>> not have to wait for the last drm_release() call. >>> >>> Note that the current "atomic_t unplugged" approach is not safe. Imagine >>> an unplugged device but a user-space context which already is beyong the >>> "drm_device_is_unplugged()" check. We have no way to prevent any following >>> mmap operation or buffer access. The percpu-rwsem avoids this by >>> protecting a whole file-op call and waiting with unplugging a device until >>> all pending calls are finished. >>> >>> FIXME: We still need to update all the driver's fops in case they don't >>> use the DRM core stubs. A quick look showed only custom mmap callbacks. >> Meh, I don't think it's possible to make the mmaps completely race free. > It is. See this scenario: > > drm_dev_unregister() sets "plugged = true;" in write_lock(). This > guarantees, there is currently no other pending user in any file-ops > or mmap-fault handler (assuming the mmap fault handler is wrapped in > drm_dev_get_active(), drm_dev_put_active()). > > After that, I call unmap_mapping_range() which resets all DRM-mapped > PTEs of user-space processes. So if a mmap is accessed afterwards, it > will trap and the fault() handler will fail with SIGBUS because > drm_dev_get_active() will return false. New mmaps cannot be created, > because drm_mmap() and alike will also be protected by > drm_dev_get_active(). > > I don't see any race here, do you? > >> I 'solved' this by taking mapping->i_mmap_mutex, it reduces the window, >> because all the mmap callbacks are called while holding that lock, so at least >> new mappings from splitting mappings up cannot happen. > Is this somehow faster/better than wrapping fault callbacks in > get_active/put_active? Did you test the last patch with PROVE_LOCKING? >> Why did you make the percpu rwsem local to the device? >> It's only going to be taking in write mode during device destruction, which >> isn't exactly fast anyway. A single global rwsem wouldn't have any negative effect. > I have 4 UDL cards at home and I dislike that all cards are blocked > for a quite long time if I unplug just one card. As long as we have > drm_global_mutex with this broad use, we cannot fix it. But I thought, > I'd avoid introducing more global locks so maybe I can some day > improve that, too. > > So I disagree, once we reduce drm_global_mutex, a single global rwsem > _will_ have a negative effect. Besides, if you have only 1 GPU, it > doesn't matter as you still only get a single rwsem. > >> My original patch was locking mapping->i_mmap_mutex, then the rwsem in write mode, then the >> global mutex lock during unplug, to ensure that all code saw the unplugged correctly. >> >> That way using any of those 3 locks would be sufficient to check dev->unplugged safely. > If we want to fix the races completely, I don't think using > i_mmap_mutex helps us. > > Regarding drm_global_mutex, as I said, my intention is to reduce this > lock to a minimum. The only place where we need it is minor-allocation > and driver-device-lists (ignoring the legacy users like drm-lock). For > both, we could easily use spinlocks. So it's the same reason as above: > I'd like to avoid contention on drm_global_mutex. > Besides, I dislike broad locks. It's not like we safe a lot of memory > here if we have one spinlock per driver instead of a global drm mutex. > And if we have small-ranging locks, it is much easier to review them > (at least to me). > > Cheers > David > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
