Re: [PATCH for-next v8 1/6] RDMA/rxe: Make MR functions accessible from other rxe source code

[Zhu Yanjun]

在 2024/10/10 15:24, Daisuke Matsuda (Fujitsu) 写道:
> On Wed, Oct 9, 2024 11:13 PM Zhu Yanjun wrote:
> > 在 2024/10/9 9:58, Daisuke Matsuda 写道:
> > > Some functions in rxe_mr.c are going to be used in rxe_odp.c, which is to
> > > be created in the subsequent patch. List the declarations of the functions
> > > in rxe_loc.h.
> > >
> > > Signed-off-by: Daisuke Matsuda <matsuda-daisuke@xxxxxxxxxxx>
> > > ---
> > >    drivers/infiniband/sw/rxe/rxe_loc.h |  8 ++++++++
> > >    drivers/infiniband/sw/rxe/rxe_mr.c  | 11 +++--------
> > >    2 files changed, 11 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> > > index ded46119151b..866c36533b53 100644
> > > --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> > > +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> > > @@ -58,6 +58,7 @@ int rxe_mmap(struct ib_ucontext *context, struct vm_area_struct *vma);
> > >
> > >    /* rxe_mr.c */
> > >    u8 rxe_get_next_key(u32 last_key);
> > > +void rxe_mr_init(int access, struct rxe_mr *mr);
> > >    void rxe_mr_init_dma(int access, struct rxe_mr *mr);
> > >    int rxe_mr_init_user(struct rxe_dev *rxe, u64 start, u64 length,
> > >    		     int access, struct rxe_mr *mr);
> > > @@ -69,6 +70,8 @@ int copy_data(struct rxe_pd *pd, int access, struct rxe_dma_info *dma,
> > >    	      void *addr, int length, enum rxe_mr_copy_dir dir);
> > >    int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg,
> > >    		  int sg_nents, unsigned int *sg_offset);
> > > +int rxe_mr_copy_xarray(struct rxe_mr *mr, u64 iova, void *addr,
> > > +		       unsigned int length, enum rxe_mr_copy_dir dir);
> > >    int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode,
> > >    			u64 compare, u64 swap_add, u64 *orig_val);
> > >    int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value);
> > > @@ -80,6 +83,11 @@ int rxe_invalidate_mr(struct rxe_qp *qp, u32 key);
> > >    int rxe_reg_fast_mr(struct rxe_qp *qp, struct rxe_send_wqe *wqe);
> > >    void rxe_mr_cleanup(struct rxe_pool_elem *elem);
> > >
> > > +static inline unsigned long rxe_mr_iova_to_index(struct rxe_mr *mr, u64 iova)
> > > +{
> > > +	return (iova >> mr->page_shift) - (mr->ibmr.iova >> mr->page_shift);
> > > +}
> >
> > The type of the function rxe_mr_iova_to_index is "unsigned long". In
> > some 32 architecture, unsigned long is 32 bit.
> >
> > The type of iova is u64. So it had better use u64 instead of "unsigned
> > long".
> >
> > Zhu Yanjun
>
> Hi,
> thanks for the comment.
>
> I think the current type declaration doesn't matter in 32-bit OS.
> The function returns an index of the page specified with 'iova'.
> Assuming the page size is typical 4KiB, u32 index can accommodate
> 16 TiB in total, which is larger than the theoretical limit imposed
> on 32-bit systems (i.e. 4GiB or 2^32 Bytes).

But in 32 bit OS, this will likely pop out "type does not match" warning 
because "unsigned long" is 32 bit in 32-bit OS while u64 is always 64 
bit. So it is better to use u64 type. This will not pop out any warnings 
whether in 32bit OS or in 64bit OS.

Zhu Yanjun
> Regards,
> Daisuke Matsuda
>
> > > +
> > >    /* rxe_mw.c */
> > >    int rxe_alloc_mw(struct ib_mw *ibmw, struct ib_udata *udata);
> > >    int rxe_dealloc_mw(struct ib_mw *ibmw);
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> > > index da3dee520876..1f7b8cf93adc 100644
> > > --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> > > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> > > @@ -45,7 +45,7 @@ int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length)
> > >    	}
> > >    }
> > >
> > > -static void rxe_mr_init(int access, struct rxe_mr *mr)
> > > +void rxe_mr_init(int access, struct rxe_mr *mr)
> > >    {
> > >    	u32 key = mr->elem.index << 8 | rxe_get_next_key(-1);
> > >
> > > @@ -72,11 +72,6 @@ void rxe_mr_init_dma(int access, struct rxe_mr *mr)
> > >    	mr->ibmr.type = IB_MR_TYPE_DMA;
> > >    }
> > >
> > > -static unsigned long rxe_mr_iova_to_index(struct rxe_mr *mr, u64 iova)
> > > -{
> > > -	return (iova >> mr->page_shift) - (mr->ibmr.iova >> mr->page_shift);
> > > -}
> > > -
> > >    static unsigned long rxe_mr_iova_to_page_offset(struct rxe_mr *mr, u64 iova)
> > >    {
> > >    	return iova & (mr_page_size(mr) - 1);
> > > @@ -242,8 +237,8 @@ int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sgl,
> > >    	return ib_sg_to_pages(ibmr, s"gl, sg_nents, sg_offset, rxe_set_page);
> > >    }
> > >
> > > -static int rxe_mr_copy_xarray(struct rxe_mr *mr, u64 iova, void *addr,
> > > -			      unsigned int length, enum rxe_mr_copy_dir dir)
> > > +int rxe_mr_copy_xarray(struct rxe_mr *mr, u64 iova, void *addr,
> > > +		       unsigned int length, enum rxe_mr_copy_dir dir)
> > >    {
> > >    	unsigned int page_offset = rxe_mr_iova_to_page_offset(mr, iova);
> > >    	unsigned long index = rxe_mr_iova_to_index(mr, iova);