On Mon, Feb 11, 2019 at 04:20:53PM +0000, Bernard Metzler wrote:
>
> >To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
> >From: "Jason Gunthorpe" <jgg@xxxxxxxx>
> >Date: 01/31/2019 06:22PM
> >Cc: linux-rdma@xxxxxxxxxxxxxxx
> >Subject: Re: [PATCH v4 03/13] SIW network and RDMA core interface
> >
> >On Thu, Jan 31, 2019 at 03:24:01PM +0000, Bernard Metzler wrote:
> >
> >> >> +static void siw_netdev_unregistered(struct work_struct *work)
> >> >> +{
> >> >> + struct siw_device *sdev = container_of(work, struct
> >siw_device,
> >> >> + netdev_unregister);
> >> >> +
> >> >> + struct siw_qp_attrs qp_attrs;
> >> >> + struct list_head *pos, *tmp;
> >> >> +
> >> >> + memset(&qp_attrs, 0, sizeof(qp_attrs));
> >> >> + qp_attrs.state = SIW_QP_STATE_ERROR;
> >> >> +
> >> >> + /*
> >> >> + * Mark all current QP's of this device dead
> >> >> + */
> >> >> + list_for_each_safe(pos, tmp, &sdev->qp_list) {
> >> >> + struct siw_qp *qp = list_entry(pos, struct siw_qp, devq);
> >> >> +
> >> >> + down_write(&qp->state_lock);
> >> >> + (void) siw_qp_modify(qp, &qp_attrs, SIW_QP_ATTR_STATE);
> >> >> + up_write(&qp->state_lock);
> >> >> + }
> >> >
> >> >Don't put code around ib_unregister. If you need to do things use
> >a
> >> >callback at the right moment in the unregister flow. You might
> >need
> >> >to
> >> >add a callback (see my latest series)
> >>
> >> I thought by having that executed in a work queue context only
> >> we'd be safe. I have to bring down all related QP's if a network
> >device
> >> suddenly disappears... Looks like I have to better understand the
> >> intended unregister flow.
> >
> >If it is important it should be done during module unload and link
> >removal, if it is not important, it shouldn't be done :)
>
> If siw catches a NETDEV_UNREGISTER event, it shall first
> close all related connection endpoints before the ib_device
> can go away.
For things to work properly you can't do any steps outside the
unregister and its callbacks.
NETDEV_UNREGISTER events should only call
ib_unregsiter_device_queued() and nothing more.
> Following your new device management design, I would now
> first schedule local work to cleanup those siw private
> resources, and later ib_unregister_device_queued(),
You should not schedule work.
Drivers should be designed so that when all PD's are closed most of
their resources are gone as well. The core code will ensure all PD's
are closed.
If there are internal driver PD/etcs then we should add a callback to
clean them up right after the clients are cleaned up.
> or is it OK to do that local cleanup stuff within the
> dealloc_driver() method?
dealloc_driver should free memory, and do any final fences against
async work. It shouldn't spawn new work.
It is an appropriate place to free socket resources and other related
things.
Jason
