Messages in this thread | | | Date | Wed, 29 Nov 2023 01:08:39 -0800 | From | Saeed Mahameed <> | Subject | Re: [PATCH V3 2/5] misc: mlx5ctl: Add mlx5ctl misc driver |
| |
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.
>Also, why a rw_semaphore? Only use those if you can prove with a >benchmark that it is actually faster, otherwise it's simpler to just use >a normal mutex (hint, you are changing the fields in the structure with >the read lock held, which feels very wrong, and so needs a LOT of >documentation, or just use a normal mutex...) >
It is needed so we can protect against underlaying device unloading while miscdevice is active, we use rw semaphore since we don't care about synchronization between any of the fops, but we want to protect current active ioctls and fops from sudden mlx5ctl_remove (auxiliary_driver.remove) which can happen dynamically due to underlaying device removal.
So here is the locking scheme:
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.
>thanks, > >greg k-h
| |