Re: [PATCH 0/5] Minor memory size optimization in debugobjects

[Woody Zhang <woodyzhang666@gmail.com>]

On Thu, Aug 22, 2024 at 02:11:56AM +0200, Thomas Gleixner wrote:
>On Thu, Aug 22 2024 at 07:05, Woody Zhang wrote:
>> As of now, debugobjects framework uses hlist_head and separate spinlock
>> as a hash table bucket. We have hlist_bl_head APIs which embeds a
>> bit_spinlock in head pointer and thus no separate spinlock is required.
>>
>> This patchset first wraps irq variant API for bit_spinlock as well as
>> hlist_bl_lock and several other APIs and macros. Lastly, It replaces
>> hlist APIs with hlist_bl counterparts.
>
>You are telling _what_ your changes are doing, but not _why_ and neither
>_what_ they are trying to achieve.
>
>Aside of that you are failing to explain how replacing a spinlock by a
>hlist bitlock is equivalent to a lockdep covered locking primitive.
>
>It is NOT.
>
>And you have to come up with a convincing argument why this makes sense
>aside of saving an unspecified amount of memory, which you haven't even
>bothered to document. Neither in the changelogs nor in the cover letter.
>
>You also completely fail to provide an analysis why converting the debug
>object locking from a fair and sensible locking implementation to a
>known to be unscalable locking implementation makes sense for a debug
>facility which is used in a lot of hotpaths.
>
>Any attempt to substitute a spinlock with a hlist_bl locking scheme
>needs to come with a proper analysis to demonstrate that:
>
>   1) this is a completely equivalent locking scheme
>
>   2) the resulting loss of lockdep coverage is justified
>
>   3) there is an actual performance benefit
>
>   4) the actual memory savings
>
>Just handwaving about an unspecified amount of memory savings (probably
>in the range of 2 bytes or such) without any of #1 -#3 above is not
>cutting it at all.

All right. I will post a v2 to address these issues and try to give a more
detailed explanation.

BR
Woody

>
>Try again.
>
>Thanks,
>
>        tglx