On Sat, Jan 12, 2019 at 10:32:54AM -0700, Parav Pandit wrote: > > > > From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma- > > owner@xxxxxxxxxxxxxxx> On Behalf Of Jason Gunthorpe > > Sent: Saturday, January 12, 2019 11:06 AM > > To: Leon Romanovsky <leon@xxxxxxxxxx> > > Cc: linux-rdma@xxxxxxxxxxxxxxx; Moni Shoua <monis@xxxxxxxxxxxx> > > Subject: Re: [PATCH for-rc 2/2] IB/mlx5: Fix how advise_mr() launches async > > work > > > > 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? > > > > Jason > > Unrelated to your question, I noticed that advise_mr does > uverbs_attr_ptr_get_array_size() for the SGEs. I think this is > incorrect. It should have been mr handles whose ownership can be > check against the PID or ucontext that registered those MRs. The SGE's have lkeys in them, and the lkeys are local to the PD passed in as argument. So the caller cannot touch resources outside the PD, hopefully the driver implementation checks this.. > Who enqueues work to dev->advise_mr_wq? schedule_work() in > mlx5_ib_advise_mr_prefetch() does to global workqueue. Ah! I *thought* I checked this, yes, using the system work queue is why I added the put_device :) > It should be done to advise_mr_wq(). This will give chance to flush > the wq when IB device is unregistered by the core. Good question - Moni?? Jason
