| Message ID | 20250322-consolidate-genmask-v1-0-54bfd36c5643@wanadoo.fr (mailing list archive) |
|---|---|
| Headers | show |
| Series | bits: Split asm and non-asm GENMASK*() and unify definitions | expand |
+ Anshuman Khandual, Catalin Marinas, linux-arm-kernel@lists.infradead.org This series moves GENMASK_U128 out of uapi. ARM is the only proposed user. Add ARM people for visibility. Thanks, Yury On Sat, Mar 22, 2025 at 07:39:35PM +0900, Vincent Mailhol via B4 Relay wrote: > This is a subset of below series: > > bits: Fixed-type GENMASK_U*() and BIT_U*() > Link: https://lore.kernel.org/r/20250308-fixed-type-genmasks-v6-0-f59315e73c29@wanadoo.fr > > Yury suggested to split the above series in two steps: > > #1 Introduce the new fixed type GENMASK_U*() > #2 Consolidate the existing GENMASK*() > > This new series is the resulting step #2 following the split. > > And thus, this series consolidate all the non-asm GENMASK*() so that > they now all depend on GENMASK_TYPE() which was introduced in step #1. > > To do so, I had to split the definition of the asm and non-asm > GENMASK(). I think this is controversial. 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) << (l)) & \ > ((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 looked 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 can. > > And so, after implementing both, the asm and non-asm split seems way > more clean and I think this is the best compromise. > > Aside from the split, the asm's GENMASK() and GENMASK_ULL() are left > untouched. While there are some strong incentives to also simplify > these as pointed by David Laight in this thread: > > https://lore.kernel.org/all/20250309102312.4ff08576@pumpkin/ > > this series deliberately limit its scope to the non-asm variants. > > 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 > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > Vincent Mailhol (3): > bits: split the definition of the asm and non-asm GENMASK*() > bits: unify the non-asm GENMASK*() > test_bits: add tests for __GENMASK() and __GENMASK_ULL() > > include/linux/bits.h | 29 ++++++----------------------- > lib/test_bits.c | 19 +++++++++++++++++++ > 2 files changed, 25 insertions(+), 23 deletions(-) > --- > base-commit: e3f42c436d7e0cb432935fe3ae275dd8d9b60f71 > change-id: 20250320-consolidate-genmask-6cd02abadf82 > prerequisite-change-id: 20250228-fixed-type-genmasks-8d1a555f34e8:v7 > prerequisite-patch-id: 572c05165229640db7dd8fe4d53e1a33ee5dd586 > prerequisite-patch-id: c16d122a487f83e2866a9a669259db097ef46a70 > prerequisite-patch-id: 35f115c0f1b327f1516cfc38b3076e07713df6cd > prerequisite-patch-id: 5fe7058f6ea73b37df75d5c39ad69a4da928058d > prerequisite-patch-id: 82fb628d052ce9f1efac7f3b61eafb2749f95847 > > Best regards, > -- > Vincent Mailhol <mailhol.vincent@wanadoo.fr> >
On 25/03/2025 at 01:11, Yury Norov wrote: > + Anshuman Khandual, Catalin Marinas, linux-arm-kernel@lists.infradead.org > > This series moves GENMASK_U128 out of uapi. ARM is the only proposed > user. Add ARM people for visibility. Actually, not yet. Here, I am decoupling GENMASK_U128() from __GENMASK_U128(), but I did not touch the uapi. After this series, __GENMASK_U128() is not used anymore in the kernel, but I am not brave enough to remove it myself because there is always a risk that some userland code somewhere is relying on it… (...) Yours sincerely, Vincent Mailhol
This is a subset of below series: bits: Fixed-type GENMASK_U*() and BIT_U*() Link: https://lore.kernel.org/r/20250308-fixed-type-genmasks-v6-0-f59315e73c29@wanadoo.fr Yury suggested to split the above series in two steps: #1 Introduce the new fixed type GENMASK_U*() #2 Consolidate the existing GENMASK*() This new series is the resulting step #2 following the split. And thus, this series consolidate all the non-asm GENMASK*() so that they now all depend on GENMASK_TYPE() which was introduced in step #1. To do so, I had to split the definition of the asm and non-asm GENMASK(). I think this is controversial. 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) << (l)) & \ ((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 looked 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 can. And so, after implementing both, the asm and non-asm split seems way more clean and I think this is the best compromise. Aside from the split, the asm's GENMASK() and GENMASK_ULL() are left untouched. While there are some strong incentives to also simplify these as pointed by David Laight in this thread: https://lore.kernel.org/all/20250309102312.4ff08576@pumpkin/ this series deliberately limit its scope to the non-asm variants. 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 Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- Vincent Mailhol (3): bits: split the definition of the asm and non-asm GENMASK*() bits: unify the non-asm GENMASK*() test_bits: add tests for __GENMASK() and __GENMASK_ULL() include/linux/bits.h | 29 ++++++----------------------- lib/test_bits.c | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 23 deletions(-) --- base-commit: e3f42c436d7e0cb432935fe3ae275dd8d9b60f71 change-id: 20250320-consolidate-genmask-6cd02abadf82 prerequisite-change-id: 20250228-fixed-type-genmasks-8d1a555f34e8:v7 prerequisite-patch-id: 572c05165229640db7dd8fe4d53e1a33ee5dd586 prerequisite-patch-id: c16d122a487f83e2866a9a669259db097ef46a70 prerequisite-patch-id: 35f115c0f1b327f1516cfc38b3076e07713df6cd prerequisite-patch-id: 5fe7058f6ea73b37df75d5c39ad69a4da928058d prerequisite-patch-id: 82fb628d052ce9f1efac7f3b61eafb2749f95847 Best regards,