On Fri, Feb 22, 2019 at 4:32 PM Matthew Wilcox <wi...@infradead.org> wrote: > > On Fri, Feb 22, 2019 at 10:40:14AM +0100, Daniel Vetter wrote: > > On Thu, Feb 21, 2019 at 10:41:41AM -0800, Matthew Wilcox wrote: > > > - Rename it to 'objects', as requested in todo.rst > > > > Yay, thanks! > > > > > - Also convert leases IDR to XArray as the two are occasionally used by > > > the same code (see drm_mode_get_lease_ioctl()) > > > - Refactor drm_mode_create_lease_ioctl() to create the new drm_master > > > early to avoid creating an XArray on the stack and reparenting it > > > afterwards. > > > > All lgtm, also the idr_replace replacement. > > > > One thing I wonder: For the lesse object xa, we really only store 0/1 in > > there, and I don't think that'll change. There was once the idea that we'd > > look up objects for a lease directly, bypassing the main object idr. But > > that doesn't work due to unregister/hotunplug rules, or at least it would > > be pain not worth having. Might be worth it to a lookup structure > > optimized for that. Or does XA already autocompress that for us? The > > object id are likely fairly compressed, good chance all the ones you need > > for a lease will fit into the first 64 id. > > The XArray doesn't compress the contents of the user pointers. It might be > worth augmenting the IDA to have all the functionality you need (there's > no ida_test() today, for example). I didn't want to take that on as part > of this project, and it's certainly no worse than the IDR. But it's on > my radar along with a couple of other places in the kernel that could benefit > from the IDA if only it had a couple of extra features (eg the fd table > would like to an ida_for_each()). > > It'd involve changing drm_mode_get_lease_ioctl() and maybe a couple of > other places where we use config.objects and leases interchangably, so > I wasn't entirely convinced it'd be worth it.
Makes all sense. > > > +++ b/drivers/gpu/drm/drm_auth.c > > > @@ -110,7 +110,7 @@ struct drm_master *drm_master_create(struct > > > drm_device *dev) > > > /* initialize the tree of output resource lessees */ > > > master->lessor = NULL; > > > master->lessee_id = 0; > > > - idr_init(&master->leases); > > > + xa_init(&master->leases); > > > > XA_FALGS_ALLOC1, for safety to make sure we never store 0 (considered > > invalid id throughout at least modern drm)? > > This xarray turns out not to be an allocating XArray, it's just one > we store pointers in at a predetermined ID. Marking it as allocating > wouldn't be terribly harmful, just slightly inefficient. tbf I didn't read what exactly the flag means in detail, just wondered whether it could help in making sure we never store something at id 0 (which would be a bug). But neither did the current code do such checks, so fine either way. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
