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

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


>>> Many upper layers depend on this.
>>
>> For kernel verbs, erdma currently supports NoF. we tested the NoF cases,
>> and found that it's never happened that newly WRs were posted after QP
>> changed to error, and the drain_qp should be the terminal of IO process.
>>
>> Also, in userspace, I find that many providers prevents new WRs if QP state is
>> not right.
> 
> Sure, but your new STOPPED bits don't propagate up to userspace, right?

Yes. they are only used for kernel QPs. The bits are used for setting an accurate
time point to prevent newly WRs when modify qp to error.

> The post calls will fail unexpectedly, because the QP is not yet in
> ERROR state.

Do you mean this happens in userspace or kernel? The new bits do not influence
userspace, and erdma will have the same behavior with other providers in userspace.

For kernel, this is only used in drain_qp scenario. posting WRs and drain qp
concurrently will lead uncertain results. This is also mentioned in the comment
of ib_drain_qp:

 * ensure that there are no other contexts that are posting WRs concurrently.
 * Otherwise the drain is not guaranteed.

> I'm also concerned how consumers who are posting their own "drain" WQEs
> will behave. This is a common approach that many already take. But now
> they will see different behavior when posting them...
> 

For userspace, erdma is not special.
For kernel, I think the standard way to drain WR is using ib_drain_qp, not custom
implementation. I'm not sure that there is some ULPs in kernel has their own drain
flow.

Thanks,
Cheng Xu

> Tom.
> 
> 
>>
>> So, it seems ok in practice.
>>
>> Thanks,
>> Cheng Xu
>>
>>
>> [1] http://www.rdmaconsortium.org/home/draft-hilland-iwarp-verbs-v1.0-RDMAC.pdf
>>
>>