| Message ID | 20211228080717.10666-6-lizhijian@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/rxe: Add RDMA FLUSH operation | expand |
On 12/28/2021 3:07 AM, Li Zhijian wrote: > Memory region should support 2 placement types: IB_ACCESS_FLUSH_PERSISTENT > and IB_ACCESS_FLUSH_GLOBAL_VISIBILITY, and only pmem/nvdimm has ability to > persist data(IB_ACCESS_FLUSH_PERSISTENT). > > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> > --- > drivers/infiniband/sw/rxe/rxe_mr.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c > index bcd5e7afa475..21616d058f29 100644 > --- a/drivers/infiniband/sw/rxe/rxe_mr.c > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c > @@ -206,6 +206,11 @@ static bool iova_in_pmem(struct rxe_mr *mr, u64 iova, int length) > return page_in_dev_pagemap(page); > } > > +static bool ib_check_flush_access_flags(struct ib_mr *mr, u32 flags) > +{ > + return mr->is_pmem || !(flags & IB_ACCESS_FLUSH_PERSISTENT); > +} It is perfectly allowed to flush ordinary memory, persistence is another matter entirely. Is this subroutine checking for flush, or persistence? Its name is confusing and needs to be clarified. > + > int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > int access, struct rxe_mr *mr) > { > @@ -282,6 +287,13 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, > > // iova_in_pmem must be called after set is updated > mr->ibmr.is_pmem = iova_in_pmem(mr, iova, length); > + if (!ib_check_flush_access_flags(&mr->ibmr, access)) { > + pr_err("Cannot set IB_ACCESS_FLUSH_PERSISTENT for non-pmem memory\n"); > + mr->state = RXE_MR_STATE_INVALID; > + mr->umem = NULL; > + err = -EINVAL; > + goto err_release_umem; > + } Setting is_pmem is reasonable, but again, this is confusing with respect to the region being flushable. In general, all memory is flushable, provided the platform supports any kind of cache flush (i.e. all of them). Tom.
delete unreachable liweihang@huawei.com from recipients On 31/12/2021 06:25, Tom Talpey wrote: > On 12/28/2021 3:07 AM, Li Zhijian wrote: >> Memory region should support 2 placement types: IB_ACCESS_FLUSH_PERSISTENT >> and IB_ACCESS_FLUSH_GLOBAL_VISIBILITY, and only pmem/nvdimm has ability to >> persist data(IB_ACCESS_FLUSH_PERSISTENT). >> >> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> >> --- >> drivers/infiniband/sw/rxe/rxe_mr.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c >> index bcd5e7afa475..21616d058f29 100644 >> --- a/drivers/infiniband/sw/rxe/rxe_mr.c >> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c >> @@ -206,6 +206,11 @@ static bool iova_in_pmem(struct rxe_mr *mr, u64 iova, int length) >> return page_in_dev_pagemap(page); >> } >> +static bool ib_check_flush_access_flags(struct ib_mr *mr, u32 flags) >> +{ >> + return mr->is_pmem || !(flags & IB_ACCESS_FLUSH_PERSISTENT); >> +} > > It is perfectly allowed to flush ordinary memory, persistence is > another matter entirely. It did, but only allows for the MRs that registered FLUSH access flags. > Is this subroutine checking for flush, > or persistence? both, but it should be called in registering MR stage. we have to 2 checking points, here is the 1st gate, where it prevent local user from registering a persistent access flag to an ordinary memory. 2nd is in [RFC PATCH rdma-next 08/10] RDMA/rxe: Implement flush execution in responder side where it prevent remote user to requesting persist data into an ordinary memory. > Its name is confusing and needs to be clarified. Err, let me see.... a more suitable name is very welcome. Thanks > >> + >> int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, >> int access, struct rxe_mr *mr) >> { >> @@ -282,6 +287,13 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, >> // iova_in_pmem must be called after set is updated >> mr->ibmr.is_pmem = iova_in_pmem(mr, iova, length); >> + if (!ib_check_flush_access_flags(&mr->ibmr, access)) { >> + pr_err("Cannot set IB_ACCESS_FLUSH_PERSISTENT for non-pmem memory\n"); >> + mr->state = RXE_MR_STATE_INVALID; >> + mr->umem = NULL; >> + err = -EINVAL; >> + goto err_release_umem; >> + } > > Setting is_pmem is reasonable, but again, this is confusing with respect > to the region being flushable. In general, all memory is flushable, > provided the platform supports any kind of cache flush (i.e. all of them). > > Tom.
On 12/30/2021 10:34 PM, lizhijian@fujitsu.com wrote: >>> +static bool ib_check_flush_access_flags(struct ib_mr *mr, u32 flags) >>> +{ >>> + return mr->is_pmem || !(flags & IB_ACCESS_FLUSH_PERSISTENT); >>> +} >> Its name is confusing and needs to be clarified. > > Err, let me see.... a more suitable name is very welcome. Since the subroutine is rather simple, and with only a single reference in a single file, it would be best to just pull the test inline and delete it. This would also remove some inefficient code. if (flags & IB_ACCESS_FLUSH_PERSISTENT) { if (!iova_in_pmem(mr, iova, length) { pr_err("Cannot set IB_ACCESS_FLUSH_PERSISTENT for non-pmem memory\n"); mr->state = RXE_MR_STATE_INVALID; mr->umem = NULL; err = -EINVAL; goto err_release_umem; } mr-> ibmr.is_pmem = true; } ...
On 31/12/2021 22:40, Tom Talpey wrote: > On 12/30/2021 10:34 PM, lizhijian@fujitsu.com wrote: > >>>> +static bool ib_check_flush_access_flags(struct ib_mr *mr, u32 flags) >>>> +{ >>>> + return mr->is_pmem || !(flags & IB_ACCESS_FLUSH_PERSISTENT); >>>> +} > >>> Its name is confusing and needs to be clarified. >> >> Err, let me see.... a more suitable name is very welcome. > > Since the subroutine is rather simple, and with only a single > reference in a single file, it would be best to just pull > the test inline and delete it. This would also remove some > inefficient code. > > if (flags & IB_ACCESS_FLUSH_PERSISTENT) { > if (!iova_in_pmem(mr, iova, length) { > pr_err("Cannot set IB_ACCESS_FLUSH_PERSISTENT for non-pmem memory\n"); > mr->state = RXE_MR_STATE_INVALID; > mr->umem = NULL; > err = -EINVAL; > goto err_release_umem; > } > mr-> ibmr.is_pmem = true; > } > ... Make sense. Thanks Zhijian
diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c index bcd5e7afa475..21616d058f29 100644 --- a/drivers/infiniband/sw/rxe/rxe_mr.c +++ b/drivers/infiniband/sw/rxe/rxe_mr.c @@ -206,6 +206,11 @@ static bool iova_in_pmem(struct rxe_mr *mr, u64 iova, int length) return page_in_dev_pagemap(page); } +static bool ib_check_flush_access_flags(struct ib_mr *mr, u32 flags) +{ + return mr->is_pmem || !(flags & IB_ACCESS_FLUSH_PERSISTENT); +} + int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, int access, struct rxe_mr *mr) { @@ -282,6 +287,13 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova, // iova_in_pmem must be called after set is updated mr->ibmr.is_pmem = iova_in_pmem(mr, iova, length); + if (!ib_check_flush_access_flags(&mr->ibmr, access)) { + pr_err("Cannot set IB_ACCESS_FLUSH_PERSISTENT for non-pmem memory\n"); + mr->state = RXE_MR_STATE_INVALID; + mr->umem = NULL; + err = -EINVAL; + goto err_release_umem; + } return 0;
Memory region should support 2 placement types: IB_ACCESS_FLUSH_PERSISTENT and IB_ACCESS_FLUSH_GLOBAL_VISIBILITY, and only pmem/nvdimm has ability to persist data(IB_ACCESS_FLUSH_PERSISTENT). Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> --- drivers/infiniband/sw/rxe/rxe_mr.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)