> -----Original Message----- > From: Doug Ledford [mailto:dledford@xxxxxxxxxx] > Sent: Monday, February 04, 2019 2:20 PM > To: Wan, Kaike <kaike.wan@xxxxxxxxx>; Dalessandro, Dennis > <dennis.dalessandro@xxxxxxxxx>; jgg@xxxxxxxx > Cc: Dixit, Ashutosh <ashutosh.dixit@xxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx; > Mitko Haralanov <mitko.haralanov@xxxxxxxxx>; Marciniszyn, Mike > <mike.marciniszyn@xxxxxxxxx> > Subject: Re: [PATCH for-next 02/17] IB/hfi1: TID RDMA flow allocation > > On Mon, 2019-02-04 at 19:05 +0000, Wan, Kaike wrote: > > > > > > and I see why you > > > have to drop the lock before calling it as it will take the lock > > > itself. But that raises the question of why you are taking the lock > > > here? Are you trying to flush any concurrent accessors out before > > > you test? If so, do you need the exp_lock too? And even if you are > > > flushing out concurrent access, what's to prevent a new concurrent > access as soon as you drop the lock? > > qp->s_flags is modified all over the hfi1 driver by both the sending and > receiving sides and we generally access it (read or modify it) only after > acquiring the qp->s_lock to prevent obtaining any incorrect state information. > > That doesn't directly address my point. Any time you take a test for a > condition that needs to be under a lock, and then put the resulting action you > take as a result of that test outside of the lock, there is an inherent flaw in > your locking strategy that must be overcome in some way. As you say "only > after acquiring the qp->s_lock to prevent obtaining any incorrect state > information", but the second you drop the lock, all guarantees about that > state information being correct are lost. So what I'm looking for here is an > explanation of how/why it is safe in this context for you to call hfi1_do_send() > if, in fact, you get interrupted right after dropping the lock and something > happens to clear out HFI1_S_WAIT_TID_SPACE (or, alternatively, why it's > impossible for such a thing to happen). I don't want to have to take the time > to completely reverse engineer how you guys do the locking in your driver, > but when you do something that is a known wrong/dangerous thing unless > there are non-obvious guarantees that make it safe, then I'd like an > explanation from you as to why/how it's safe. That's fair. The explanation is the following: - A qp will be put on a waiting list when tid resources (tid flow or tid entries) are not available and the HFI1_S_WAIT_TID_SPACE bit will be set in qp->s_flags under the protection of qp->s_lock. - When the tid resources are freed by another qp, the function tid_rdma_trigger_resume (the trigger work item) will be scheduled/invoked for the qp at the head of the waiting list. As the function shows, it will do nothing if HFI1_S_WAIT_TID_SPACE is cleared. Otherwise, hfi1_do_send(the send engine) will be called for this qp. At this time, the qp is not removed from the waiting list; - The send engine will try to send any request or responses after grabbing the qp->s_lock. During this process, it will try to allocate the tid resources. If it succeeds, it will remove the qp from the waiting list and clear HFI1_S_WAIT_TID_SPACE bit. Otherwise, the send engine will quit while the qp remains on the waiting list. If resources are available, the send engine will continuously send requests/responses in a loop. As described above, the flag HFI1_S_WAIT_TID_SPACE is not required for hfi1_do_send() to function properly. If it is cleared by another process for the qp, the qp must have been removed from the waiting list and the send engine simply tries to send another request or response if the tid resources are still available. This flag is only important for managing the qp on the waiting list or resuming the send engine, but has little if any to do with how the send engine works. Hopefully that is clear. Kaike > > -- > Doug Ledford <dledford@xxxxxxxxxx> > GPG KeyID: B826A3330E572FDD > Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
