On 8/26/22 9:11 PM, Tom Talpey wrote: > On 8/25/2022 11:21 PM, Cheng Xu wrote: >> On 8/26/22 12:37 AM, Tom Talpey wrote: >>> On 8/24/2022 9:54 PM, Cheng Xu wrote: >>>> >>>> >>>> On 8/24/22 10:08 PM, Tom Talpey wrote: >>>>> On 8/24/2022 5:42 AM, Cheng Xu wrote: >>>>>> Hi, >>>>>> >>>>>> This series introduces erdma's implementation of drain_sq and drain_rq. >>>>>> Our hardware will stop processing any new WRs if QP state is error. >>>>> >>>>> Doesn't this violate the IB specification? Failing newly posted WRs >>>>> before older WRs have flushed to the CQ means that ordering is not >>>>> preserved. >>>> >>>> I agree with Bernard's point. >>>> >>>> I'm not very familiar with with IB specification. But for RNIC/iWarp [1], >>>> post WR in Error state has two optional actions: "Post WQE, and then Flush it" >>>> or "Return an Immediate Error" (Showed in Figure 10). So, I think failing >>>> newly posted WRs is reasonable. >>> >>> <...> But the QP can only enter ERROR once the >>> SQ and RQ are fully drained to the CQ(s). Until that happens, the >>> WRs need to flush through. >>> >> >> Emm, let's put erdma aside first, it seems that specification does not require >> this. According to "6.2.4 Error State" in the document [1]: >> >> The following is done on entry into the Error state: >> * The RI MUST flush any incomplete WRs on the SQ or RQ. >> ..... >> * At some point in the execution of the flushing operation, the RI >> MUST begin to return an Immediate Error for any attempt to post >> a WR to a Work Queue; >> .... >> >> As the second point says, The flushing operation and the behavior of returning >> Immediate Error are asynchronous. what you mentioned is not guaranteed. Failing >> the post_send/post_recv may happens at any time during modify_qp to error. >> >> [1] http://www.rdmaconsortium.org/home/draft-hilland-iwarp-verbs-v1.0-RDMAC.pdf > > Well, that language is very imprecise, "at some point" is not exactly > definitive. I'll explain one scenario that makes it problematic. > >>> Your code seems to start failing WR's when the TX_STOPPED or RX_STOPPED >>> bits are set. But that bit is being set when the drain *begins*, not >>> when it flushes through. That seems wrong, to me. >>> >> >> Back to erdma's scenario, As I explains above, I think failing immediately when >> flushing begins does not violate the specification. > > Consider a consumer which posts with a mix of IB_SEND_SIGNALED and > also unsignaled WRs, for example, fast-memory registration followed > by a send, a very typical storage consumer operation. > > - post_wr(memreg, !signaled) => post success > - => operation success, no completion generated > - ... <= provider detects error here > - post_wr(send, signaled) => post fail (new in your patch) > - ... <= provider notifies async error, etc. > > The consumer now knows there's an error, and needs to tear down. > It must remove the DMA mapping before proceeding, but the hardware > may still be using it. How does it determine the status of that > first post_wr, so it may proceed? > > The IB spec explicitly states that the post verb can only return > the immediate error after the QP has exited the ERROR state, which > includes all pending WRs having been flushed and made visible on > the CQ. Here is an excerpt from the Post Send Request section > 11.4.1.1 specifying its output modifiers: > > -> Invalid QP state. > -> Note: This error is returned only when the QP is in the Reset, > -> Init, or RTR states. It is not returned when the QP is in the Error > -> or Send Queue Error states due to race conditions that could > -> result in indeterminate behavior. Work Requests posted to the > -> Send Queue while the QP is in the Error or Send Queue Error > -> states are completed with a flush error. > Get it, thanks. The IB spec seems to be more clear. > So, the consumer will post a new, signaled, work request, and wait > for it to "flush through" by polling the CQ. Because WR's always > complete in-order, this final completion must appear after *all* > prior WR's, and this gives the consumer the green light to proceed. > Yeah, this is right, and the default ib_drain_qp do it in this way. > With your change, ERDMA will pre-emptively fail such a newly posted > request, and generate no new completion. The consumer is left in limbo > on the status of its prior requests. Providers must not override this. For the ULPs that do not use ib_drain_qp interface, we will have problem. But currently it seems that almost all the ULPs in kernel call ib_drain_qp to finish the drain flow. While ib_drain_qp allows vendors to have custom ib_drain_qp implementations which is invisible to ULPs. Thanks, Cheng Xu > Tom. >
