Messages in this thread | | | Date | Wed, 29 Nov 2023 15:41:54 +0000 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH V3 2/5] misc: mlx5ctl: Add mlx5ctl misc driver |
| |
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
| |