Re: [PATCH for-next 0/2] RDMA/erdma: Introduce custom implementation of drain_sq and drain_rq

[Cheng Xu <chengyou@linux.alibaba.com>]

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.
>