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

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] [day] [month] [year] [list]

Message-ID: <20240822102650.GA50561@dev>
Date: Thu, 22 Aug 2024 18:26:50 +0800
From: Woody Zhang <woodyzhang666@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/5] Minor memory size optimization in debugobjects

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