Re: [PATCH for-next v5 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()

[Bob Pearson <rpearsonhpe@gmail.com>]

On 1/17/23 09:11, Jason Gunthorpe wrote:
> On Mon, Jan 16, 2023 at 05:56:01PM -0600, Bob Pearson wrote:
> 
>> +/* only implemented for 64 bit architectures */
>> +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
>> +{
>> +#if defined CONFIG_64BIT
> 
> #ifdef
> 
> It is a little more typical style to provide an alternate version of
> the function when #ifdefing

I will do that.
> 
>> +	u64 *va;
>> +
>> +	/* See IBA oA19-28 */
>> +	if (unlikely(mr->state != RXE_MR_STATE_VALID)) {
>> +		rxe_dbg_mr(mr, "mr not in valid state");
>> +		return -EINVAL;
>> +	}
>> +
>> +	va = iova_to_vaddr(mr, iova, sizeof(value));
>> +	if (unlikely(!va)) {
>> +		rxe_dbg_mr(mr, "iova out of range");
>> +		return -ERANGE;
>> +	}
>> +
>> +	/* See IBA A19.4.2 */
>> +	if (unlikely((uintptr_t)va & 0x7 || iova & 0x7)) {
>> +		rxe_dbg_mr(mr, "misaligned address");
>> +		return -RXE_ERR_NOT_ALIGNED;
>> +	}
>> +
>> +	/* Do atomic write after all prior operations have completed */
>> +	smp_store_release(va, value);
>> +
>> +	return 0;
>> +#else
>> +	WARN_ON(1);
>> +	return -EINVAL;
>> +#endif
>> +}
>> +
>>  int advance_dma_data(struct rxe_dma_info *dma, unsigned int length)
>>  {
>>  	struct rxe_sge		*sge	= &dma->sge[dma->cur_sge];
>> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
>> index 1e38e5da1f4c..49298ff88d25 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
>> @@ -764,30 +764,40 @@ static enum resp_states atomic_reply(struct rxe_qp *qp,
>>  	return RESPST_ACKNOWLEDGE;
>>  }
>>  
>> -#ifdef CONFIG_64BIT
>> -static enum resp_states do_atomic_write(struct rxe_qp *qp,
>> -					struct rxe_pkt_info *pkt)
>> +static enum resp_states atomic_write_reply(struct rxe_qp *qp,
>> +					   struct rxe_pkt_info *pkt)
>>  {
>> -	struct rxe_mr *mr = qp->resp.mr;
>> -	int payload = payload_size(pkt);
>> -	u64 src, *dst;
>> -
>> -	if (mr->state != RXE_MR_STATE_VALID)
>> -		return RESPST_ERR_RKEY_VIOLATION;
>> +	struct resp_res *res = qp->resp.res;
>> +	struct rxe_mr *mr;
>> +	u64 value;
>> +	u64 iova;
>> +	int err;
>>  
>> -	memcpy(&src, payload_addr(pkt), payload);
>> +	if (!res) {
>> +		res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_WRITE_MASK);
>> +		qp->resp.res = res;
>> +	}
>>  
>> -	dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset, payload);
>> -	/* check vaddr is 8 bytes aligned. */
>> -	if (!dst || (uintptr_t)dst & 7)
>> -		return RESPST_ERR_MISALIGNED_ATOMIC;
>> +	if (res->replay)
>> +		return RESPST_ACKNOWLEDGE;
>>  
>> -	/* Do atomic write after all prior operations have completed */
>> -	smp_store_release(dst, src);
>> +	mr = qp->resp.mr;
>> +	value = *(u64 *)payload_addr(pkt);
>> +	iova = qp->resp.va + qp->resp.offset;
>>  
>> -	/* decrease resp.resid to zero */
>> -	qp->resp.resid -= sizeof(payload);
>> +#if defined CONFIG_64BIT
> 
> Shouldn't need a #ifdef here

This avoids a new special error (i.e. NOT_64_bit) and makes it clear we
won't call the code in mr.

I really don't understand why Fujitsu did it all this way instead of just
using a spinlock for 32 bit architectures as a fallback. But if I want to
keep to the spirit of their implementation this is fairly clear I think.

Bob
> 
>> +	err = rxe_mr_do_atomic_write(mr, iova, value);
>> +	if (unlikely(err)) {
>> +		if (err == -RXE_ERR_NOT_ALIGNED)
>> +			return RESPST_ERR_MISALIGNED_ATOMIC;
>> +		else
>> +			return RESPST_ERR_RKEY_VIOLATION;
> 
> Again why not return the RESPST directly then the stub can return
> RESPST_ERR_UNSUPPORTED_OPCODE?
> 
> Jason