Messages in this thread | | | Date | Tue, 31 May 2022 18:15:56 -0700 | From | Yury Norov <> | Subject | Re: [GIT PULL] Bitmap patches for v5.19-rc1 |
| |
On Fri, May 27, 2022 at 09:17:21PM -0700, Linus Torvalds wrote: > 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.
Ok, I'll send a new pull with all but bitmap_weight_cmp() patches.
| |