Re: Re: [PATCH v3] iwcm: don't hold the irq disabled lock on iw_rem_ref

[Krishnamraju Eraparaju <krishna2@chelsio.com>]

Hi Steve & Bernard,

Thanks for the review comments.
I will do those formating changes.

Thanks,
Krishna.
On Wednesday, September 09/11/19, 2019 at 20:12:43 +0530, Steve Wise wrote:
> On Wed, Sep 11, 2019 at 4:38 AM Bernard Metzler <BMT@zurich.ibm.com> wrote:
> >
> > -----"Krishnamraju Eraparaju" <krishna2@chelsio.com> wrote: -----
> >
> > >To: "Sagi Grimberg" <sagi@grimberg.me>, "Steve Wise"
> > ><larrystevenwise@gmail.com>, "Bernard Metzler" <BMT@zurich.ibm.com>
> > >From: "Krishnamraju Eraparaju" <krishna2@chelsio.com>
> > >Date: 09/10/2019 09:22PM
> > >Cc: "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>, "Jason
> > >Gunthorpe" <jgg@ziepe.ca>
> > >Subject: [EXTERNAL] Re: [PATCH v3] iwcm: don't hold the irq disabled
> > >lock on iw_rem_ref
> > >
> > >Please review the below patch, I will resubmit this in patch-series
> > >after review.
> > >- As kput_ref handler(siw_free_qp) uses vfree, iwcm can't call
> > >  iw_rem_ref() with spinlocks held. Doing so can cause vfree() to
> > >sleep
> > >  with irq disabled.
> > >  Two possible solutions:
> > >  1)With spinlock acquired, take a copy of "cm_id_priv->qp" and
> > >update
> > >    it to NULL. And after releasing lock use the copied qp pointer
> > >for
> > >    rem_ref().
> > >  2)Replacing issue causing vmalloc()/vfree to kmalloc()/kfree in SIW
> > >    driver, may not be a ideal solution.
> > >
> > >  Solution 2 may not be ideal as allocating huge contigous memory for
> > >   SQ & RQ doesn't look appropriate.
> > >
> > >- The structure "siw_base_qp" is getting freed in siw_destroy_qp(),
> > >but
> > >  if cm_close_handler() holds the last reference, then siw_free_qp(),
> > >  via cm_close_handler(), tries to get already freed "siw_base_qp"
> > >from
> > >  "ib_qp".
> > >   Hence, "siw_base_qp" should be freed at the end of siw_free_qp().
> > >
> >
> > Regarding the siw driver, I am fine with that proposed
> > change. Delaying freeing the base_qp is OK. In fact,
> > I'd expect the drivers soon are passing that responsibility
> > to the rdma core anyway -- like for CQ/SRQ/PD/CTX objects,
> > which are already allocated and freed up there.
> >
> > The iwcm changes look OK to me as well.
> >
> 
> Hey Krishna,  Since the iwcm struct/state is still correctly being
> manipulated under the lock, then I think it this patch correct.  Test
> the heck out of it. :)
> 
> Steve.