Messages in this thread | | | From | Linus Torvalds <> | Date | Fri, 27 May 2022 21:17:21 -0700 | Subject | Re: [GIT PULL] Bitmap patches for v5.19-rc1 |
| |
On Fri, May 27, 2022 at 8:44 AM Yury Norov <yury.norov@gmail.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
| |