| Message ID | 20250308-fixed-type-genmasks-v6-0-f59315e73c29@wanadoo.fr (mailing list archive) |
|---|---|
| Headers | show |
| Series | bits: Fixed-type GENMASK_U*() and BIT_U*() | expand |
No rush, please allow your reviewers a week or two before submitting a new iteration unless you want to disregard the previous version for some reason, of course. This will not get into the upcoming merge window, anyways. So, what should I do? Go through the v5 and all discussions in there, or just jump on this? On Sat, Mar 08, 2025 at 01:48:47AM +0900, Vincent Mailhol via B4 Relay wrote: > Introduce some fixed width variant of the GENMASK() and the BIT() > macros in bits.h. Note that the main goal is not to get the correct > type, but rather to enforce more checks at compile time. For example: > > GENMASK_U16(16, 0) > > will raise a build bug. > > This series is a continuation of: > > https://lore.kernel.org/intel-xe/20240208074521.577076-1-lucas.demarchi@intel.com > > from Lucas De Marchi. Above series is one year old. I really think > that this was a good idea and I do not want this series to die. So I > am volunteering to revive it. > > Meanwhile, many changes occurred in bits.h. The most significant > change is that __GENMASK() was moved to the uapi headers. > > In v4 and onward, I introduce one big change: split the definition of > the asm and non-asm GENMASK(). I think this is controversial. > Especially, Yury commented that he did not want such split. So I > initially implemented a first draft in which both the asm and non-asm > version would rely on the same helper macro, i.e. adding this: > > #define __GENMASK_TYPE(t, w, h, l) \ > (((t)~_ULL(0) - ((t)1 << (l)) + 1) & \ > ((t)~_ULL(0) >> (w - 1 - (h)))) > > to uapi/bits.h. And then, the different GENMASK()s would look like > this: > > #define __GENMASK(h, l) __GENMASK_TYPE(unsigned long, __BITS_PER_LONG, h, l) > > and so on. > > I implemented it, and the final result looks quite ugly. Not only do > we need to manually provide the width each time, the biggest concern > is that adding this to the uapi is asking for trouble. Who knows how > people are going to use this? And once it is in the uapi, there is > virtually no way back. > > Adding to this, that macro can not even be generalized to u128 > integers, whereas after the split, it is. > > Finally, I do not think it makes sense to expose the fixed width > variants to the asm. The fixed width integers type are a C > concept. For asm, the long and long long variants seems sufficient. > > And so, after implementing both, the asm and non-asm split seems way > more clean and I think this is the best compromise. Let me know what > you think :) > > As requested, here are the bloat-o-meter stats: > > $ ./scripts/bloat-o-meter vmlinux_before.o vmlinux_after.o > add/remove: 0/0 grow/shrink: 4/2 up/down: 5/-4 (1) > Function old new delta > intel_psr_invalidate 666 668 +2 > mst_stream_compute_config 1652 1653 +1 > intel_psr_flush 977 978 +1 > intel_dp_compute_link_config 1327 1328 +1 > cfg80211_inform_bss_data 5109 5108 -1 > intel_drrs_activate 379 376 -3 > Total: Before=22723481, After=22723482, chg +0.00% > > (done with GCC 12.4.1 on an x86_64 defconfig) > > -- > 2.43.0 > > --- > Changes from v5: > > - Update the cover letter message. I was still refering to > GENMASK_t() instead of GENMASK_TYPE(). > > - Add a comment in the cover letter to explain that a common > GENMASK_TYPE() for C and asm wouldn't allow to generate the u128 > variant. > > - Restore the comment saying that BUILD_BUG_ON() is not available in > asm code. > > - Add a FIXME message to highlight the absence of the asm GENMASK*() > unit tests. > > - Use git's histogram diff algorithm > > - Link to v5: https://lore.kernel.org/r/20250306-fixed-type-genmasks-v5-0-b443e9dcba63@wanadoo.fr > > Changes from v4: > > - Rebase on https://github.com/norov/linux/tree/bitmap-for-next > > - Rename GENMASK_t() to GENMASK_TYPE() > > - First patch of v4 (the typo fix 'init128' -> 'int128') is removed > because it was resent separately in: > https://lore.kernel.org/all/20250305-fix_init128_typo-v1-1-cbe5b8e54e7d@wanadoo.fr > > - Replace the (t)~ULL(0) by type_max(t). This way, GENMASK_TYPE() > can now be used to generate GENMASK_U128(). > > - Get rid of the unsigned int cast for the U8 and U16 variants. > > - Add the BIT_TYPE() helper macro. > > - Link to v4: https://lore.kernel.org/r/20250305-fixed-type-genmasks-v4-0-1873dcdf6723@wanadoo.fr > > Changes from v3: > > - Rebase on v6.14-rc5 > > - Fix a typo in GENMASK_U128() comment. > > - Split the asm and non-asm definition of > > - Replace ~0ULL by ~ULL(0) > > - Since v3, __GENMASK() was moved to the uapi and people started > using directly. Introduce GENMASK_t() instead. > > - Link to v3: https://lore.kernel.org/intel-xe/20240208074521.577076-1-lucas.demarchi@intel.com > > Changes from v2: > > - Document both in commit message and code about the strict type > checking and give examples how it´d break with invalid params. > > - Link to v2: https://lore.kernel.org/intel-xe/20240124050205.3646390-1-lucas.demarchi@intel.com > > Link to v1: https://lore.kernel.org/intel-xe/20230509051403.2748545-1-lucas.demarchi@intel.com > > --- > Lucas De Marchi (3): > bits: introduce fixed-type BIT_U*() > drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*() > test_bits: add tests for GENMASK_U*() > > Vincent Mailhol (3): > bits: split the definition of the asm and non-asm GENMASK() > test_bits: add tests for __GENMASK() and __GENMASK_ULL() > test_bits: add tests for BIT_U*() > > Yury Norov (1): > bits: introduce fixed-type genmasks > > drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++------------------------------- > include/linux/bitops.h | 1 - > include/linux/bits.h | 91 ++++++++++++++++++++--------- > lib/test_bits.c | 49 ++++++++++++++++ > 4 files changed, 124 insertions(+), 125 deletions(-) > --- > base-commit: 0312e94abe484b9ee58c32d2f8ba177e04955b35 > change-id: 20250228-fixed-type-genmasks-8d1a555f34e8 > > Best regards, > -- > Vincent Mailhol <mailhol.vincent@wanadoo.fr> >
On Fri, Mar 07, 2025 at 12:18:02PM -0500, Yury Norov wrote: > No rush, please allow your reviewers a week or two before submitting > a new iteration unless you want to disregard the previous version for > some reason, of course. This will not get into the upcoming merge > window, anyways. > > So, what should I do? Go through the v5 and all discussions in there, > or just jump on this? There is also question to you. Are we going to leave with U128 variants or is it subject to remove? If the latter, can you issue a formal patch?
On Fri, Mar 07, 2025 at 07:43:57PM +0200, Andy Shevchenko wrote: > On Fri, Mar 07, 2025 at 12:18:02PM -0500, Yury Norov wrote: > > No rush, please allow your reviewers a week or two before submitting > > a new iteration unless you want to disregard the previous version for > > some reason, of course. This will not get into the upcoming merge > > window, anyways. > > > > So, what should I do? Go through the v5 and all discussions in there, > > or just jump on this? > > There is also question to you. Are we going to leave with U128 variants or is > it subject to remove? If the latter, can you issue a formal patch? I asked Anshuman about it as he's the only person interested in it. Will wait for a _usual_ few weeks for reply before making any conclusions. If you know anyone relevant in ARM or everywhere else, feel free to loop them.
On Fri, Mar 07, 2025 at 12:48:36PM -0500, Yury Norov wrote: > On Fri, Mar 07, 2025 at 07:43:57PM +0200, Andy Shevchenko wrote: > > On Fri, Mar 07, 2025 at 12:18:02PM -0500, Yury Norov wrote: > > > No rush, please allow your reviewers a week or two before submitting > > > a new iteration unless you want to disregard the previous version for > > > some reason, of course. This will not get into the upcoming merge > > > window, anyways. > > > > > > So, what should I do? Go through the v5 and all discussions in there, > > > or just jump on this? > > > > There is also question to you. Are we going to leave with U128 variants or is > > it subject to remove? If the latter, can you issue a formal patch? > > I asked Anshuman about it as he's the only person interested in it. Will wait > for a _usual_ few weeks for reply before making any conclusions. If you know > anyone relevant in ARM or everywhere else, feel free to loop them. I see, yep, we still have time for that, let's wait a bit.
On 08/03/2025 at 02:50, Andy Shevchenko wrote: > On Fri, Mar 07, 2025 at 12:48:36PM -0500, Yury Norov wrote: >> On Fri, Mar 07, 2025 at 07:43:57PM +0200, Andy Shevchenko wrote: >>> On Fri, Mar 07, 2025 at 12:18:02PM -0500, Yury Norov wrote: >>>> No rush, please allow your reviewers a week or two before submitting >>>> a new iteration unless you want to disregard the previous version for >>>> some reason, of course. This will not get into the upcoming merge >>>> window, anyways. Ack. I was not expecting this to go into the next merge windows either. Most of the feedback was not on the actual code but just on the naming, the code comments or the patch descriptions. I normally wait longer on the first version of a series but I tend to do kick re-spin when addressing the nitpicks. But message taken! I will wait a couple of weeks before the next iteration. >>>> So, what should I do? Go through the v5 and all discussions in there, >>>> or just jump on this? The code is the same between v5 and v6. There is this message from David in which he suggested to make some changes to the uapi __GENMASK() and __GENMASK_ULL() and to which I commented that I was not confident doing such changes: https://lore.kernel.org/all/20250306192331.2701a029@pumpkin/t/#u Aside from the above, you wouldn't miss much by directly jumping on this v6. >>> There is also question to you. Are we going to leave with U128 variants or is >>> it subject to remove? If the latter, can you issue a formal patch? >> >> I asked Anshuman about it as he's the only person interested in it. Will wait >> for a _usual_ few weeks for reply before making any conclusions. If you know >> anyone relevant in ARM or everywhere else, feel free to loop them. > > I see, yep, we still have time for that, let's wait a bit. Ack. Andy, I already addressed your last comments in my local tree. I will now wait for others' feedback. Yours sincerely, Vincent Mailhol
Introduce some fixed width variant of the GENMASK() and the BIT() macros in bits.h. Note that the main goal is not to get the correct type, but rather to enforce more checks at compile time. For example: GENMASK_U16(16, 0) will raise a build bug. This series is a continuation of: https://lore.kernel.org/intel-xe/20240208074521.577076-1-lucas.demarchi@intel.com from Lucas De Marchi. Above series is one year old. I really think that this was a good idea and I do not want this series to die. So I am volunteering to revive it. Meanwhile, many changes occurred in bits.h. The most significant change is that __GENMASK() was moved to the uapi headers. In v4 and onward, I introduce one big change: split the definition of the asm and non-asm GENMASK(). I think this is controversial. Especially, Yury commented that he did not want such split. So I initially implemented a first draft in which both the asm and non-asm version would rely on the same helper macro, i.e. adding this: #define __GENMASK_TYPE(t, w, h, l) \ (((t)~_ULL(0) - ((t)1 << (l)) + 1) & \ ((t)~_ULL(0) >> (w - 1 - (h)))) to uapi/bits.h. And then, the different GENMASK()s would look like this: #define __GENMASK(h, l) __GENMASK_TYPE(unsigned long, __BITS_PER_LONG, h, l) and so on. I implemented it, and the final result looks quite ugly. Not only do we need to manually provide the width each time, the biggest concern is that adding this to the uapi is asking for trouble. Who knows how people are going to use this? And once it is in the uapi, there is virtually no way back. Adding to this, that macro can not even be generalized to u128 integers, whereas after the split, it is. Finally, I do not think it makes sense to expose the fixed width variants to the asm. The fixed width integers type are a C concept. For asm, the long and long long variants seems sufficient. And so, after implementing both, the asm and non-asm split seems way more clean and I think this is the best compromise. Let me know what you think :) As requested, here are the bloat-o-meter stats: $ ./scripts/bloat-o-meter vmlinux_before.o vmlinux_after.o add/remove: 0/0 grow/shrink: 4/2 up/down: 5/-4 (1) Function old new delta intel_psr_invalidate 666 668 +2 mst_stream_compute_config 1652 1653 +1 intel_psr_flush 977 978 +1 intel_dp_compute_link_config 1327 1328 +1 cfg80211_inform_bss_data 5109 5108 -1 intel_drrs_activate 379 376 -3 Total: Before=22723481, After=22723482, chg +0.00% (done with GCC 12.4.1 on an x86_64 defconfig) -- 2.43.0 --- Changes from v5: - Update the cover letter message. I was still refering to GENMASK_t() instead of GENMASK_TYPE(). - Add a comment in the cover letter to explain that a common GENMASK_TYPE() for C and asm wouldn't allow to generate the u128 variant. - Restore the comment saying that BUILD_BUG_ON() is not available in asm code. - Add a FIXME message to highlight the absence of the asm GENMASK*() unit tests. - Use git's histogram diff algorithm - Link to v5: https://lore.kernel.org/r/20250306-fixed-type-genmasks-v5-0-b443e9dcba63@wanadoo.fr Changes from v4: - Rebase on https://github.com/norov/linux/tree/bitmap-for-next - Rename GENMASK_t() to GENMASK_TYPE() - First patch of v4 (the typo fix 'init128' -> 'int128') is removed because it was resent separately in: https://lore.kernel.org/all/20250305-fix_init128_typo-v1-1-cbe5b8e54e7d@wanadoo.fr - Replace the (t)~ULL(0) by type_max(t). This way, GENMASK_TYPE() can now be used to generate GENMASK_U128(). - Get rid of the unsigned int cast for the U8 and U16 variants. - Add the BIT_TYPE() helper macro. - Link to v4: https://lore.kernel.org/r/20250305-fixed-type-genmasks-v4-0-1873dcdf6723@wanadoo.fr Changes from v3: - Rebase on v6.14-rc5 - Fix a typo in GENMASK_U128() comment. - Split the asm and non-asm definition of - Replace ~0ULL by ~ULL(0) - Since v3, __GENMASK() was moved to the uapi and people started using directly. Introduce GENMASK_t() instead. - Link to v3: https://lore.kernel.org/intel-xe/20240208074521.577076-1-lucas.demarchi@intel.com Changes from v2: - Document both in commit message and code about the strict type checking and give examples how it´d break with invalid params. - Link to v2: https://lore.kernel.org/intel-xe/20240124050205.3646390-1-lucas.demarchi@intel.com Link to v1: https://lore.kernel.org/intel-xe/20230509051403.2748545-1-lucas.demarchi@intel.com --- Lucas De Marchi (3): bits: introduce fixed-type BIT_U*() drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*() test_bits: add tests for GENMASK_U*() Vincent Mailhol (3): bits: split the definition of the asm and non-asm GENMASK() test_bits: add tests for __GENMASK() and __GENMASK_ULL() test_bits: add tests for BIT_U*() Yury Norov (1): bits: introduce fixed-type genmasks drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++------------------------------- include/linux/bitops.h | 1 - include/linux/bits.h | 91 ++++++++++++++++++++--------- lib/test_bits.c | 49 ++++++++++++++++ 4 files changed, 124 insertions(+), 125 deletions(-) --- base-commit: 0312e94abe484b9ee58c32d2f8ba177e04955b35 change-id: 20250228-fixed-type-genmasks-8d1a555f34e8 Best regards,