Messages in this thread | | | Date | Tue, 25 May 2021 17:00:57 -0300 | From | Jason Gunthorpe <> | Subject | Re: [PATCH rdma-next v3 8/8] IB/cm: Protect cm_dev, cm_ports and mad_agent with kref and lock |
| |
On Tue, May 11, 2021 at 11:22:12AM +0300, Leon Romanovsky wrote: > @@ -2139,6 +2197,8 @@ static int cm_req_handler(struct cm_work *work) > sa_path_set_dmac(&work->path[0], > cm_id_priv->av.ah_attr.roce.dmac); > work->path[0].hop_limit = grh->hop_limit; > + > + cm_destroy_av(&cm_id_priv->av); > ret = cm_init_av_by_path(&work->path[0], gid_attr, &cm_id_priv->av); > if (ret) { > int err;
Why add cm_destroy_av() here? The cm_id_priv was freshly created at the top of this function and hasn't left the stack frame yet?
> @@ -4419,12 +4486,19 @@ static void cm_remove_one(struct ib_device *ib_device, void *client_data) > * after that we can call the unregister_mad_agent > */ > flush_workqueue(cm.wq); > - ib_unregister_mad_agent(port->mad_agent); > + /* > + * The above ensures no call paths from the work are running, > + * the remaining paths all take the unregistration lock
"unregistration lock" is "mad_agent_lock"
> + */ > + spin_lock(&cm_dev->mad_agent_lock); > + port->mad_agent = NULL; > + spin_unlock(&cm_dev->mad_agent_lock); > + ib_unregister_mad_agent(mad_agent); > cm_remove_port_fs(port); > - kfree(port); > } > > - kfree(cm_dev); > + /* All touches can only be on call path from the work */
Not sure anymore what this comment means, the work was flushed? I think it is saying all touches can only be on a place outside the work.
Other than these little details it all looks OK
Jason
| |