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

[Guoqing Jiang <guoqing.jiang@linux.dev>]

On 1/26/24 01:27, 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?

The warning still appears with KASAN enabled from my side.

> I am sure we do not want to optimize the driver for building with KASAN
> debug options enabled.

Agreed.

> 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.

I am not sure about it after check the relevant bugzilla, maybe the reporter
can verify it.

Thanks,
Guoqing