linux-kernel - Re: [PATCH] bcachefs: Remove the handling of bch2_trans_iter_exit() in __bch2_bkey_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0293d72e-8d57-42c2-bb11-a7a553675ae6@huawei.com>
Date: Fri, 30 Aug 2024 11:48:45 +0800
From: Hongbo Li <lihongbo22@...wei.com>
To: Youling Tang <youling.tang@...ux.dev>
CC: <linux-bcachefs@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Youling
 Tang <tangyouling@...inos.cn>, Kent Overstreet <kent.overstreet@...ux.dev>
Subject: Re: [PATCH] bcachefs: Remove the handling of bch2_trans_iter_exit()
 in __bch2_bkey_get_iter()



On 2024/8/30 9:19, Youling Tang wrote:
> On 2024/8/23 22:51, Kent Overstreet wrote:
> 
>> On Fri, Aug 23, 2024 at 02:07:20PM GMT, Youling Tang wrote:
>>> On 23/08/2024 11:55, Kent Overstreet wrote:
>>>> On Fri, Aug 23, 2024 at 11:19:55AM GMT, Youling Tang wrote:
>>>>> From: Youling Tang <tangyouling@...inos.cn>
>>>>>
>>>>> - Reduces bkey_err() calls.
>>>>> - Avoid redundant calls to bch2_trans_iter_exit() in some functions.
>>>> no, a function that returns an error should clean up after itself
>>> Yes, functions should self-clean when they fail.
>>>
>>> However, there are repeated calls to bch2_trans_iter_exit in
>>> some functions, take lookup_inode() as an example,
>>>
>>> When bkey_err(k) returns a non-zero, call bch2_trans_iter_exit()
>>> once in bch2_bkey_get_iter(). It is then called again in
>>> lookup_inode() via 'goto err'. (We can correct it by simply changing
>>> it to 'return ret', but there are many similar cases.)
>> I'm aware, but I'm not looking to microoptimize at the expense of making
>> the code more fragile and less clear, especially right now when the
>> priority is stabilizing and fixing bugs.
>>
>> If you were also doing performance testing and could show that it
>> makes a measurable difference I'd consider it. Did you even look at the
>> assembly output for any of these functions? CSE might be optimizing away
>> the redundant calls.
> I haven't performed the corresponding performance testing. Looking at
> the assembly code, taking `lookup_inode()` as an example,
> 
I think his point is that your modification is error-free, but improving 
readability is not a high priority at the moment unless it actually 
affects performance. There might currently be a shortage of manpower, 
with many features to be completed and bugs to be fixed.
> Before the patch,
>      142f:       74 96 je     13c7 <lookup_inode+0x117>
>      1431:       48 8d b5 68 ff ff ff lea    -0x98(%rbp),%rsi
>      1438:       4c 89 e7 mov    %r12,%rdi
>      143b:       e8 00 00 00 00 call   1440 <lookup_inode+0x190>
>      1440:       eb b4 jmp    13f6 <lookup_inode+0x146>
>      1442:       e8 00 00 00 00 call   1447 <lookup_inode+0x197>
>      1447:       66 0f 1f 84 00 00 00 nopw   0x0(%rax,%rax,1)
>      144e:       00 00
> 
> After,
>      111f:       74 96 je     10b7 <lookup_inode+0x117>
>      1121:       eb c3 jmp    10e6 <lookup_inode+0x146>
>      1123:       e8 00 00 00 00 call   1128 <lookup_inode+0x188>
>      1128:       0f 1f 84 00 00 00 00 nopl   0x0(%rax,%rax,1)
>      112f:       00
> 
> The following three assembly instructions have been reduced,
>      1431:       48 8d b5 68 ff ff ff lea    -0x98(%rbp),%rsi
>      1438:       4c 89 e7 mov    %r12,%rdi
>      143b:       e8 00 00 00 00 call   1440 <lookup_inode+0x190>
>