>-----Original Message----- >From: Leon Romanovsky [mailto:leon@xxxxxxxxxx] >Sent: Monday, April 15, 2019 11:21 PM >To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx> >Cc: Dalessandro, Dennis <dennis.dalessandro@xxxxxxxxx>; jgg@xxxxxxxx; >dledford@xxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; Marciniszyn, Mike ><mike.marciniszyn@xxxxxxxxx> >Subject: Re: [PATCH RESEND for-next 5/9] IB/{rdmavt, qib, hfi1}: Use new >routine to release reference counts > >On Mon, Apr 15, 2019 at 08:52:52PM +0000, Ruhl, Michael J wrote: >> >-----Original Message----- >> >From: Leon Romanovsky [mailto:leon@xxxxxxxxxx] >> >Sent: Monday, April 15, 2019 9:13 AM >> >To: Dalessandro, Dennis <dennis.dalessandro@xxxxxxxxx> >> >Cc: jgg@xxxxxxxx; dledford@xxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; >Ruhl, >> >Michael J <michael.j.ruhl@xxxxxxxxx>; Marciniszyn, Mike >> ><mike.marciniszyn@xxxxxxxxx> >> >Subject: Re: [PATCH RESEND for-next 5/9] IB/{rdmavt, qib, hfi1}: Use new >> >routine to release reference counts >> > >> >On Mon, Apr 15, 2019 at 08:31:03AM -0400, Dennis Dalessandro wrote: >> >> On 4/12/2019 11:43 AM, Leon Romanovsky wrote: >> >> > On Fri, Apr 12, 2019 at 06:41:42AM -0700, Dennis Dalessandro wrote: >> >> > > From: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxx> >> >> > > >> >> > > The reference count adjustments on reference count completion >> >> > > are open coded throughout. >> >> > > >> >> > > Add a routine to do all reference count adjustments and use. >> >> > > >> >> > > Reviewed-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx> >> >> > > Signed-off-by: Mike Marciniszyn <mike.marciniszyn@xxxxxxxxx> >> >> > > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxx> >> >> > > >> >> > > --- >> >> > > For some reason patch didn't make the list or patchworks. This >> >> > > is just a re-send. All the rest seem to have made it. >> >> > > --- >> >> > > drivers/infiniband/hw/hfi1/rc.c | 4 ++-- >> >> > > drivers/infiniband/hw/qib/qib_rc.c | 4 ++-- >> >> > > drivers/infiniband/sw/rdmavt/qp.c | 9 ++------- >> >> > > include/rdma/rdmavt_qp.h | 14 ++++++++++++++ >> >> > > 4 files changed, 20 insertions(+), 11 deletions(-) >> >> > > >> >> > > diff --git a/drivers/infiniband/hw/hfi1/rc.c >> >b/drivers/infiniband/hw/hfi1/rc.c >> >> > > index 5ba39a9..a922edc 100644 >> >> > > --- a/drivers/infiniband/hw/hfi1/rc.c >> >> > > +++ b/drivers/infiniband/hw/hfi1/rc.c >> >> > > @@ -1834,7 +1834,7 @@ void hfi1_rc_send_complete(struct rvt_qp >> >*qp, struct hfi1_opa_header *opah) >> >> > > qp->s_last = s_last; >> >> > > /* see post_send() */ >> >> > > barrier(); >> >> > > - rvt_put_swqe(wqe); >> >> > > + rvt_put_qp_swqe(qp, wqe); >> >> > > rvt_qp_swqe_complete(qp, >> >> > > wqe, >> >> > > ib_hfi1_wc_opcode[wqe- >>wr.opcode], >> >> > > @@ -1882,7 +1882,7 @@ struct rvt_swqe *do_rc_completion(struct >> >rvt_qp *qp, >> >> > > u32 s_last; >> >> > > >> >> > > trdma_clean_swqe(qp, wqe); >> >> > > - rvt_put_swqe(wqe); >> >> > > + rvt_put_qp_swqe(qp, wqe); >> >> > > rvt_qp_wqe_unreserve(qp, wqe); >> >> > > s_last = qp->s_last; >> >> > > trace_hfi1_qp_send_completion(qp, wqe, s_last); >> >> > > diff --git a/drivers/infiniband/hw/qib/qib_rc.c >> >b/drivers/infiniband/hw/qib/qib_rc.c >> >> > > index 50dd981..2ac4c67 100644 >> >> > > --- a/drivers/infiniband/hw/qib/qib_rc.c >> >> > > +++ b/drivers/infiniband/hw/qib/qib_rc.c >> >> > > @@ -933,7 +933,7 @@ void qib_rc_send_complete(struct rvt_qp >*qp, >> >struct ib_header *hdr) >> >> > > qp->s_last = s_last; >> >> > > /* see post_send() */ >> >> > > barrier(); >> >> > > - rvt_put_swqe(wqe); >> >> > > + rvt_put_qp_swqe(qp, wqe); >> >> > > rvt_qp_swqe_complete(qp, >> >> > > wqe, >> >> > > ib_qib_wc_opcode[wqe- >>wr.opcode], >> >> > > @@ -975,7 +975,7 @@ static inline void update_last_psn(struct >rvt_qp >> >*qp, u32 psn) >> >> > > qib_cmp24(qp->s_sending_psn, qp->s_sending_hpsn) > 0) { >> >> > > u32 s_last; >> >> > > >> >> > > - rvt_put_swqe(wqe); >> >> > > + rvt_put_qp_swqe(qp, wqe); >> >> > > s_last = qp->s_last; >> >> > > if (++s_last >= qp->s_size) >> >> > > s_last = 0; >> >> > > diff --git a/drivers/infiniband/sw/rdmavt/qp.c >> >b/drivers/infiniband/sw/rdmavt/qp.c >> >> > > index 2460303..31a2e65 100644 >> >> > > --- a/drivers/infiniband/sw/rdmavt/qp.c >> >> > > +++ b/drivers/infiniband/sw/rdmavt/qp.c >> >> > > @@ -623,10 +623,7 @@ static void rvt_clear_mr_refs(struct rvt_qp >*qp, >> >int clr_sends) >> >> > > while (qp->s_last != qp->s_head) { >> >> > > struct rvt_swqe *wqe = >rvt_get_swqe_ptr(qp, qp- >> >>s_last); >> >> > > >> >> > > - rvt_put_swqe(wqe); >> >> > > - if (qp->allowed_ops == IB_OPCODE_UD) >> >> > > - atomic_dec(&ibah_to_rvtah( >> >> > > - wqe->ud_wr.ah)- >>refcount); >> >> > > + rvt_put_qp_swqe(qp, wqe); >> >> > > if (++qp->s_last >= qp->s_size) >> >> > > qp->s_last = 0; >> >> > > smp_wmb(); /* see qp_set_savail */ >> >> > > @@ -2683,9 +2680,7 @@ void rvt_send_complete(struct rvt_qp *qp, >> >struct rvt_swqe *wqe, >> >> > > qp->s_last = last; >> >> > > /* See post_send() */ >> >> > > barrier(); >> >> > > - rvt_put_swqe(wqe); >> >> > > - if (qp->allowed_ops == IB_OPCODE_UD) >> >> > > - atomic_dec(&ibah_to_rvtah(wqe->ud_wr.ah)- >>refcount); >> >> > > + rvt_put_qp_swqe(qp, wqe); >> >> > > >> >> > > rvt_qp_swqe_complete(qp, >> >> > > wqe, >> >> > > diff --git a/include/rdma/rdmavt_qp.h b/include/rdma/rdmavt_qp.h >> >> > > index a00c46a..68e38c2 100644 >> >> > > --- a/include/rdma/rdmavt_qp.h >> >> > > +++ b/include/rdma/rdmavt_qp.h >> >> > > @@ -723,6 +723,20 @@ static inline void rvt_mod_retry_timer(struct >> >rvt_qp *qp) >> >> > > return rvt_mod_retry_timer_ext(qp, 0); >> >> > > } >> >> > > >> >> > > +/** >> >> > > + * rvt_put_qp_swqe - drop refs held by swqe >> >> > > + * @qp: the send qp >> >> > > + * @wqe: the send wqe >> >> > > + * >> >> > > + * This drops any references held by the swqe >> >> > > + */ >> >> > > +static inline void rvt_put_qp_swqe(struct rvt_qp *qp, struct >rvt_swqe >> >*wqe) >> >> > > +{ >> >> > > + rvt_put_swqe(wqe); >> >> > > + if (qp->allowed_ops == IB_OPCODE_UD) >> >> > > + atomic_dec(&ibah_to_rvtah(wqe->ud_wr.ah)- >>refcount); >> >> > > +} >> >> > >> >> > In the context of my allocation change patches, I wanted to ask on >which >> >> > objects should driver perform refcounting? >> >> >> >> Are you asking if we really need to do our own refcounting in the driver >> >> since your patch series went in to for-next? >> > >> >For this AH code - yes, I don't think that refcounting is needed. >> >> Hi Leon, >> >> We do need the reference count because the AH is used by the >asynchronous >> send engine. >> >> I am going to work up a patch to implement the SLEEPABLE path in our >destroy >> to deal with this. > >I see, but it is not SLEEPABLE/not SLEEPABLE paths, there is a need to ensure >that destroy AH is synchronous operation. > >Do you have time expectation when will it be ready? > >It seems like my commit d345691471b4 ("RDMA: Handle AH >allocations by IB/core") assumed that all releases are synchronous. > >There is use-after-free bug now in your driver. Hi Leon, WRT this specific refcount, after further review, I am going to eliminate it in an upcoming patch (hopefully available next week). Because of the issues cleaned up with this patch set, I am basing my patch on the patch set that includes this one (i.e. it is a follow on) I will be looking into the other objects you mentioned (MR and QPs) after the AH refcount is cleaned up to see if I can add any good info there. Thanks, Mike >> >> >However my question is more general - I need to know in which objects >> >we can remove refcounting without any worries that I'll break some driver. >> >> That is tougher to answer, and will need some more research. >> >> Do you have a list of objects in mind to start with? > >The most challengeable are MR and QPs, everything else can definitely be >transformed to be without refcounting. > >> >> Thanks, >> >> Mike >> >> >> >Thanks >> > >> >> >> >> -Denny >> >> >> >> >> >>
