| Message ID | 1623944783-9093-1-git-send-email-haakon.bugge@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-next] RDMA/cma: Remove unnecessary INIT->INIT transition | expand |
Hi "Håkon,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on rdma/for-next]
[also build test WARNING on v5.13-rc6 next-20210617]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/H-kon-Bugge/RDMA-cma-Remove-unnecessary-INIT-INIT-transition/20210617-234819
base: https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git for-next
config: powerpc64-randconfig-r011-20210617 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
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
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/c0ea00a31278d0ffe083f121ddb13e27f999c0a8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review H-kon-Bugge/RDMA-cma-Remove-unnecessary-INIT-INIT-transition/20210617-234819
git checkout c0ea00a31278d0ffe083f121ddb13e27f999c0a8
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:249:1: note: expanded from here
__do_insb
^
arch/powerpc/include/asm/io.h:556:56: note: expanded from macro '__do_insb'
#define __do_insb(p, b, n) readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/infiniband/core/cma.c:14:
In file included from include/linux/igmp.h:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:251:1: note: expanded from here
__do_insw
^
arch/powerpc/include/asm/io.h:557:56: note: expanded from macro '__do_insw'
#define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/infiniband/core/cma.c:14:
In file included from include/linux/igmp.h:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:253:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/infiniband/core/cma.c:14:
In file included from include/linux/igmp.h:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:255:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/infiniband/core/cma.c:14:
In file included from include/linux/igmp.h:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:257:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/infiniband/core/cma.c:14:
In file included from include/linux/igmp.h:13:
In file included from include/linux/skbuff.h:31:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:259:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
>> drivers/infiniband/core/cma.c:948:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (id->qp_type == IB_QPT_UD)
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/infiniband/core/cma.c:950:6: note: uninitialized use occurs here
if (ret)
^~~
drivers/infiniband/core/cma.c:948:2: note: remove the 'if' if its condition is always true
if (id->qp_type == IB_QPT_UD)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/infiniband/core/cma.c:933:9: note: initialize the variable 'ret' to silence this warning
int ret;
^
= 0
14 warnings generated.
vim +948 drivers/infiniband/core/cma.c
e51060f08a6196 Sean Hefty 2006-06-17 927
e51060f08a6196 Sean Hefty 2006-06-17 928 int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd,
e51060f08a6196 Sean Hefty 2006-06-17 929 struct ib_qp_init_attr *qp_init_attr)
e51060f08a6196 Sean Hefty 2006-06-17 930 {
e51060f08a6196 Sean Hefty 2006-06-17 931 struct rdma_id_private *id_priv;
e51060f08a6196 Sean Hefty 2006-06-17 932 struct ib_qp *qp;
e51060f08a6196 Sean Hefty 2006-06-17 933 int ret;
e51060f08a6196 Sean Hefty 2006-06-17 934
e51060f08a6196 Sean Hefty 2006-06-17 935 id_priv = container_of(id, struct rdma_id_private, id);
ed999f820a6c57 Chuck Lever 2019-12-18 936 if (id->device != pd->device) {
ed999f820a6c57 Chuck Lever 2019-12-18 937 ret = -EINVAL;
ed999f820a6c57 Chuck Lever 2019-12-18 938 goto out_err;
ed999f820a6c57 Chuck Lever 2019-12-18 939 }
e51060f08a6196 Sean Hefty 2006-06-17 940
0691a286d59183 Christoph Hellwig 2016-05-03 941 qp_init_attr->port_num = id->port_num;
e51060f08a6196 Sean Hefty 2006-06-17 942 qp = ib_create_qp(pd, qp_init_attr);
ed999f820a6c57 Chuck Lever 2019-12-18 943 if (IS_ERR(qp)) {
ed999f820a6c57 Chuck Lever 2019-12-18 944 ret = PTR_ERR(qp);
ed999f820a6c57 Chuck Lever 2019-12-18 945 goto out_err;
ed999f820a6c57 Chuck Lever 2019-12-18 946 }
e51060f08a6196 Sean Hefty 2006-06-17 947
b26f9b9949013f Sean Hefty 2010-04-01 @948 if (id->qp_type == IB_QPT_UD)
c8f6a362bf3eb2 Sean Hefty 2007-02-15 949 ret = cma_init_ud_qp(id_priv, qp);
e51060f08a6196 Sean Hefty 2006-06-17 950 if (ret)
ed999f820a6c57 Chuck Lever 2019-12-18 951 goto out_destroy;
e51060f08a6196 Sean Hefty 2006-06-17 952
e51060f08a6196 Sean Hefty 2006-06-17 953 id->qp = qp;
e51060f08a6196 Sean Hefty 2006-06-17 954 id_priv->qp_num = qp->qp_num;
e51060f08a6196 Sean Hefty 2006-06-17 955 id_priv->srq = (qp->srq != NULL);
ed999f820a6c57 Chuck Lever 2019-12-18 956 trace_cm_qp_create(id_priv, pd, qp_init_attr, 0);
e51060f08a6196 Sean Hefty 2006-06-17 957 return 0;
ed999f820a6c57 Chuck Lever 2019-12-18 958 out_destroy:
e51060f08a6196 Sean Hefty 2006-06-17 959 ib_destroy_qp(qp);
ed999f820a6c57 Chuck Lever 2019-12-18 960 out_err:
ed999f820a6c57 Chuck Lever 2019-12-18 961 trace_cm_qp_create(id_priv, pd, qp_init_attr, ret);
e51060f08a6196 Sean Hefty 2006-06-17 962 return ret;
e51060f08a6196 Sean Hefty 2006-06-17 963 }
e51060f08a6196 Sean Hefty 2006-06-17 964 EXPORT_SYMBOL(rdma_create_qp);
e51060f08a6196 Sean Hefty 2006-06-17 965
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 6/17/2021 11:46 PM, Håkon Bugge wrote: > External email: Use caution opening links or attachments > > > In rdma_create_qp(), a connected QP will be transitioned to the INIT > state. > > Afterwards, the QP will be transitioned to the RTR state by the > cma_modify_qp_rtr() function. But this function starts by performing > an ib_modify_qp() to the INIT state again, before another > ib_modify_qp() is performed to transition the QP to the RTR state. > > Hence, there is no need to transition the QP to the INIT state in > rdma_create_qp(). The comment in cma_modify_qp_rtr() says: /* Need to update QP attributes from default values. */ So maybe both are needed? E.g., qp_attr->qp_access_flags maybe updated in cm_init_qp_init_attr()? > Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> > --- > drivers/infiniband/core/cma.c | 15 --------------- > 1 file changed, 15 deletions(-) > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index 2b9ffc2..937e44e 100644 > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -925,19 +925,6 @@ static int cma_init_ud_qp(struct rdma_id_private *id_priv, struct ib_qp *qp) > return ret; > } > > -static int cma_init_conn_qp(struct rdma_id_private *id_priv, struct ib_qp *qp) > -{ > - struct ib_qp_attr qp_attr; > - int qp_attr_mask, ret; > - > - qp_attr.qp_state = IB_QPS_INIT; > - ret = rdma_init_qp_attr(&id_priv->id, &qp_attr, &qp_attr_mask); > - if (ret) > - return ret; > - > - return ib_modify_qp(qp, &qp_attr, qp_attr_mask); > -} > - > int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, > struct ib_qp_init_attr *qp_init_attr) > { > @@ -960,8 +947,6 @@ int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, > > if (id->qp_type == IB_QPT_UD) > ret = cma_init_ud_qp(id_priv, qp); > - else > - ret = cma_init_conn_qp(id_priv, qp); > if (ret) > goto out_destroy; > > -- > 1.8.3.1 >
> On 20 Jun 2021, at 14:19, Mark Zhang <markzhang@nvidia.com> wrote: > > On 6/17/2021 11:46 PM, Håkon Bugge wrote: >> External email: Use caution opening links or attachments >> In rdma_create_qp(), a connected QP will be transitioned to the INIT >> state. >> Afterwards, the QP will be transitioned to the RTR state by the >> cma_modify_qp_rtr() function. But this function starts by performing >> an ib_modify_qp() to the INIT state again, before another >> ib_modify_qp() is performed to transition the QP to the RTR state. >> Hence, there is no need to transition the QP to the INIT state in >> rdma_create_qp(). > > The comment in cma_modify_qp_rtr() says: > /* Need to update QP attributes from default values. */ > > So maybe both are needed? E.g., qp_attr->qp_access_flags maybe updated in cm_init_qp_init_attr()? I'll give you two reasons why that is not the case :-) 1. In cm_init_qp_init_attr(), which sets the mask both places, the mask is set hard to IB_QP_STATE | IB_QP_ACCESS_FLAGS | IB_QP_PKEY_INDEX | IB_QP_PORT If we do the old RESET -> INIT -> INIT, it is the last modify_qp to INIT which will persist. And therefore, the values will be the same if we skip RESET -> INIT. 2. I think the rationale behind modifying the QP to INIT in rdma_create_qp() was to enable ULPs to tweak some of the values. But no ULP calls modify_qp on a QP created by rdma_create_qp(). And if one did, the values would be overwritten when the state transitions to RTR. Thxs, Håkon > >> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> >> --- >> drivers/infiniband/core/cma.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c >> index 2b9ffc2..937e44e 100644 >> --- a/drivers/infiniband/core/cma.c >> +++ b/drivers/infiniband/core/cma.c >> @@ -925,19 +925,6 @@ static int cma_init_ud_qp(struct rdma_id_private *id_priv, struct ib_qp *qp) >> return ret; >> } >> -static int cma_init_conn_qp(struct rdma_id_private *id_priv, struct ib_qp *qp) >> -{ >> - struct ib_qp_attr qp_attr; >> - int qp_attr_mask, ret; >> - >> - qp_attr.qp_state = IB_QPS_INIT; >> - ret = rdma_init_qp_attr(&id_priv->id, &qp_attr, &qp_attr_mask); >> - if (ret) >> - return ret; >> - >> - return ib_modify_qp(qp, &qp_attr, qp_attr_mask); >> -} >> - >> int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, >> struct ib_qp_init_attr *qp_init_attr) >> { >> @@ -960,8 +947,6 @@ int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, >> if (id->qp_type == IB_QPT_UD) >> ret = cma_init_ud_qp(id_priv, qp); >> - else >> - ret = cma_init_conn_qp(id_priv, qp); >> if (ret) >> goto out_destroy; >> -- >> 1.8.3.1 >
On Thu, Jun 17, 2021 at 05:46:23PM +0200, Håkon Bugge wrote: > In rdma_create_qp(), a connected QP will be transitioned to the INIT > state. > > Afterwards, the QP will be transitioned to the RTR state by the > cma_modify_qp_rtr() function. But this function starts by performing > an ib_modify_qp() to the INIT state again, before another > ib_modify_qp() is performed to transition the QP to the RTR state. > > Hence, there is no need to transition the QP to the INIT state in > rdma_create_qp(). > > Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> > --- > drivers/infiniband/core/cma.c | 15 --------------- > 1 file changed, 15 deletions(-) > Thanks, Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
On 6/21/2021 4:44 PM, Haakon Bugge wrote: > External email: Use caution opening links or attachments > > >> On 20 Jun 2021, at 14:19, Mark Zhang <markzhang@nvidia.com> wrote: >> >> On 6/17/2021 11:46 PM, Håkon Bugge wrote: >>> External email: Use caution opening links or attachments >>> In rdma_create_qp(), a connected QP will be transitioned to the INIT >>> state. >>> Afterwards, the QP will be transitioned to the RTR state by the >>> cma_modify_qp_rtr() function. But this function starts by performing >>> an ib_modify_qp() to the INIT state again, before another >>> ib_modify_qp() is performed to transition the QP to the RTR state. >>> Hence, there is no need to transition the QP to the INIT state in >>> rdma_create_qp(). >> >> The comment in cma_modify_qp_rtr() says: >> /* Need to update QP attributes from default values. */ >> >> So maybe both are needed? E.g., qp_attr->qp_access_flags maybe updated in cm_init_qp_init_attr()? > > I'll give you two reasons why that is not the case :-) > > 1. In cm_init_qp_init_attr(), which sets the mask both places, the mask is set hard to > > IB_QP_STATE | IB_QP_ACCESS_FLAGS | IB_QP_PKEY_INDEX | IB_QP_PORT > > If we do the old RESET -> INIT -> INIT, it is the last modify_qp to INIT which will persist. And therefore, the values will be the same if we skip RESET -> INIT. > > 2. I think the rationale behind modifying the QP to INIT in rdma_create_qp() was to enable ULPs to tweak some of the values. But no ULP calls modify_qp on a QP created by rdma_create_qp(). And if one did, the values would be overwritten when the state transitions to RTR. > Got it, thanks Haakon:)
> On 21 Jun 2021, at 12:07, Leon Romanovsky <leon@kernel.org> wrote: > > On Thu, Jun 17, 2021 at 05:46:23PM +0200, Håkon Bugge wrote: >> In rdma_create_qp(), a connected QP will be transitioned to the INIT >> state. >> >> Afterwards, the QP will be transitioned to the RTR state by the >> cma_modify_qp_rtr() function. But this function starts by performing >> an ib_modify_qp() to the INIT state again, before another >> ib_modify_qp() is performed to transition the QP to the RTR state. >> >> Hence, there is no need to transition the QP to the INIT state in >> rdma_create_qp(). >> >> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> >> --- >> drivers/infiniband/core/cma.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> > > Thanks, > Reviewed-by: Leon Romanovsky <leonro@nvidia.com> Thank you Leon! Håkon
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index 2b9ffc2..937e44e 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -925,19 +925,6 @@ static int cma_init_ud_qp(struct rdma_id_private *id_priv, struct ib_qp *qp) return ret; } -static int cma_init_conn_qp(struct rdma_id_private *id_priv, struct ib_qp *qp) -{ - struct ib_qp_attr qp_attr; - int qp_attr_mask, ret; - - qp_attr.qp_state = IB_QPS_INIT; - ret = rdma_init_qp_attr(&id_priv->id, &qp_attr, &qp_attr_mask); - if (ret) - return ret; - - return ib_modify_qp(qp, &qp_attr, qp_attr_mask); -} - int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, struct ib_qp_init_attr *qp_init_attr) { @@ -960,8 +947,6 @@ int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, if (id->qp_type == IB_QPT_UD) ret = cma_init_ud_qp(id_priv, qp); - else - ret = cma_init_conn_qp(id_priv, qp); if (ret) goto out_destroy;
In rdma_create_qp(), a connected QP will be transitioned to the INIT state. Afterwards, the QP will be transitioned to the RTR state by the cma_modify_qp_rtr() function. But this function starts by performing an ib_modify_qp() to the INIT state again, before another ib_modify_qp() is performed to transition the QP to the RTR state. Hence, there is no need to transition the QP to the INIT state in rdma_create_qp(). Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> --- drivers/infiniband/core/cma.c | 15 --------------- 1 file changed, 15 deletions(-)