Re: Re: Re: [PATCH] RDMA/siw: Trim size of page array to max size needed

[Leon Romanovsky <leon@kernel.org>]

On Thu, Feb 08, 2024 at 11:32:37AM +0000, Bernard Metzler wrote:
> 
> 
> > -----Original Message-----
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Friday, January 26, 2024 12:06 PM
> > To: Bernard Metzler <BMT@zurich.ibm.com>
> > Cc: Guoqing Jiang <guoqing.jiang@linux.dev>; linux-rdma@vger.kernel.org;
> > jgg@ziepe.ca; ionut_n2001@yahoo.com
> > Subject: [EXTERNAL] Re: Re: [PATCH] RDMA/siw: Trim size of page array to
> > max size needed
> > 
> > On Thu, Jan 25, 2024 at 05:27:52PM +0000, Bernard Metzler wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Guoqing Jiang <guoqing.jiang@linux.dev>
> > > > Sent: Thursday, January 25, 2024 1:15 AM
> > > > To: Bernard Metzler <BMT@zurich.ibm.com>; linux-rdma@vger.kernel.org
> > > > Cc: jgg@ziepe.ca; leon@kernel.org; ionut_n2001@yahoo.com
> > > > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Trim size of page array to
> > max
> > > > size needed
> > > >
> > > > Hi Bernard,
> > > >
> > > > On 1/25/24 03:59, Bernard Metzler wrote:
> > > > >> -----Original Message-----
> > > > >> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> > > > >> Sent: Tuesday, January 23, 2024 3:43 AM
> > > > >> To: Bernard Metzler <BMT@zurich.ibm.com>; linux-rdma@vger.kernel.org
> > > > >> Cc: jgg@ziepe.ca; leon@kernel.org; ionut_n2001@yahoo.com
> > > > >> Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Trim size of page array to
> > > > max
> > > > >> size needed
> > > > >>
> > > > >> Hi Bernard,
> > > > >>
> > > > >> On 1/19/24 21:05, Bernard Metzler wrote:
> > > > >>> siw tries sending all parts of an iWarp wire frame in one socket
> > > > >>> send operation. If user data can be send without copy, user data
> > > > >>> pages for one wire frame are referenced in an fixed size page
> > array.
> > > > >>> The size of this array can be made 2 elements smaller, since it
> > > > >>> does not reference iWarp header and trailer crc. Trimming
> > > > >>> the page array reduces the affected siw_tx_hdt() functions frame
> > > > >>> size, staying below 1024 bytes. This avoids the following
> > > > >>> compile-time warning:
> > > > >>>
> > > > >>>    drivers/infiniband/sw/siw/siw_qp_tx.c: In function 'siw_tx_hdt':
> > > > >>>    drivers/infiniband/sw/siw/siw_qp_tx.c:677:1: warning: the frame
> > > > >>>    size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-
> > > > than=]
> > > > >> I saw similar warning in my ubuntu 22.04 VM which has below gcc.
> > > > >>
> > > > >> root@buk:/home/gjiang/linux-mirror# make
> > M=drivers/infiniband/sw/siw/
> > > > >> -j16 W=1
> > > > >>     CC [M]  drivers/infiniband/sw/siw/siw_qp_tx.o
> > > > >> drivers/infiniband/sw/siw/siw_qp_tx.c: In function ‘siw_tx_hdt’:
> > > > >> drivers/infiniband/sw/siw/siw_qp_tx.c:665:1: warning: the frame size
> > > > of
> > > > >> 1440 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > > > >>     665 | }
> > > > >>         | ^
> > > > >>
> > > > > Whew.. that is quite substantially off the target!
> > > > > How come different compilers making so much of a difference.
> > > > > Guoqing, can you check if the macro computing the maximum number
> > > > > of fragments is broken, i.e., computes different values in
> > > > > the cases you refer?
> > > >
> > > > Sorry, I was wrong 😅.
> > > >
> > > > The warning is not relevant with compiler, and it also appears with
> > gcc-
> > > > 13.1
> > > > after enable KASAN which is used to find out-of-bounds bugs. Also,
> > there
> > > > are lots of -Wframe-larger-than warning from other places as well.
> > > >
> > > > > Thanks a lot!
> > > > > Bernard
> > > > >> # gcc --version
> > > > >> gcc (Ubuntu 12.3.0-1ubuntu1~22.04) 12.3.0
> > > > >>
> > > > >> And it still appears after apply this patch on top of 6.8-rc1.
> > > > >>
> > > > >> root@buk:/home/gjiang/linux-mirror# git am
> > > > >>
> > > > ./20240119_bmt_rdma_siw_trim_size_of_page_array_to_max_size_needed.mbx
> > > > >> Applying: RDMA/siw: Trim size of page array to max size needed
> > > > >> root@buk:/home/gjiang/linux-mirror# make
> > M=drivers/infiniband/sw/siw/
> > > > >> -j16 W=1
> > > > >>     CC [M]  drivers/infiniband/sw/siw/siw_qp_tx.o
> > > > >> drivers/infiniband/sw/siw/siw_qp_tx.c: In function ‘siw_tx_hdt’:
> > > > >> drivers/infiniband/sw/siw/siw_qp_tx.c:668:1: warning: the frame size
> > > > of
> > > > >> 1408 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > > > >>     668 | }
> > > > >>         | ^
> > > >
> > > > The patch actually reduced the frame size from 1440 to 1408 though it
> > is
> > > > still larger than 1024.
> > > >
> > >
> > > So in your opinion, does this patch fix the issue of having a
> > > frame size larger than 1024 bytes for a typical build? I am sure
> > > we do not want to optimize the driver for building with KASAN
> > > debug options enabled.
> > 
> > But this is how we are running or supposed to run kernels. In any
> > sane regression run, KASAN is enabled.
> > 
> > I would speculate that most people who run SIW, use it to test their ULPs.
> > 
> > So I would like to see it fixed for them too.
> 
> Understood. I propose to take the patch as I sent for now as a fix
> of the problem under the conditions reported. I'll look into
> restructuring the transmit path to squeeze its size below 1024
> even for KASAN builds. It will require some time.
> 
> Kernel builds with KASAN enabled emit lots of similar compile
> time warnings reporting frame sizes above 1024 bytes. Our core/nldev.c
> alone spills 13 of those. Any action needed? ;)

Yes, report them to ML and someone will fix them :).

Thanks

> 
> Best,
> Bernard
> > 
> > Thanks
> > 
> > >
> > > The original bug report claimed a frame size of 1040 bytes for a
> > > build w/o KASAN, being larger than 1024 bytes by 16 bytes. I
> > > think this patch fixes the reported issue.
> > >
> > > Thanks a lot,
> > > Bernard.
> > >
> > >
> > > > Thanks,
> > > > Guoqing
> > >