Re: [PATCH for-next v2 2/2] RDMA/rxe: Enable ODP in ATOMIC WRITE operation

[Leon Romanovsky <leon@kernel.org>]

On Wed, Mar 19, 2025 at 02:58:51AM +0000, Daisuke Matsuda (Fujitsu) wrote:
> On Tue, Mar 18, 2025 7:10 PM Leon Romanovsky wrote:
> > On Tue, Mar 18, 2025 at 06:49:32PM +0900, Daisuke Matsuda wrote:
> > 
> > <...>
> > 
> > > +static inline int rxe_odp_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
> > > +{
> > > +	return RESPST_ERR_UNSUPPORTED_OPCODE;
> > > +}
> > >  #endif /* CONFIG_INFINIBAND_ON_DEMAND_PAGING */
> > 
> > You are returning "enum resp_states", while function expects to return "int". You should return -EOPNOTSUPP.
> 
> Other than my patches, there are some functions that do the same thing.

Yes, but you are adding new code and in the new code you should try to
have correlated function declaration and returned values.

> I would like to post a patch to make them consistent, but I think we need
> reach an agreement on the design of rxe responder before taking up.
> Please see my opinion below.
> 
> > 
> > >
> > >  #endif /* RXE_LOC_H */
> 
> <...>
> 
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_odp.c b/drivers/infiniband/sw/rxe/rxe_odp.c
> > > index 9a9aae967486..f3443c604a7f 100644
> > > --- a/drivers/infiniband/sw/rxe/rxe_odp.c
> > > +++ b/drivers/infiniband/sw/rxe/rxe_odp.c
> > > @@ -378,3 +378,56 @@ int rxe_odp_flush_pmem_iova(struct rxe_mr *mr, u64 iova,
> > >
> > >  	return 0;
> > >  }
> > > +
> > > +#if defined CONFIG_64BIT
> > > +/* only implemented or called for 64 bit architectures */
> > > +int rxe_odp_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
> > > +{
> > > +	struct ib_umem_odp *umem_odp = to_ib_umem_odp(mr->umem);
> > > +	unsigned int page_offset;
> > > +	unsigned long index;
> > > +	struct page *page;
> > > +	int err;
> > > +	u64 *va;
> > > +
> > > +	/* See IBA oA19-28 */
> > > +	err = mr_check_range(mr, iova, sizeof(value));
> > > +	if (unlikely(err)) {
> > > +		rxe_dbg_mr(mr, "iova out of range\n");
> > > +		return RESPST_ERR_RKEY_VIOLATION;
> > 
> > Please don't redefine returned errors.
> 
> As a general principle, I think your comment is totally correct.
> The problem is that rxe_receiver(), the responder of rxe, is originally designed
> as a state machine, and the returned values of "enum resp_states" are used
> to specify the next state.
> 
> One thing to note is that rxe_receiver() run solely in workqueue, so the errors
> generated in the bottom half context are never returned to userspace. In that regard,
> I think redefining the error codes with different enum values can be justified.

In places where rxe_odp_do_atomic_write() respond is important, you can
write something like:
err = rxe_odp_do_atomic_write(...)
if (err == -EPERM)
   state = RESPST_ERR_RKEY_VIOLATION
...

or declare rxe_odp_do_atomic_write() to return enum resp_state.

Thanks