Re: [PATCH 01/19] char_dev: replace cdev_map with an xarray

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Aug 27, 2020 at 10:53:53AM +0200, Christoph Hellwig wrote:
> On Wed, Aug 26, 2020 at 10:19:05AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Aug 26, 2020 at 08:24:28AM +0200, Christoph Hellwig wrote:
> > > None of the complicated overlapping regions bits of the kobj_map are
> > > required for the character device lookup, so just a trivial xarray
> > > instead.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > Really?  This is ok to use and just as fast?  If so, wonderful, it would
> > be great to clean up kobj_map users.
> 
> Xarrays provide pretty efficient as long as we have a unsigned long
> or smaller index (check, dev_t is small) and the indices are reasonable
> clustered (check, minors for the same major).  Memory usage will go down
> vs the probes, and lookup speed up.

Ok, great!

xarrays weren't around when this code was written (back in the 2.5
days).

> > > +	mutex_lock(&chrdevs_lock);
> > > +	for (i = 0; i < count; i++) {
> > > +		error = xa_insert(&cdev_map, dev + i, p, GFP_KERNEL);
> > > +		if (error)
> > > +			goto out_unwind;
> > > +	}
> > > +	mutex_unlock(&chrdevs_lock);
> > >  
> > >  	kobject_get(p->kobj.parent);
> > 
> > Can't you drop this kobject_get() too?
> 
> I'll have to drop it or add back the put on the delete side.  And
> I think the latter is safer for now, because..
> 
> > 
> > And also the "struct kobj" in struct cdev can be gone as well, as the
> > kobj_map was the only "real" user of this structure.  I know some
> > drivers liked to touch that field as well, but it never actually did
> > anything for them, so it was pointless, but it will take some 'make
> > allmodconfig' builds to flush them out.
> 
> I looked at it, but it does get registered and shows up in sysfs.

It does?  Where does that happen?  I see a bunch of kobject_init()
calls, but nothing that registers it in sysfs that I can see.

Note, this is not the kobject that shows up in /sys/dev/char/ as a
symlink, that comes from the driver core logic and is independent of the
cdev code.

The kobject does handle the structure lifetime rules, but that should be
able to be replaced with a simple kref instead.

> I don't really dare to touch this for now, as it can have huge
> implications.  Better done in a separate series (if we can actually do
> it at all).

Fair enough, I will be willing to tackle that once this gets merged, so
this is fine as-is.

thanks,

greg k-h