Messages in this thread | | | Date | Thu, 11 Aug 2022 23:15:32 -0700 | From | Yury Norov <> | Subject | Re: [GIT PULL] Bitmap patches for v6.0-rc1 |
| |
On Sun, Aug 07, 2022 at 05:12:33PM -0700, Linus Torvalds wrote: > On Thu, Aug 4, 2022 at 7:29 PM Yury Norov <yury.norov@gmail.com> wrote: > > > > https://github.com/norov/linux.git/ tags/bitmap-6.0-rc1 > > Grr. > > So I delayed this because I was anticipating pain, but it was worse > than expected. > > I think I've sorted things out, but part of sorting things out was to > just basically undo this: > > > lib/bitmap: change type of bitmap_weight to unsigned long > > which just was pointless and the explanation for it didn't match what it did. > > The explanation was "unsigned makes sense". Ok. > > But what it *did* was not to make the return value unsigned, but to > also expand it from "int" to "long". > > That did *not* make sense, and caused pointless changes to printf strings. > > And - surprise surprise - new users had come in since, so there would > have been even *more* pointless changes to printf strings just to > resolve those.
That's true. I checked the generated code for arm64 and x86, and long is nothing better than int. I was sure that there was a difference, but it's obviously a false memory.
I wish I could rebase everything on top of origin/master before sending a pull request. It would help me make sure that everything will be OK on your side, but it doesn't work for you for other reasons.
> So I basically undid almost all of it. I did leave the "unsigned" > part, but removed the "long" part, so at least the printf strings > noise didn't happen. > > That still caused the type clash in the 'min()' in the > mellanox/mlx4/fw.c file, but at least I couldn't find any new cases of > that pattern, so it seemed acceptable. > > Can bitmap sizes in theory be "unsigned long" bits and thus have > "weight" not fit in "int"? Yes. But it's not a realistic concern, I > feel, and I really really didn't want to deal with this pointless > churn.
Many bitmap functions use int type to store the length, so currently bitmaps can't be larger than INT_MAX, even in theory.
> And honestly, this was enough churn that I seriously just considered > throwing the whole pull request away. And I feel like I will have to > do a few other architecture tests too before I actually push this out, > so I may end up still doing that if that shows something else wrong. > > So for any future pull requests, please > > (a) make them more targeted > > (b) avoids things like that return value change that clearly was just > churn and affected random files in the tree > > because right now I'm left with a "never again" feeling about this all.
I understand that. I'm not happy as well. I'll be more careful with pull requests. Sorry again.
Thanks, Yury
| |