linux-kernel - Re: [GIT PULL] Bitmap patches for v5.19-rc1

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]

Date:   Fri, 27 May 2022 21:17:21 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Yury Norov <yury.norov@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: Re: [GIT PULL] Bitmap patches for v5.19-rc1

On Fri, May 27, 2022 at 8:44 AM Yury Norov <yury.norov@...il.com> wrote:
>
>       bitmap: add bitmap_weight_{cmp, eq, gt, ge, lt, le} functions

So honestly, I pulled this, looked at the code, and then unpulled it again.

This is not helping.

Making changes like this:

-       if (mm != current->active_mm || cpumask_weight(mm_cpumask(mm)) != 1) {
+       if (mm != current->active_mm || !cpumask_weight_eq(mm_cpumask(mm), 1)) {

only makes the code harder to understand.

And it gets worse:

-               if (cpumask_weight(mask) > cpumask_weight(sibling_mask(cpu)))
+               if (cpumask_weight_gt(mask, cpumask_weight(sibling_mask(cpu))))

is just disgusting. That original line is simple to read and makes
sense. That new replacement really makes you do "Whaa?"

Now, I understand that these kinds of helper functions could make for
slightly more efficient code in that you can break out of the bitmap
scanning early when you have found enough bits set. I get it.

BUT.

 (a) code legibility is really really important

 (b) the places I found this weren't that performance-critical.

 (c) in most cases, the bitmaps in question are one single word

so I'm unpulling this again.

Now, some other parts of the pull were clear improvements. For
example, the hyperv changes like this:

-               if (hc->var_cnt != bitmap_weight((unsigned long
*)&valid_bank_mask, 64))
+               if (hc->var_cnt != hweight64(valid_bank_mask))

were clear improvements where the old code was disgusting, and clearly
improved by the change.

But the "bitmap_weight_cmp()" functions (and the cpumask_weight_cmp()
ones) are just not a direction we want to go.

The special case of zero (ie "cpumask_weight() == 0" ->
"bitmap_empty()") is one thing: making that kind of change tends to
keep the code legible or even make it more understandable. So I didn't
mind that. But I do mind the pointlessly complex new arbitrary weight
comparisons, and the kind of mental cost they have.

There are people in the CS world that think "abstractions are always
good". Those people are very very wrong.

              Linus