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.
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.
The responder using the state machine is easy to understand and maintain.
TBH, I am not inclined to change the design, but would like to know the
opinions of other people.
>
> > + }
>
> <...>
>
> > +#else
> > +int rxe_odp_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
> > +{
> > + return RESPST_ERR_UNSUPPORTED_OPCODE;
> > +}
> > +#endif
>
> You already have empty declaration in rxe_loc.h, use it.
That's right. I will change it.
Thanks,
Daisuke
>
> Thanks
