On Mon, Jan 14, 2019 at 08:26:20PM +0200, Leon Romanovsky wrote: > On Mon, Jan 14, 2019 at 05:36:52PM +0000, Jason Gunthorpe wrote: > > On Sun, Jan 13, 2019 at 10:26:27AM +0200, Leon Romanovsky wrote: > > > On Sat, Jan 12, 2019 at 05:05:49PM +0000, Jason Gunthorpe wrote: > > > > On Sat, Jan 12, 2019 at 05:06:02PM +0200, Leon Romanovsky wrote: > > > > > On Fri, Jan 11, 2019 at 07:31:24PM -0700, Jason Gunthorpe wrote: > > > > > > From: Jason Gunthorpe <jgg@xxxxxxxxxxxx> > > > > > > > > > > > > Work must hold a kref on the ib_device otherwise the dev pointer can > > > > > > become free before the work runs. > > > > > > > > > > How can it occur exactly? > > > > > > > > > > "dev" will be released after mlx5_ib will exist, mlx5_ib exist will > > > > > drain advise_mr queue before it. Such drain will ensure that no works > > > > > are running. > > > > > > > > Okay, that is fine for the get_device, it can be removed. > > > > > > > > Are we OK with the reg_state hunk? > > > > > > I'm not sure about that > > > Maybe current code is not nice as you suggested but IB_DEV_REGISTERED > > > check also valid approach. > > > > It is not. Checking the state without holding the appropriate locks > > does nothing useful. > > > > We need to delete the test or properly hold the registration lock. > > > > > More general, I don't like how IB/core handles states and IMHO we should > > > push our stack to be similar to block layer elevator. Block layer implements > > > clear stages and calls to callbacks provided by drivers. It removes the need > > > to manage and check any state in those drivers. > > > > I'm not super familiar with the block layer, but I don't think more > > callbacks will be in anyway more clear here. We have a very > > complicated lifetime model already.. > > The idea is to provide very clear device state machine which will ensure > that once driver was called, IB/core won't change state without need to > annotate driver code with various get/put. We do that. Today the driver is in control of the lifetime of both the memory and the 'registration', as it is the one that calls ib_device_unregister and the badly named ib_dealloc The registration lock is a tool the driver can use to prevent progress through unregistration in cases where it cares about that. This is much better than having every driver try and build their own lock, as I saw in bxnt and rxe for instance. Jason
