[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <3f60f1de-f96c-434b-a4de-355100ce9e79@huawei.com>
Date: Fri, 30 Aug 2024 11:53:11 +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 11:48, Hongbo Li wrote:
>
>
> 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,
>>>>
This is indeed the case. Personally, I think the bcachefs code is quite
difficult to understand (such as too many macro, etc..), with many parts
that could be simplified. I think may be only Kent himself can fully
hold it.
>>>> 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>
>>
Powered by blists - more mailing lists