> -----Original Message----- > From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- > owner@xxxxxxxxxxxxxxx] On Behalf Of Doug Ledford > Sent: Monday, February 04, 2019 5:12 PM > To: Dalessandro, Dennis <dennis.dalessandro@xxxxxxxxx>; jgg@xxxxxxxx > Cc: linux-rdma@xxxxxxxxxxxxxxx; Marciniszyn, Mike > <mike.marciniszyn@xxxxxxxxx>; Wan, Kaike <kaike.wan@xxxxxxxxx> > Subject: Re: [PATCH for-next 14/17] IB/hfi1: Integrate TID RDMA READ > protocol into RC protocol > > On Wed, 2019-01-23 at 19:32 -0800, Dennis Dalessandro wrote: > > +error_qp: > > + spin_unlock_irqrestore(&qp->s_lock, ps->flags); > > + spin_lock_irqsave(&qp->r_lock, ps->flags); > > + spin_lock(&qp->s_lock); > > + rvt_error_qp(qp, IB_WC_WR_FLUSH_ERR); > > + spin_unlock(&qp->s_lock); > > + spin_unlock_irqrestore(&qp->r_lock, ps->flags); > > + spin_lock_irqsave(&qp->s_lock, ps->flags); > > That's just ugly. I get that you have lock ordering that you have to obey, but > have you considered doing a spin_trylock and only doing the unwind/rewind > if it fails? It might not matter much here on the QP error path, but if you > have this same unwind/rewind elsewhere in the driver in what amounts to > more of a hot path... > It's ugly indeed. However, if I use spin_trylock, it will even be more ugly: after calling rvt_err_qp(), we need to unlock qp->r_lock. Then we have to decide whether we get here from grabbing s_lock first or r_lock first, and subsequently use either spin_unlock or spin_unlock_irqrestore to unlock r_lock, and whether we need to unlock and then acquire s_lock. As you said, we are in an error path and we don't need to worry about performance that much. We want to keep the code simpler (even though ugly). Kaike > -- > Doug Ledford <dledford@xxxxxxxxxx> > GPG KeyID: B826A3330E572FDD > Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
