Re: [PATCH V3 2/5] misc: mlx5ctl: Add mlx5ctl misc driver

[Greg Kroah-Hartman]

On Wed, Nov 29, 2023 at 09:02:00AM -0400, Jason Gunthorpe wrote:
> On Wed, Nov 29, 2023 at 09:20:32AM +0000, Greg Kroah-Hartman wrote:
> > On Wed, Nov 29, 2023 at 01:08:39AM -0800, Saeed Mahameed wrote:
> > > On 27 Nov 18:59, Greg Kroah-Hartman wrote:
> > > > On Mon, Nov 20, 2023 at 11:06:16PM -0800, Saeed Mahameed wrote:
> > > > > +struct mlx5ctl_dev {
> > > > > +	struct mlx5_core_dev *mdev;
> > > > > +	struct miscdevice miscdev;
> > > > > +	struct auxiliary_device *adev;
> > > > > +	struct list_head fd_list;
> > > > > +	spinlock_t fd_list_lock; /* protect list add/del */
> > > > > +	struct rw_semaphore rw_lock;
> > > > > +	struct kref refcount;
> > > > 
> > > > You now have 2 different things that control the lifespan of this
> > > > structure.  We really need some way to automatically check this so that
> > > > people don't keep making this same mistake, it happens all the time :(
> > > > 
> > > > Please pick one structure (miscdevice) or the other (kref) to control
> > > > the lifespan, having 2 will just not work.
> > > > 
> > > 
> > > miscdevice doesn't handle the lifespan, open files will remain open even
> > > after the miscdevice was unregistered, hence we use the kref to defer the
> > > kfree until the last open file is closed.
> > 
> > miscdevice has a reference counter and a lifecycle, you can not have two
> > reference counted objects in the same structure and expect things to
> > work well.
> 
> This second refcount is hidden well:
> 
> struct miscdevice {
> 	int minor;
> 	const char *name;
> 	const struct file_operations *fops;
> 	struct list_head list;
> 	struct device *parent;
> 	struct device *this_device;
> 	const struct attribute_group **groups;
> 	const char *nodename;
> 	umode_t mode;
> };

Ugh, you are right, I was wrong, there is no reference count here, using
a miscdevice _requires_ you to have a separate reference count, like you
all did.  My fault.

> > > write_lock() : only on mlx5_ctl remove and mark the device is down
> > > via assigning NULL to mcdev->dev, to let all new readers abort and to wait
> > > for current readers to finish their task.
> > > 
> > > read_lock(): used in all fops and ioctls, to make sure underlaying
> > > mlx5_core device is still active, and to prevent open files to access the
> > > device when miscdevice was already unregistered.
> > > 
> > > I agree, this should've been documented in the code, I will add
> > > documentation.
> > 
> > Just make it simple and use a normal mutex please.
> 
> A normal mutex would make the entire ioctl interface single threaded,
> this is not desirable.

Why not?  It's an ioctl for a single device, surely this isn't
performance criticial.  And then only grab it when needed, on
read/write/ioctl path it shouldn't be needed at all due to the proper
reference counting of the structures.  Only on open/close, right?

And again, for a rw semaphore, benchmarks matter, often, if not almost
always, a normal mutex is faster for stuff like this.  If not, then a
benchmark will show it.

> > But before you do that, please see my other email about why not using
> > devlink for all of this instead.
> 
> We've been over this already, the devlink discussion is about some
> configuration stuff.

It was?  I see device-specific diagonostic data for the mlx5 driver
being exported through devlink today, that's not configuration.  Why not
just add more?

> It has never been suggested to cover the debug interface. This series
> is primarily about debug, the devlink thing is a distraction to main
> point.

For me it is the main point at the moment.  Please explain why devlink
does not work for the information that you have created a misc device
where you want an ioctl api instead, as I honestly do not understand.

thanks,

greg k-h