| Message ID | 20230113002116.457324-5-rpearsonhpe@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/rxe: Replace mr page map with an xarray | expand |
Hi Bob,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bd99ede8ef2dc03e29a181b755ba4f78da2644e6]
url: https://github.com/intel-lab-lkp/linux/commits/Bob-Pearson/RDMA-rxe-Cleanup-mr_check_range/20230113-082406
base: bd99ede8ef2dc03e29a181b755ba4f78da2644e6
patch link: https://lore.kernel.org/r/20230113002116.457324-5-rpearsonhpe%40gmail.com
patch subject: [PATCH for-next v2 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()
config: powerpc-allmodconfig
compiler: powerpc-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/c7e5c2723da0d6b13e37cbff63dad653e5c8801c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Bob-Pearson/RDMA-rxe-Cleanup-mr_check_range/20230113-082406
git checkout c7e5c2723da0d6b13e37cbff63dad653e5c8801c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/infiniband/sw/rxe/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from <command-line>:
drivers/infiniband/sw/rxe/rxe_mr.c: In function 'rxe_mr_do_atomic_write':
>> include/linux/compiler_types.h:358:45: error: call to '__compiletime_assert_870' declared with attribute error: Need native word sized stores/loads for atomicity.
358 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
include/linux/compiler_types.h:339:25: note: in definition of macro '__compiletime_assert'
339 | prefix ## suffix(); \
| ^~~~~~
include/linux/compiler_types.h:358:9: note: in expansion of macro '_compiletime_assert'
358 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:361:9: note: in expansion of macro 'compiletime_assert'
361 | compiletime_assert(__native_word(t), \
| ^~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/barrier.h:74:9: note: in expansion of macro 'compiletime_assert_atomic_type'
74 | compiletime_assert_atomic_type(*p); \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/barrier.h:172:55: note: in expansion of macro '__smp_store_release'
172 | #define smp_store_release(p, v) do { kcsan_release(); __smp_store_release(p, v); } while (0)
| ^~~~~~~~~~~~~~~~~~~
drivers/infiniband/sw/rxe/rxe_mr.c:605:9: note: in expansion of macro 'smp_store_release'
605 | smp_store_release(vaddr, value);
| ^~~~~~~~~~~~~~~~~
vim +/__compiletime_assert_870 +358 include/linux/compiler_types.h
eb5c2d4b45e3d2 Will Deacon 2020-07-21 344
eb5c2d4b45e3d2 Will Deacon 2020-07-21 345 #define _compiletime_assert(condition, msg, prefix, suffix) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 346 __compiletime_assert(condition, msg, prefix, suffix)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 347
eb5c2d4b45e3d2 Will Deacon 2020-07-21 348 /**
eb5c2d4b45e3d2 Will Deacon 2020-07-21 349 * compiletime_assert - break build and emit msg if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 350 * @condition: a compile-time constant condition to check
eb5c2d4b45e3d2 Will Deacon 2020-07-21 351 * @msg: a message to emit if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 352 *
eb5c2d4b45e3d2 Will Deacon 2020-07-21 353 * In tradition of POSIX assert, this macro will break the build if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 354 * supplied condition is *false*, emitting the supplied error message if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 355 * compiler has support to do so.
eb5c2d4b45e3d2 Will Deacon 2020-07-21 356 */
eb5c2d4b45e3d2 Will Deacon 2020-07-21 357 #define compiletime_assert(condition, msg) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 @358 _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 359
On 1/12/23 21:43, kernel test robot wrote: > Hi Bob, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on bd99ede8ef2dc03e29a181b755ba4f78da2644e6] > > url: https://github.com/intel-lab-lkp/linux/commits/Bob-Pearson/RDMA-rxe-Cleanup-mr_check_range/20230113-082406 > base: bd99ede8ef2dc03e29a181b755ba4f78da2644e6 > patch link: https://lore.kernel.org/r/20230113002116.457324-5-rpearsonhpe%40gmail.com > patch subject: [PATCH for-next v2 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply() > config: powerpc-allmodconfig > compiler: powerpc-linux-gcc (GCC) 12.1.0 > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/intel-lab-lkp/linux/commit/c7e5c2723da0d6b13e37cbff63dad653e5c8801c > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Bob-Pearson/RDMA-rxe-Cleanup-mr_check_range/20230113-082406 > git checkout c7e5c2723da0d6b13e37cbff63dad653e5c8801c > # save the config file > mkdir build_dir && cp config build_dir/.config > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc olddefconfig > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/infiniband/sw/rxe/ > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > In file included from <command-line>: > drivers/infiniband/sw/rxe/rxe_mr.c: In function 'rxe_mr_do_atomic_write': >>> include/linux/compiler_types.h:358:45: error: call to '__compiletime_assert_870' declared with attribute error: Need native word sized stores/loads for atomicity. Jason, Does this mean that powerpc doesn't support u64 as a native type? Perhaps this is why the Fujitsu folks used CONFIG_64BIT in the atomic write patch which seemed a little strange. Bob > 358 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > | ^ > include/linux/compiler_types.h:339:25: note: in definition of macro '__compiletime_assert' > 339 | prefix ## suffix(); \ > | ^~~~~~ > include/linux/compiler_types.h:358:9: note: in expansion of macro '_compiletime_assert' > 358 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > | ^~~~~~~~~~~~~~~~~~~ > include/linux/compiler_types.h:361:9: note: in expansion of macro 'compiletime_assert' > 361 | compiletime_assert(__native_word(t), \ > | ^~~~~~~~~~~~~~~~~~ > arch/powerpc/include/asm/barrier.h:74:9: note: in expansion of macro 'compiletime_assert_atomic_type' > 74 | compiletime_assert_atomic_type(*p); \ > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/asm-generic/barrier.h:172:55: note: in expansion of macro '__smp_store_release' > 172 | #define smp_store_release(p, v) do { kcsan_release(); __smp_store_release(p, v); } while (0) > | ^~~~~~~~~~~~~~~~~~~ > drivers/infiniband/sw/rxe/rxe_mr.c:605:9: note: in expansion of macro 'smp_store_release' > 605 | smp_store_release(vaddr, value); > | ^~~~~~~~~~~~~~~~~ > > > vim +/__compiletime_assert_870 +358 include/linux/compiler_types.h > > eb5c2d4b45e3d2 Will Deacon 2020-07-21 344 > eb5c2d4b45e3d2 Will Deacon 2020-07-21 345 #define _compiletime_assert(condition, msg, prefix, suffix) \ > eb5c2d4b45e3d2 Will Deacon 2020-07-21 346 __compiletime_assert(condition, msg, prefix, suffix) > eb5c2d4b45e3d2 Will Deacon 2020-07-21 347 > eb5c2d4b45e3d2 Will Deacon 2020-07-21 348 /** > eb5c2d4b45e3d2 Will Deacon 2020-07-21 349 * compiletime_assert - break build and emit msg if condition is false > eb5c2d4b45e3d2 Will Deacon 2020-07-21 350 * @condition: a compile-time constant condition to check > eb5c2d4b45e3d2 Will Deacon 2020-07-21 351 * @msg: a message to emit if condition is false > eb5c2d4b45e3d2 Will Deacon 2020-07-21 352 * > eb5c2d4b45e3d2 Will Deacon 2020-07-21 353 * In tradition of POSIX assert, this macro will break the build if the > eb5c2d4b45e3d2 Will Deacon 2020-07-21 354 * supplied condition is *false*, emitting the supplied error message if the > eb5c2d4b45e3d2 Will Deacon 2020-07-21 355 * compiler has support to do so. > eb5c2d4b45e3d2 Will Deacon 2020-07-21 356 */ > eb5c2d4b45e3d2 Will Deacon 2020-07-21 357 #define compiletime_assert(condition, msg) \ > eb5c2d4b45e3d2 Will Deacon 2020-07-21 @358 _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > eb5c2d4b45e3d2 Will Deacon 2020-07-21 359 >
On Fri, Jan 13, 2023 at 03:26:25PM -0600, Bob Pearson wrote: > On 1/12/23 21:43, kernel test robot wrote: > > Hi Bob, > > > > Thank you for the patch! Yet something to improve: > > > > [auto build test ERROR on bd99ede8ef2dc03e29a181b755ba4f78da2644e6] > > > > url: https://github.com/intel-lab-lkp/linux/commits/Bob-Pearson/RDMA-rxe-Cleanup-mr_check_range/20230113-082406 > > base: bd99ede8ef2dc03e29a181b755ba4f78da2644e6 > > patch link: https://lore.kernel.org/r/20230113002116.457324-5-rpearsonhpe%40gmail.com > > patch subject: [PATCH for-next v2 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply() > > config: powerpc-allmodconfig > > compiler: powerpc-linux-gcc (GCC) 12.1.0 > > reproduce (this is a W=1 build): > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # https://github.com/intel-lab-lkp/linux/commit/c7e5c2723da0d6b13e37cbff63dad653e5c8801c > > git remote add linux-review https://github.com/intel-lab-lkp/linux > > git fetch --no-tags linux-review Bob-Pearson/RDMA-rxe-Cleanup-mr_check_range/20230113-082406 > > git checkout c7e5c2723da0d6b13e37cbff63dad653e5c8801c > > # save the config file > > mkdir build_dir && cp config build_dir/.config > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc olddefconfig > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/infiniband/sw/rxe/ > > > > If you fix the issue, kindly add following tag where applicable > > | Reported-by: kernel test robot <lkp@intel.com> > > > > All errors (new ones prefixed by >>): > > > > In file included from <command-line>: > > drivers/infiniband/sw/rxe/rxe_mr.c: In function 'rxe_mr_do_atomic_write': > >>> include/linux/compiler_types.h:358:45: error: call to '__compiletime_assert_870' declared with attribute error: Need native word sized stores/loads for atomicity. > > Jason, > Does this mean that powerpc doesn't support u64 as a native type? Perhaps this is > why the Fujitsu folks used CONFIG_64BIT in the atomic write patch which seemed a > little strange. Yes and yes Jason
diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h index bcb1bbcf50df..fd70c71a9e4e 100644 --- a/drivers/infiniband/sw/rxe/rxe_loc.h +++ b/drivers/infiniband/sw/rxe/rxe_loc.h @@ -74,6 +74,7 @@ int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, void *iova_to_vaddr(struct rxe_mr *mr, u64 iova, int length); 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, void *addr); struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key, enum rxe_mr_lookup_type type); int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length); diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c index 791731be6067..d11b38117e58 100644 --- a/drivers/infiniband/sw/rxe/rxe_mr.c +++ b/drivers/infiniband/sw/rxe/rxe_mr.c @@ -568,6 +568,45 @@ int rxe_mr_do_atomic_op(struct rxe_mr *mr, u64 iova, int opcode, return 0; } +/* + * Returns: + * 0 on success + * -1 for misaligned address + * -2 for access errors + */ +int rxe_mr_do_atomic_write(struct rxe_mr *mr, u64 iova, void *addr) +{ + u64 *vaddr; + u64 value; + unsigned int length = 8; + + /* See IBA oA19-28 */ + if (unlikely(mr->state != RXE_MR_STATE_VALID)) { + rxe_dbg_mr(mr, "mr not valid"); + return -2; + } + + /* See IBA A19.4.2 */ + if (unlikely((uintptr_t)vaddr & 0x7 || iova & 0x7)) { + rxe_dbg_mr(mr, "misaligned address"); + return -1; + } + + vaddr = iova_to_vaddr(mr, iova, length); + if (unlikely(!vaddr)) { + rxe_dbg_mr(mr, "iova out of range"); + return -2; + } + + /* this makes no sense. What of payload is not 8? */ + memcpy(&value, addr, length); + + /* Do atomic write after all prior operations have completed */ + smp_store_release(vaddr, value); + + return 0; +} + 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 02301e3f343c..b60404fd33b0 100644 --- a/drivers/infiniband/sw/rxe/rxe_resp.c +++ b/drivers/infiniband/sw/rxe/rxe_resp.c @@ -762,26 +762,33 @@ 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) { + u64 iova = qp->resp.va + qp->resp.offset; + struct resp_res *res = qp->resp.res; struct rxe_mr *mr = qp->resp.mr; + void *addr = payload_addr(pkt); int payload = payload_size(pkt); - u64 src, *dst; + int err; - if (mr->state != RXE_MR_STATE_VALID) - return RESPST_ERR_RKEY_VIOLATION; + if (!res) { + res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_WRITE_MASK); + qp->resp.res = res; + } - memcpy(&src, payload_addr(pkt), payload); + if (res->replay) + return RESPST_ACKNOWLEDGE; - 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 !defined(CONFIG_64BIT) + return RESPST_ERR_UNSUPPORTED_OPCODE; +#endif - /* Do atomic write after all prior operations have completed */ - smp_store_release(dst, src); + err = rxe_mr_do_atomic_write(mr, iova, addr); + if (err == -1) + return RESPST_ERR_MISALIGNED_ATOMIC; + else if (err) + return RESPST_ERR_RKEY_VIOLATION; /* decrease resp.resid to zero */ qp->resp.resid -= sizeof(payload); @@ -794,29 +801,8 @@ static enum resp_states do_atomic_write(struct rxe_qp *qp, qp->resp.opcode = pkt->opcode; qp->resp.status = IB_WC_SUCCESS; - return RESPST_ACKNOWLEDGE; -} -#else -static enum resp_states do_atomic_write(struct rxe_qp *qp, - struct rxe_pkt_info *pkt) -{ - return RESPST_ERR_UNSUPPORTED_OPCODE; -} -#endif /* CONFIG_64BIT */ - -static enum resp_states atomic_write_reply(struct rxe_qp *qp, - struct rxe_pkt_info *pkt) -{ - struct resp_res *res = qp->resp.res; - if (!res) { - res = rxe_prepare_res(qp, pkt, RXE_ATOMIC_WRITE_MASK); - qp->resp.res = res; - } - - if (res->replay) - return RESPST_ACKNOWLEDGE; - return do_atomic_write(qp, pkt); + return RESPST_ACKNOWLEDGE; } static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
Isolate mr specific code from atomic_write_reply() in rxe_resp.c into a subroutine rxe_mr_do_atomic_write() in rxe_mr.c. Check length for atomic write operation. Make iova_to_vaddr() static. Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> --- v2: Fixed missing if() after else in atomic_write_reply(). drivers/infiniband/sw/rxe/rxe_loc.h | 1 + drivers/infiniband/sw/rxe/rxe_mr.c | 39 +++++++++++++++++++ drivers/infiniband/sw/rxe/rxe_resp.c | 56 +++++++++++----------------- 3 files changed, 61 insertions(+), 35 deletions(-)