Compile Error fs/nfsd/nfs4state.o - clamp() low limit slotsize greater than high limit total_avail/scale_factor
Subsystems (3)
Mike-SPC writes via Kernel.org Bugzilla:
Hi there,
with kernel version > 6.1.156 I get the following error by compiling it (make bzImage) on a 32bit platform:
CALL scripts/checksyscalls.sh
CC fs/nfsd/nfs4state.o
In file included from <command-line>:
In function 'nfsd4_get_drc_mem',
inlined from 'check_forechannel_attrs' at fs/nfsd/nfs4state.c:3539:16,
inlined from 'nfsd4_create_session' at fs/nfsd/nfs4state.c:3612:11:././include/linux/compiler_types.h:375:38: error: call to '__compiletime_assert_587' declared with attribute error: clamp() low limit slotsize greater than high limit total_avail/scale_factor
375 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^././include/linux/compiler_types.h:356:4: note: in definition of macro '__compiletime_assert'
356 | prefix ## suffix(); \
| ^~~~~~
././include/linux/compiler_types.h:375:2: note: in expansion of macro '_compiletime_assert'
375 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
./include/linux/minmax.h:188:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
188 | BUILD_BUG_ON_MSG(statically_true(ulo > uhi), \
| ^~~~~~~~~~~~~~~~
./include/linux/minmax.h:195:2: note: in expansion of macro '__clamp_once'
195 | __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
| ^~~~~~~~~~~~
./include/linux/minmax.h:218:36: note: in expansion of macro '__careful_clamp'
218 | #define clamp_t(type, val, lo, hi) __careful_clamp(type, val, lo, hi)
| ^~~~~~~~~~~~~~~
fs/nfsd/nfs4state.c:1825:10: note: in expansion of macro 'clamp_t'
1825 | avail = clamp_t(unsigned long, avail, slotsize,
| ^~~~~~~make[3]: *** [scripts/Makefile.build:250: fs/nfsd/nfs4state.o] Error 1 make[2]: *** [scripts/Makefile.build:503: fs/nfsd] Error 2 make[1]: *** [scripts/Makefile.build:503: fs] Error 2
make: *** [Makefile:2025: .] Error 2
I'm not a coder, so I checked it with OpenAI, which throwed out the following patch:
fs/nfsd/nfs4state.c +6 -2
@@ -1822,8 +1822,12 @@ static u32 nfsd4_get_drc_mem(struct nfsd4_channel_attrs *ca, struct nfsd_net *nn*/ scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads); - avail = clamp_t(unsigned long, avail, slotsize, - total_avail/scale_factor); +/* Ensure hi >= lo per "give at least one slot" policy */ +do { +unsigned long hi = total_avail / scale_factor; +if (hi < slotsize) hi = slotsize; + avail = clamp_t(unsigned long, avail, slotsize, hi); + }while (0 ); num =min_t (int, num, avail / slotsize);
After implementing it, I'm able to compile the kernel.
But, as I mentioned before, I'm not a coder, so I cannot test the patch from a programming perspective.
Therefore, it would be nice if a patch could be made available by a human. :)
Thanks in advance - regards,
Michael
View: https://bugzilla.kernel.org/show_bug.cgi?id=220745#c0
You can reply to this message to join the discussion.
Signature · 2 lines
Deet-doot-dot, I am a bot. Kernel.org Bugzilla (bugspray 0.1-dev)
Chuck Lever
maintainerQuoted 66 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
Hi there,
with kernel version > 6.1.156 I get the following error by compiling it (make bzImage) on a 32bit platform:
CALL
scripts/checksyscalls.sh
CCfs/nfsd/nfs4state.o
In file included from <command-line>:
In function 'nfsd4_get_drc_mem',
inlined from 'check_forechannel_attrs' atfs/nfsd/nfs4state.c:3539:16,
inlined from 'nfsd4_create_session' atfs/nfsd/nfs4state.c:3612:11:././include/linux/compiler_types.h:375:38: error: call to '__compiletime_assert_587' declared with attribute error:clamp()low limit slotsize greater than high limit total_avail/scale_factor
375 |_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^././include/linux/compiler_types.h:356:4: note: in definition of macro '__compiletime_assert'356 | prefix ## suffix(); \ | ^~~~~~ ././include/linux/compiler_types.h:375:2: note: in expansion of macro '_compiletime_assert' 375 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ ./include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ ./include/linux/minmax.h:188:2: note: in expansion of macro 'BUILD_BUG_ON_MSG' 188 | BUILD_BUG_ON_MSG(statically_true(ulo > uhi), \ | ^~~~~~~~~~~~~~~~ ./include/linux/minmax.h:195:2: note: in expansion of macro '__clamp_once' 195 | __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_)) | ^~~~~~~~~~~~ ./include/linux/minmax.h:218:36: note: in expansion of macro '__careful_clamp' 218 | #define clamp_t(type, val, lo, hi) __careful_clamp(type, val, lo, hi) | ^~~~~~~~~~~~~~~ fs/nfsd/nfs4state.c:1825:10: note: in expansion of macro 'clamp_t' 1825 | avail = clamp_t(unsigned long, avail, slotsize, | ^~~~~~~make[3]: *** [
scripts/Makefile.build:250:fs/nfsd/nfs4state.o] Error 1 make[2]: *** [scripts/Makefile.build:503: fs/nfsd] Error 2 make[1]: *** [scripts/Makefile.build:503: fs] Error 2
make: *** [Makefile:2025: .] Error 2I'm not a coder, so I checked it with OpenAI, which throwed out the following patch:
fs/nfsd/nfs4state.c +6
-2@@ -1822,8 +1822,12 @@ static u32 nfsd4_get_drc_mem(struct nfsd4_channel_attrs *ca, struct nfsd_net *nn*/ scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads); - avail = clamp_t(unsigned long, avail, slotsize, - total_avail/scale_factor); +/* Ensure hi >= lo per "give at least one slot" policy */ +do { +unsigned long hi = total_avail / scale_factor; +if (hi < slotsize) hi = slotsize; + avail = clamp_t(unsigned long, avail, slotsize, hi); + }while (0 ); num =min_t (int, num, avail / slotsize);After implementing it, I'm able to compile the kernel.
But, as I mentioned before, I'm not a coder, so I cannot test the patch from a programming perspective.
Quoted 7 lines
Therefore, it would be nice if a patch could be made available by a human. :)
Thanks in advance - regards,
MichaelView: https://bugzilla.kernel.org/show_bug.cgi?id=220745#c0
You can reply to this message to join the discussion.
The failing code was introduced by commit 2030ca560c5f ("nfsd: degraded slot-count more gracefully as allocation nears exhaustion.") in v5.4. It is not a backport to v6.1. I don't see any changes to that code until b5fba969a2e4 ("nfsd: remove artificial limits on the session-based DRC"), when it was removed whole-sale.
That means we don't have this code to patch in upstream. If there is a fix to be made in NFSD, we will have to create one-off patches for each of the LTS kernels.
It appears that there have been several clean-ups to clamp_t and friends since v6.1, and they have been mostly backported to v6.1.y. It's hard to say without a bisect whether one of those is the reason for the breakage.
Neil, according to the last touch rule, you're the owner of this code ;-) Do you have any thoughts? I'm not quite sure what the compile-time assertion is complaining about.
Signature · 1 line
Chuck Lever
Quoted 6 lines · Chuck Lever
Quoted 5 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
Hi there,
with kernel version > 6.1.156 I get the following error by compiling it (make bzImage) on a 32bit platform:
Which platform? Which compiler?
I just build nfsd on 6.1.158 for mips (32 bit) with
$ /opt/cross/bin/mipsel-unknown-linux-gnu-gcc -v Using built-in specs. COLLECT_GCC=/opt/cross/bin/mipsel-unknown-linux-gnu-gcc COLLECT_LTO_WRAPPER=/opt/cross/libexec/gcc/mipsel-unknown-linux-gnu/7.2.0/lto-wrapper Target: mipsel-unknown-linux-gnu Configured with: ../configure --target=mipsel-unknown-linux-gnu --prefix=/opt/cross --enable-languages=c --without-headers --with-gnu-ld --with-gnu-as --disable-shared --disable-threads --disable-libmudflap --disable-libgomp --disable-libssp --disable-libquadmath --disable-libatomic Thread model: single gcc version 7.2.0 (GCC)
and it worked fine.
Have you found a 6.1.y kernel for which the build doesn't fail?
There is no way that code could reasonably trigger that warning. I suspect a compiler bug/incompatibility.
If you compiler is GCC earlier than 5.1 or Clang before 11.0.0 then upgrade your compiler.
If it is newer, then report the incompatibility to [email protected].
NeilBrown
Quoted 93 lines
Quoted 61 lines
CALL
scripts/checksyscalls.sh
CCfs/nfsd/nfs4state.o
In file included from <command-line>:
In function 'nfsd4_get_drc_mem',
inlined from 'check_forechannel_attrs' atfs/nfsd/nfs4state.c:3539:16,
inlined from 'nfsd4_create_session' atfs/nfsd/nfs4state.c:3612:11:././include/linux/compiler_types.h:375:38: error: call to '__compiletime_assert_587' declared with attribute error:clamp()low limit slotsize greater than high limit total_avail/scale_factor
375 |_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^././include/linux/compiler_types.h:356:4: note: in definition of macro '__compiletime_assert'356 | prefix ## suffix(); \ | ^~~~~~ ././include/linux/compiler_types.h:375:2: note: in expansion of macro '_compiletime_assert' 375 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ ./include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ ./include/linux/minmax.h:188:2: note: in expansion of macro 'BUILD_BUG_ON_MSG' 188 | BUILD_BUG_ON_MSG(statically_true(ulo > uhi), \ | ^~~~~~~~~~~~~~~~ ./include/linux/minmax.h:195:2: note: in expansion of macro '__clamp_once' 195 | __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_)) | ^~~~~~~~~~~~ ./include/linux/minmax.h:218:36: note: in expansion of macro '__careful_clamp' 218 | #define clamp_t(type, val, lo, hi) __careful_clamp(type, val, lo, hi) | ^~~~~~~~~~~~~~~ fs/nfsd/nfs4state.c:1825:10: note: in expansion of macro 'clamp_t' 1825 | avail = clamp_t(unsigned long, avail, slotsize, | ^~~~~~~make[3]: *** [
scripts/Makefile.build:250:fs/nfsd/nfs4state.o] Error 1 make[2]: *** [scripts/Makefile.build:503: fs/nfsd] Error 2 make[1]: *** [scripts/Makefile.build:503: fs] Error 2
make: *** [Makefile:2025: .] Error 2I'm not a coder, so I checked it with OpenAI, which throwed out the following patch:
fs/nfsd/nfs4state.c +6
-2@@ -1822,8 +1822,12 @@ static u32 nfsd4_get_drc_mem(struct nfsd4_channel_attrs *ca, struct nfsd_net *nn*/ scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads); - avail = clamp_t(unsigned long, avail, slotsize, - total_avail/scale_factor); +/* Ensure hi >= lo per "give at least one slot" policy */ +do { +unsigned long hi = total_avail / scale_factor; +if (hi < slotsize) hi = slotsize; + avail = clamp_t(unsigned long, avail, slotsize, hi); + }while (0 ); num =min_t (int, num, avail / slotsize);After implementing it, I'm able to compile the kernel.
But, as I mentioned before, I'm not a coder, so I cannot test the patch from a programming perspective.Quoted 7 lines
Therefore, it would be nice if a patch could be made available by a human. :)
Thanks in advance - regards,
MichaelView: https://bugzilla.kernel.org/show_bug.cgi?id=220745#c0
You can reply to this message to join the discussion.The failing code was introduced by commit
2030ca560c5f("nfsd: degraded slot-count more gracefully as allocation nears exhaustion.") in v5.4. It is not a backport to v6.1. I don't see any changes to that code untilb5fba969a2e4("nfsd: remove artificial limits on the session-based DRC"), when it was removed whole-sale.That means we don't have this code to patch in upstream. If there is a fix to be made in NFSD, we will have to create one-off patches for each of the LTS kernels.
It appears that there have been several clean-ups to
clamp_tand friends since v6.1, and they have been mostly backported to v6.1.y. It's hard to say without a bisect whether one of those is the reason for the breakage.Neil, according to the last touch rule, you're the owner of this code ;-) Do you have any thoughts? I'm not quite sure what the compile-time assertion is complaining about.
Signature · 1 line
Chuck Lever
Mike-SPC writes via Kernel.org Bugzilla:
(In reply to Bugspray Bot from comment #2)
Quoted 32 lines
NeilBrown <[email protected]> replies to comment #1:
Quoted 7 lines · Chuck Lever
Quoted 5 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
Hi there,
with kernel version > 6.1.156 I get the following error by compiling it
(make bzImage) on a 32bit platform:
Which platform? Which compiler?
I just build nfsd on 6.1.158 for mips (32 bit) with
$ /opt/cross/bin/mipsel-unknown-linux-gnu-gcc -v Using built-in specs. COLLECT_GCC=/opt/cross/bin/mipsel-unknown-linux-gnu-gcc COLLECT_LTO_WRAPPER=/opt/cross/libexec/gcc/mipsel-unknown-linux-gnu/7.2.0/ lto-wrapper Target: mipsel-unknown-linux-gnu Configured with: ../configure --target=mipsel-unknown-linux-gnu --prefix=/opt/cross --enable-languages=c --without-headers --with-gnu-ld --with-gnu-as --disable-shared --disable-threads --disable-libmudflap --disable-libgomp --disable-libssp --disable-libquadmath --disable-libatomic Thread model: single gcc version 7.2.0 (GCC)and it worked fine.
Have you found a 6.1.y kernel for which the build doesn't fail?
Quoted 8 lines
There is no way that code could reasonably trigger that warning. I suspect a compiler bug/incompatibility.
If you compiler is GCC earlier than 5.1 or Clang before 11.0.0 then upgrade your compiler.
If it is newer, then report the incompatibility to [email protected].NeilBrown
Oh - sorry for the missing information.
Platform: i686 (32bit).
Used GCC Version:
$ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/libexec/gcc/i686-pc-linux-gnu/9.3.0/lto-wrapper Target: i686-pc-linux-gnu Configured with: ../configure --prefix=/usr --enable-languages=c,c++ --disable-multilib --disable-bootstrap --with-system-zlib Thread model: posix gcc version 9.3.0 (GCC)
So it seems to be newer. :)
Quoted 112 lines
Quoted 8 lines
Quoted 8 lines
CALL
scripts/checksyscalls.sh
CCfs/nfsd/nfs4state.o
In file included from <command-line>:
In function 'nfsd4_get_drc_mem',
inlined from 'check_forechannel_attrs' at
fs/nfsd/nfs4state.c:3539:16,Quoted 15 lines
Quoted 2 lines
inlined from 'nfsd4_create_session' at fs/nfsd/nfs4state.c:3612:11:
././include/linux/compiler_types.h:375:38: error: call to'
__compiletime_assert_587' declared with attribute error:clamp()low limit slotsize greater than high limit total_avail/scale_factorQuoted 1 line
375 | _compiletime_assert(condition, msg,
__compiletime_assert_,
__COUNTER__)Quoted 2 lines
| ^
././include/linux/compiler_types.h:356:4: note: in definition of macro'
__compiletime_assert'Quoted 3 lines
356 | prefix ## suffix(); \ | ^~~~~~
././include/linux/compiler_types.h:375:2: note: in expansion of macro'
_compiletime_assert'Quoted 1 line
375 | _compiletime_assert(condition, msg,
__compiletime_assert_,
__COUNTER__)Quoted 11 lines
Quoted 2 lines
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro'
compiletime_assert'Quoted 1 line
39 | #define
BUILD_BUG_ON_MSG(cond, msg)compiletime_assert(!(cond),msg)
Quoted 2 lines
| ^~~~~~~~~~~~~~~~~~
./include/linux/minmax.h:188:2: note: in expansion of macro'
BUILD_BUG_ON_MSG'Quoted 3 lines
188 | BUILD_BUG_ON_MSG(statically_true(ulo > uhi), \ | ^~~~~~~~~~~~~~~~
./include/linux/minmax.h:195:2: note: in expansion of macro'
__clamp_once'Quoted 1 line
Quoted 1 line
195 | __clamp_once(type, val, lo, hi,
__UNIQUE_ID(v_),
__UNIQUE_ID(l_),Quoted 17 lines
__UNIQUE_ID(h_))Quoted 2 lines
| ^~~~~~~~~~~~
./include/linux/minmax.h:218:36: note: in expansion of macro'
__careful_clamp'Quoted 1 line
218 | #define
clamp_t(type, val, lo, hi)__careful_clamp(type, val, lo,hi)
Quoted 11 lines
| ^~~~~~~~~~~~~~~ fs/nfsd/nfs4state.c:1825:10: note: in expansion of macro 'clamp_t' 1825 | avail = clamp_t(unsigned long, avail, slotsize, | ^~~~~~~make[3]: *** [
scripts/Makefile.build:250:fs/nfsd/nfs4state.o] Error 1 make[2]: *** [scripts/Makefile.build:503: fs/nfsd] Error 2 make[1]: *** [scripts/Makefile.build:503: fs] Error 2
make: *** [Makefile:2025: .] Error 2Quoted 9 lines
Quoted 1 line
I'm not a coder, so I checked it with OpenAI, which throwed out the
following patch:
fs/nfsd/nfs4state.c static u32 nfsd4_get_drc_mem(struct
Quoted 4 lines
fs/nfsd/nfs4state.c +0
-0@@ -1822,8 +1822,12 @@ static u32 nfsd4_get_drc_mem(struct
nfsd4_channel_attrs*ca, structnfsd_net*nnQuoted 2 lines
fs/nfsd/nfs4state.c +0
-0*/ scale_factor = max_t(unsigned int, 8,
nn->nfsd_serv->sv_nrthreads);Quoted 13 lines
Quoted 13 lines
fs/nfsd/nfs4state.c +6
-2- avail = clamp_t(unsigned long, avail, slotsize, - total_avail/scale_factor); +/* Ensure hi >= lo per "give at least one slot" policy */ +do { +unsigned long hi = total_avail / scale_factor; +if (hi < slotsize) hi = slotsize; + avail = clamp_t(unsigned long, avail, slotsize, hi); + }while (0 ); num =min_t (int, num, avail / slotsize);Quoted 5 lines
Quoted 2 lines
After implementing it, I'm able to compile the kernel.
But, as I mentioned before, I'm not a coder, so I cannot test the patchfrom a programming perspective.
Quoted 2 lines
Therefore, it would be nice if a patch could be made available by a
human.
Quoted 28 lines
:)
Quoted 6 lines
Thanks in advance - regards,
MichaelView: https://bugzilla.kernel.org/show_bug.cgi?id=220745#c0
You can reply to this message to join the discussion.The failing code was introduced by commit
2030ca560c5f("nfsd: degraded slot-count more gracefully as allocation nears exhaustion.") in v5.4. It is not a backport to v6.1. I don't see any changes to that code untilb5fba969a2e4("nfsd: remove artificial limits on the session-based DRC"), when it was removed whole-sale.That means we don't have this code to patch in upstream. If there is a fix to be made in NFSD, we will have to create one-off patches for each of the LTS kernels.
It appears that there have been several clean-ups to
clamp_tand friends since v6.1, and they have been mostly backported to v6.1.y. It's hard to say without a bisect whether one of those is the reason for the breakage.Neil, according to the last touch rule, you're the owner of this code ;-) Do you have any thoughts? I'm not quite sure what the compile-time assertion is complaining about.
Quoted 8 lines
Quoted 4 lines
Signature · 1 line
Chuck Lever(via
https://msgid.link/[email protected].
name)
View: https://bugzilla.kernel.org/show_bug.cgi?id=220745#c3
You can reply to this message to join the discussion.
Signature · 2 lines
Deet-doot-dot, I am a bot. Kernel.org Bugzilla (bugspray 0.1-dev)
Mike-SPC writes via Kernel.org Bugzilla:
Quoted 1 line
Have you found a 6.1.y kernel for which the build doesn't fail?
Yes. Compiling Version 6.1.155 works without problems. Versions >= 6.1.156 aren't.
Regards,
Michael
View: https://bugzilla.kernel.org/show_bug.cgi?id=220745#c4
You can reply to this message to join the discussion.
Signature · 2 lines
Deet-doot-dot, I am a bot. Kernel.org Bugzilla (bugspray 0.1-dev)
Chuck Lever
maintainerQuoted 6 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
Quoted 1 line
Have you found a 6.1.y kernel for which the build doesn't fail?
Yes. Compiling Version 6.1.155 works without problems. Versions >= 6.1.156 aren't.
My analysis yesterday suggests that, because the nfs4state.c code hasn't changed, it's probably something elsewhere that introduced this problem. As we can't reproduce the issue, can you use "git bisect" between v6.1.155 and v6.1.156 to find the culprit commit?
Signature · 1 line
Chuck Lever
Mike-SPC writes via Kernel.org Bugzilla:
(In reply to Bugspray Bot from comment #5)
Quoted 10 lines
Chuck Lever <[email protected]> replies to comment #4:
Quoted 6 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
Quoted 1 line
Have you found a 6.1.y kernel for which the build doesn't fail?
Yes. Compiling Version 6.1.155 works without problems. Versions >= 6.1.156 aren't.
Quoted 6 lines
My analysis yesterday suggests that, because the
nfs4state.ccode hasn't changed, it's probably something elsewhere that introduced this problem. As we can't reproduce the issue, can you use "git bisect" between v6.1.155 and v6.1.156 to find the culprit commit?
Yes, your analysis is right (thanks for it).
After some investigation, the issue appears to be caused by changes introduced in include/linux/minmax.h.
I verified this by replacing minmax.h in 6.1.156 with the version from 6.1.155, and the kernel then compiles successfully.
The relevant section in the 6.1.156 changelog (https://cdn.kernel.org/pub/linux/kernel/v6.x/ChangeLog-6.1.156) shows several modifications to minmax.h (notably around __clamp_once() and the use of BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...)), which seem to trigger a compile-time assertion when building NFSD.
Replacing the updated header with the previous one resolves the issue, so this appears to be a regression introduced by the new clamp() logic.
Could you please advise who is the right person or mailing list to report this issue to (minmax.h maintainers, kernel core, or stable tree)?
Regards,
Michael
View: https://bugzilla.kernel.org/show_bug.cgi?id=220745#c6
You can reply to this message to join the discussion.
Signature · 2 lines
Deet-doot-dot, I am a bot. Kernel.org Bugzilla (bugspray 0.1-dev)
Jeff Layton
maintainerQuoted 28 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
(In reply to Bugspray Bot from comment #5)
Quoted 10 lines
Chuck Lever <[email protected]> replies to comment #4:
Quoted 6 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
Quoted 1 line
Have you found a 6.1.y kernel for which the build doesn't fail?
Yes. Compiling Version 6.1.155 works without problems. Versions >= 6.1.156 aren't.
Quoted 6 lines
My analysis yesterday suggests that, because the
nfs4state.ccode hasn't changed, it's probably something elsewhere that introduced this problem. As we can't reproduce the issue, can you use "git bisect" between v6.1.155 and v6.1.156 to find the culprit commit?Yes, your analysis is right (thanks for it).
After some investigation, the issue appears to be caused by changes introduced ininclude/linux/minmax.h.I verified this by replacing
minmax.hin 6.1.156 with the version from 6.1.155, and the kernel then compiles successfully.
Quoted 9 lines
The relevant section in the 6.1.156 changelog (https://cdn.kernel.org/pub/linux/kernel/v6.x/ChangeLog-6.1.156) shows several modifications to
minmax.h(notably around__clamp_once()and the use ofBUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...)), which seem to trigger a compile-time assertion when building NFSD.Replacing the updated header with the previous one resolves the issue, so this appears to be a regression introduced by the new
clamp()logic.Could you please advise who is the right person or mailing list to report this issue to (
minmax.hmaintainers, kernel core, or stable tree)?
I'd let all 3 know, and I'd include the author of the patches that you suspect are the problem. They'll probably want to revise the one that's a problem.
Cheers,
Signature · 1 line
Jeff Layton <[email protected]>
Chuck Lever
maintainerFYI
https://bugzilla.kernel.org/show_bug.cgi?id=220745
-------- Forwarded Message --------
Subject: Re: Compile Error fs/nfsd/nfs4state.o - clamp() low limit slotsize greater than high limit total_avail/scale_factor
Date: Thu, 06 Nov 2025 07:29:25 -0500
From: Jeff Layton <[email protected]>
To: Mike-SPC via Bugspray Bot <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Quoted 28 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
(In reply to Bugspray Bot from comment #5)
Quoted 10 lines
Chuck Lever <[email protected]> replies to comment #4:
Quoted 6 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
Quoted 1 line
Have you found a 6.1.y kernel for which the build doesn't fail?
Yes. Compiling Version 6.1.155 works without problems. Versions >= 6.1.156 aren't.
Quoted 6 lines
My analysis yesterday suggests that, because the
nfs4state.ccode hasn't changed, it's probably something elsewhere that introduced this problem. As we can't reproduce the issue, can you use "git bisect" between v6.1.155 and v6.1.156 to find the culprit commit?Yes, your analysis is right (thanks for it).
After some investigation, the issue appears to be caused by changes introduced ininclude/linux/minmax.h.I verified this by replacing
minmax.hin 6.1.156 with the version from 6.1.155, and the kernel then compiles successfully.
Quoted 9 lines
The relevant section in the 6.1.156 changelog (https://cdn.kernel.org/pub/linux/kernel/v6.x/ChangeLog-6.1.156) shows several modifications to
minmax.h(notably around__clamp_once()and the use ofBUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...)), which seem to trigger a compile-time assertion when building NFSD.Replacing the updated header with the previous one resolves the issue, so this appears to be a regression introduced by the new
clamp()logic.Could you please advise who is the right person or mailing list to report this issue to (
minmax.hmaintainers, kernel core, or stable tree)?
I'd let all 3 know, and I'd include the author of the patches that you suspect are the problem. They'll probably want to revise the one that's a problem.
Cheers,
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 3 lines · On Thu, 6 Nov 2025 09:33:28 -0500 Chuck Lever <[email protected]> wrote:
FYI
Ugg - that code is horrid.
It seems to have got deleted since, but it is:
u32 slotsize = slot_bytes(ca); u32 num = ca->maxreqs; unsigned long avail, total_avail; unsigned int scale_factor; spin_lock(&nfsd_drc_lock); if (nfsd_drc_max_mem > nfsd_drc_mem_used) total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; else /* We have handed out more space than we chose in * set_max_drc() to allow. That isn't really a * problem as long as that doesn't make us think we * have lots more due to integer overflow. */ total_avail = 0; avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail); /* * Never use more than a fraction of the remaining memory, * unless it's the only way to give this client a slot. * The chosen fraction is either 1/8 or 1/number of threads, * whichever is smaller. This ensures there are adequate * slots to support multiple clients per thread. * Give the client one slot even if that would require * over-allocation--it is better than failure. */ scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads); avail = clamp_t(unsigned long, avail, slotsize, total_avail/scale_factor); num = min_t(int, num, avail / slotsize); num = max_t(int, num, 1);
Lets rework it a bit...
if (nfsd_drc_max_mem > nfsd_drc_mem_used) {
total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used;
avail = min(NFSD_MAX_MEM_PER_SESSION, total_avail);
avail = clamp(avail, n + sizeof(xxx), total_avail/8)
} else {
total_avail = 0;
avail = 0;
avail = clamp(0, n + sizeof(xxx), 0);
}Neither of those clamp() are sane at all - should be clamp(val, lo, hi) with 'lo <= hi' otherwise the result is dependant on the order of the comparisons.
The compiler sees the second one and rightly bleats.
I can't even guess what the code is actually trying to calculate!
Maybe looking at where the code came from, or the current version might help.
It MIGHT be that the 'lo' of slotsize was an attempt to ensure that the following 'avail / slotsize' was as least one.
Some software archaeology might show that the 'num = max(num, 1)' was added because the code above didn't work.
In that case the clamp can be clamp(avail, 0, total_avail/scale_factor) which is just min(avail, total_avail/scale_factor).
The person who rewrote it between 6.1 and 6.18 might now more.
David
Quoted 55 lines
-------- Forwarded Message --------
Subject: Re: Compile Errorfs/nfsd/nfs4state.o-clamp()low limit slotsize greater than high limit total_avail/scale_factor
Date: Thu, 06 Nov 2025 07:29:25 -0500
From: Jeff Layton <[email protected]>
To: Mike-SPC via Bugspray Bot <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]Quoted 28 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
(In reply to Bugspray Bot from comment #5)
Quoted 10 lines
Chuck Lever <[email protected]> replies to comment #4:
Quoted 6 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
Quoted 1 line
Have you found a 6.1.y kernel for which the build doesn't fail?
Yes. Compiling Version 6.1.155 works without problems. Versions >= 6.1.156 aren't.
Quoted 6 lines
My analysis yesterday suggests that, because the
nfs4state.ccode hasn't changed, it's probably something elsewhere that introduced this problem. As we can't reproduce the issue, can you use "git bisect" between v6.1.155 and v6.1.156 to find the culprit commit?Yes, your analysis is right (thanks for it).
After some investigation, the issue appears to be caused by changes introduced ininclude/linux/minmax.h.I verified this by replacing
minmax.hin 6.1.156 with the version from 6.1.155, and the kernel then compiles successfully.Quoted 9 lines
The relevant section in the 6.1.156 changelog (https://cdn.kernel.org/pub/linux/kernel/v6.x/ChangeLog-6.1.156) shows several modifications to
minmax.h(notably around__clamp_once()and the use ofBUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...)), which seem to trigger a compile-time assertion when building NFSD.Replacing the updated header with the previous one resolves the issue, so this appears to be a regression introduced by the new
clamp()logic.Could you please advise who is the right person or mailing list to report this issue to (
minmax.hmaintainers, kernel core, or stable tree)?I'd let all 3 know, and I'd include the author of the patches that you suspect are the problem. They'll probably want to revise the one that's a problem.
Cheers,
Chuck Lever
maintainerQuoted 53 lines · David Laight
Quoted 3 lines · On Thu, 6 Nov 2025 09:33:28 -0500 Chuck Lever <[email protected]> wrote:
FYI
Ugg - that code is horrid.
It seems to have got deleted since, but it is:u32 slotsize = slot_bytes(ca); u32 num = ca->maxreqs; unsigned long avail, total_avail; unsigned int scale_factor; spin_lock(&nfsd_drc_lock); if (nfsd_drc_max_mem > nfsd_drc_mem_used) total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; else /* We have handed out more space than we chose in * set_max_drc() to allow. That isn't really a * problem as long as that doesn't make us think we * have lots more due to integer overflow. */ total_avail = 0; avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail); /* * Never use more than a fraction of the remaining memory, * unless it's the only way to give this client a slot. * The chosen fraction is either 1/8 or 1/number of threads, * whichever is smaller. This ensures there are adequate * slots to support multiple clients per thread. * Give the client one slot even if that would require * over-allocation--it is better than failure. */ scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads); avail = clamp_t(unsigned long, avail, slotsize, total_avail/scale_factor); num = min_t(int, num, avail / slotsize); num = max_t(int, num, 1);Lets rework it a bit...
if (nfsd_drc_max_mem > nfsd_drc_mem_used) { total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; avail = min(NFSD_MAX_MEM_PER_SESSION, total_avail); avail = clamp(avail, n + sizeof(xxx), total_avail/8) } else { total_avail = 0; avail = 0; avail = clamp(0, n + sizeof(xxx), 0); }
Quoted 7 lines
Neither of those
clamp()are sane at all - should beclamp(val, lo, hi)with 'lo <= hi' otherwise the result is dependant on the order of the comparisons.
The compiler sees the second one and rightly bleats.
I can't even guess what the code is actually trying to calculate!Maybe looking at where the code came from, or the current version might help.
The current upstream code is part of a new feature that is not appropriate to backport to LTS kernels. I consider that code out of play.
The compiler error showed up in 6.1.y with the recent minmax.h changes -- there have been no reported problems in any of the LTS kernels until now, including with 32-bit builds.
The usual guidelines about regressions suggest that the most recent backports (ie, minmax.h) are the ones that should be removed or reworked to address the compile breakage. I don't think we should address this by writing special clean-ups to code that wasn't broken before the minmax.h changes. Cleaning that code up is more likely to introduce bugs than reverting the minmax.h changes.
Quoted 67 lines
It MIGHT be that the 'lo' of slotsize was an attempt to ensure that the following 'avail / slotsize' was as least one.
Some software archaeology might show that the 'num =max(num, 1)' was added because the code above didn't work.
In that case the clamp can beclamp(avail, 0, total_avail/scale_factor)which is justmin(avail, total_avail/scale_factor).The person who rewrote it between 6.1 and 6.18 might now more.
David
Quoted 55 lines
-------- Forwarded Message --------
Subject: Re: Compile Errorfs/nfsd/nfs4state.o-clamp()low limit slotsize greater than high limit total_avail/scale_factor
Date: Thu, 06 Nov 2025 07:29:25 -0500
From: Jeff Layton <[email protected]>
To: Mike-SPC via Bugspray Bot <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]Quoted 28 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
(In reply to Bugspray Bot from comment #5)
Quoted 10 lines
Chuck Lever <[email protected]> replies to comment #4:
Quoted 6 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
Quoted 1 line
Have you found a 6.1.y kernel for which the build doesn't fail?
Yes. Compiling Version 6.1.155 works without problems. Versions >= 6.1.156 aren't.
Quoted 6 lines
My analysis yesterday suggests that, because the
nfs4state.ccode hasn't changed, it's probably something elsewhere that introduced this problem. As we can't reproduce the issue, can you use "git bisect" between v6.1.155 and v6.1.156 to find the culprit commit?Yes, your analysis is right (thanks for it).
After some investigation, the issue appears to be caused by changes introduced ininclude/linux/minmax.h.I verified this by replacing
minmax.hin 6.1.156 with the version from 6.1.155, and the kernel then compiles successfully.Quoted 9 lines
The relevant section in the 6.1.156 changelog (https://cdn.kernel.org/pub/linux/kernel/v6.x/ChangeLog-6.1.156) shows several modifications to
minmax.h(notably around__clamp_once()and the use ofBUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...)), which seem to trigger a compile-time assertion when building NFSD.Replacing the updated header with the previous one resolves the issue, so this appears to be a regression introduced by the new
clamp()logic.Could you please advise who is the right person or mailing list to report this issue to (
minmax.hmaintainers, kernel core, or stable tree)?I'd let all 3 know, and I'd include the author of the patches that you suspect are the problem. They'll probably want to revise the one that's a problem.
Cheers,
Signature · 1 line
Chuck Lever
Quoted 70 lines · On Thu, 6 Nov 2025 14:32:34 -0500 Chuck Lever <[email protected]> wrote:
Quoted 53 lines · David Laight
Quoted 3 lines · On Thu, 6 Nov 2025 09:33:28 -0500 Chuck Lever <[email protected]> wrote:
FYI
Ugg - that code is horrid.
It seems to have got deleted since, but it is:u32 slotsize = slot_bytes(ca); u32 num = ca->maxreqs; unsigned long avail, total_avail; unsigned int scale_factor; spin_lock(&nfsd_drc_lock); if (nfsd_drc_max_mem > nfsd_drc_mem_used) total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; else /* We have handed out more space than we chose in * set_max_drc() to allow. That isn't really a * problem as long as that doesn't make us think we * have lots more due to integer overflow. */ total_avail = 0; avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail); /* * Never use more than a fraction of the remaining memory, * unless it's the only way to give this client a slot. * The chosen fraction is either 1/8 or 1/number of threads, * whichever is smaller. This ensures there are adequate * slots to support multiple clients per thread. * Give the client one slot even if that would require * over-allocation--it is better than failure. */ scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads); avail = clamp_t(unsigned long, avail, slotsize, total_avail/scale_factor); num = min_t(int, num, avail / slotsize); num = max_t(int, num, 1);Lets rework it a bit...
if (nfsd_drc_max_mem > nfsd_drc_mem_used) { total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; avail = min(NFSD_MAX_MEM_PER_SESSION, total_avail); avail = clamp(avail, n + sizeof(xxx), total_avail/8) } else { total_avail = 0; avail = 0; avail = clamp(0, n + sizeof(xxx), 0); }Quoted 7 lines
Neither of those
clamp()are sane at all - should beclamp(val, lo, hi)with 'lo <= hi' otherwise the result is dependant on the order of the comparisons.
The compiler sees the second one and rightly bleats.
I can't even guess what the code is actually trying to calculate!Maybe looking at where the code came from, or the current version might help.
The current upstream code is part of a new feature that is not appropriate to backport to LTS kernels. I consider that code out of play.
The compiler error showed up in 6.1.y with the recent
minmax.hchanges -- there have been no reported problems in any of the LTS kernels until now, including with 32-bit builds.
Quoted 6 lines
The usual guidelines about regressions suggest that the most recent backports (ie,
minmax.h) are the ones that should be removed or reworked to address the compile breakage. I don't think we should address this by writing special clean-ups to code that wasn't broken before theminmax.hchanges. Cleaning that code up is more likely to introduce bugs than reverting theminmax.hchanges.
No, that code needs fixing. It is broken.....
The compiler warning/error is completely valid.
The result of that clamp() has never been well defined.
It is likely that it always generated the wrong result.
It might be that a much older version of the function exists before someone changed a pair of conditionals to be a call to clamp(). That old version may well be fine.
David
Quoted 71 lines
Quoted 69 lines
It MIGHT be that the 'lo' of slotsize was an attempt to ensure that the following 'avail / slotsize' was as least one.
Some software archaeology might show that the 'num =max(num, 1)' was added because the code above didn't work.
In that case the clamp can beclamp(avail, 0, total_avail/scale_factor)which is justmin(avail, total_avail/scale_factor).The person who rewrote it between 6.1 and 6.18 might now more.
David
Quoted 55 lines
-------- Forwarded Message --------
Subject: Re: Compile Errorfs/nfsd/nfs4state.o-clamp()low limit slotsize greater than high limit total_avail/scale_factor
Date: Thu, 06 Nov 2025 07:29:25 -0500
From: Jeff Layton <[email protected]>
To: Mike-SPC via Bugspray Bot <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]Quoted 28 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
(In reply to Bugspray Bot from comment #5)
Quoted 10 lines
Chuck Lever <[email protected]> replies to comment #4:
Quoted 6 lines · Mike-SPC via Bugspray Bot
Mike-SPC writes via Kernel.org Bugzilla:
Quoted 1 line
Have you found a 6.1.y kernel for which the build doesn't fail?
Yes. Compiling Version 6.1.155 works without problems. Versions >= 6.1.156 aren't.
Quoted 6 lines
My analysis yesterday suggests that, because the
nfs4state.ccode hasn't changed, it's probably something elsewhere that introduced this problem. As we can't reproduce the issue, can you use "git bisect" between v6.1.155 and v6.1.156 to find the culprit commit?Yes, your analysis is right (thanks for it).
After some investigation, the issue appears to be caused by changes introduced ininclude/linux/minmax.h.I verified this by replacing
minmax.hin 6.1.156 with the version from 6.1.155, and the kernel then compiles successfully.Quoted 9 lines
The relevant section in the 6.1.156 changelog (https://cdn.kernel.org/pub/linux/kernel/v6.x/ChangeLog-6.1.156) shows several modifications to
minmax.h(notably around__clamp_once()and the use ofBUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...)), which seem to trigger a compile-time assertion when building NFSD.Replacing the updated header with the previous one resolves the issue, so this appears to be a regression introduced by the new
clamp()logic.Could you please advise who is the right person or mailing list to report this issue to (
minmax.hmaintainers, kernel core, or stable tree)?I'd let all 3 know, and I'd include the author of the patches that you suspect are the problem. They'll probably want to revise the one that's a problem.
Cheers,
Quoted 53 lines · David Laight
Quoted 3 lines · On Thu, 6 Nov 2025 09:33:28 -0500 Chuck Lever <[email protected]> wrote:
FYI
Ugg - that code is horrid.
It seems to have got deleted since, but it is:u32 slotsize = slot_bytes(ca); u32 num = ca->maxreqs; unsigned long avail, total_avail; unsigned int scale_factor; spin_lock(&nfsd_drc_lock); if (nfsd_drc_max_mem > nfsd_drc_mem_used) total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; else /* We have handed out more space than we chose in * set_max_drc() to allow. That isn't really a * problem as long as that doesn't make us think we * have lots more due to integer overflow. */ total_avail = 0; avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail); /* * Never use more than a fraction of the remaining memory, * unless it's the only way to give this client a slot. * The chosen fraction is either 1/8 or 1/number of threads, * whichever is smaller. This ensures there are adequate * slots to support multiple clients per thread. * Give the client one slot even if that would require * over-allocation--it is better than failure. */ scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads); avail = clamp_t(unsigned long, avail, slotsize, total_avail/scale_factor); num = min_t(int, num, avail / slotsize); num = max_t(int, num, 1);Lets rework it a bit...
if (nfsd_drc_max_mem > nfsd_drc_mem_used) { total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; avail = min(NFSD_MAX_MEM_PER_SESSION, total_avail); avail = clamp(avail, n + sizeof(xxx), total_avail/8) } else { total_avail = 0; avail = 0; avail = clamp(0, n + sizeof(xxx), 0); }
Quoted 4 lines
Neither of those
clamp()are sane at all - should beclamp(val, lo, hi)with 'lo <= hi' otherwise the result is dependant on the order of the comparisons.
The compiler sees the second one and rightly bleats.
In fact only gcc-9 bleats.
gcc-7 gcc-10 gcc-13 gcc-15
all seem to think it is fine.
NeilBrown
Quoted 54 lines · On Fri, 07 Nov 2025 22:17:20 +1100 NeilBrown <[email protected]> wrote:
Quoted 53 lines · David Laight
Quoted 3 lines · On Thu, 6 Nov 2025 09:33:28 -0500 Chuck Lever <[email protected]> wrote:
FYI
Ugg - that code is horrid.
It seems to have got deleted since, but it is:u32 slotsize = slot_bytes(ca); u32 num = ca->maxreqs; unsigned long avail, total_avail; unsigned int scale_factor; spin_lock(&nfsd_drc_lock); if (nfsd_drc_max_mem > nfsd_drc_mem_used) total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; else /* We have handed out more space than we chose in * set_max_drc() to allow. That isn't really a * problem as long as that doesn't make us think we * have lots more due to integer overflow. */ total_avail = 0; avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail); /* * Never use more than a fraction of the remaining memory, * unless it's the only way to give this client a slot. * The chosen fraction is either 1/8 or 1/number of threads, * whichever is smaller. This ensures there are adequate * slots to support multiple clients per thread. * Give the client one slot even if that would require * over-allocation--it is better than failure. */ scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads); avail = clamp_t(unsigned long, avail, slotsize, total_avail/scale_factor); num = min_t(int, num, avail / slotsize); num = max_t(int, num, 1);Lets rework it a bit...
if (nfsd_drc_max_mem > nfsd_drc_mem_used) { total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; avail = min(NFSD_MAX_MEM_PER_SESSION, total_avail); avail = clamp(avail, n + sizeof(xxx), total_avail/8) } else { total_avail = 0; avail = 0; avail = clamp(0, n + sizeof(xxx), 0); }
Quoted 6 lines
Quoted 4 lines
Neither of those
clamp()are sane at all - should beclamp(val, lo, hi)with 'lo <= hi' otherwise the result is dependant on the order of the comparisons.
The compiler sees the second one and rightly bleats.In fact only gcc-9 bleats.
That is probably why it didn't get picked up earlier.
Quoted 2 lines
gcc-7 gcc-10 gcc-13 gcc-15
all seem to think it is fine.
Which, of course, it isn't...
David
Quoted 2 lines
NeilBrown
Quoted 62 lines · David Laight
Quoted 59 lines · On Fri, 07 Nov 2025 22:17:20 +1100 NeilBrown <[email protected]> wrote:
Quoted 57 lines · David Laight
Quoted 3 lines · On Thu, 6 Nov 2025 09:33:28 -0500 Chuck Lever <[email protected]> wrote:
FYI
Ugg - that code is horrid.
It seems to have got deleted since, but it is:u32 slotsize = slot_bytes(ca); u32 num = ca->maxreqs; unsigned long avail, total_avail; unsigned int scale_factor; spin_lock(&nfsd_drc_lock); if (nfsd_drc_max_mem > nfsd_drc_mem_used) total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; else /* We have handed out more space than we chose in * set_max_drc() to allow. That isn't really a * problem as long as that doesn't make us think we * have lots more due to integer overflow. */ total_avail = 0; avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail); /* * Never use more than a fraction of the remaining memory, * unless it's the only way to give this client a slot. * The chosen fraction is either 1/8 or 1/number of threads, * whichever is smaller. This ensures there are adequate * slots to support multiple clients per thread. * Give the client one slot even if that would require * over-allocation--it is better than failure. */ scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads); avail = clamp_t(unsigned long, avail, slotsize, total_avail/scale_factor); num = min_t(int, num, avail / slotsize); num = max_t(int, num, 1);Lets rework it a bit...
if (nfsd_drc_max_mem > nfsd_drc_mem_used) { total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; avail = min(NFSD_MAX_MEM_PER_SESSION, total_avail); avail = clamp(avail, n + sizeof(xxx), total_avail/8) } else { total_avail = 0; avail = 0; avail = clamp(0, n + sizeof(xxx), 0); }Neither of those
clamp()are sane at all - should beclamp(val, lo, hi)with 'lo <= hi' otherwise the result is dependant on the order of the comparisons.
The compiler sees the second one and rightly bleats.
Quoted 8 lines
Quoted 1 line
In fact only gcc-9 bleats.
That is probably why it didn't get picked up earlier.
Quoted 2 lines
gcc-7 gcc-10 gcc-13 gcc-15
all seem to think it is fine.Which, of course, it isn't...
I've now had a proper look at your analysis of the code - thanks.
I agree that the code is unclear (at best) and that if it were still upstream I would want to fix it. However is does function correctly.
As you say, when min > max, the result of clamp(val, min, max) depends on the order of comparison, and we know what the order of comparison is because we can look at the code for clamp().
Currently it is
((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
which will use max when max is below val and min.
Previously it was
min((typeof(val))max(val, lo), hi)
which also will use max when it is below val and min
Before that it was
#define clamp_t(type, val, min, max) ({ \
type __val = (val); \
type __min = (min); \
type __max = (max); \
__val = __val < __min ? __min: __val; \
__val > __max ? __max: __val; })which also uses max when that is less that val and min.
So I think the nfsd code has always worked correctly. That is not sufficient for mainline - there we want it to also be robust and maintainable. But for stable kernels it should be sufficient. Adding a patch to "stable" kernels which causes working code to fail to compile does not seem, to me, to be in the spirit of "stability". (Have the "clamp" checking in mainline, finding problems there, and backporting the fixes to stable seems to me to be the best way to use these checking improvements to improve "stable").
Thanks,
NeilBrown
Chuck Lever
maintainerQuoted 100 lines · NeilBrown
Quoted 62 lines · David Laight
Quoted 59 lines · On Fri, 07 Nov 2025 22:17:20 +1100 NeilBrown <[email protected]> wrote:
Quoted 57 lines · David Laight
Quoted 3 lines · On Thu, 6 Nov 2025 09:33:28 -0500 Chuck Lever <[email protected]> wrote:
FYI
Ugg - that code is horrid.
It seems to have got deleted since, but it is:u32 slotsize = slot_bytes(ca); u32 num = ca->maxreqs; unsigned long avail, total_avail; unsigned int scale_factor; spin_lock(&nfsd_drc_lock); if (nfsd_drc_max_mem > nfsd_drc_mem_used) total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; else /* We have handed out more space than we chose in * set_max_drc() to allow. That isn't really a * problem as long as that doesn't make us think we * have lots more due to integer overflow. */ total_avail = 0; avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail); /* * Never use more than a fraction of the remaining memory, * unless it's the only way to give this client a slot. * The chosen fraction is either 1/8 or 1/number of threads, * whichever is smaller. This ensures there are adequate * slots to support multiple clients per thread. * Give the client one slot even if that would require * over-allocation--it is better than failure. */ scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads); avail = clamp_t(unsigned long, avail, slotsize, total_avail/scale_factor); num = min_t(int, num, avail / slotsize); num = max_t(int, num, 1);Lets rework it a bit...
if (nfsd_drc_max_mem > nfsd_drc_mem_used) { total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; avail = min(NFSD_MAX_MEM_PER_SESSION, total_avail); avail = clamp(avail, n + sizeof(xxx), total_avail/8) } else { total_avail = 0; avail = 0; avail = clamp(0, n + sizeof(xxx), 0); }Neither of those
clamp()are sane at all - should beclamp(val, lo, hi)with 'lo <= hi' otherwise the result is dependant on the order of the comparisons.
The compiler sees the second one and rightly bleats.Quoted 8 lines
Quoted 1 line
In fact only gcc-9 bleats.
That is probably why it didn't get picked up earlier.
Quoted 2 lines
gcc-7 gcc-10 gcc-13 gcc-15
all seem to think it is fine.Which, of course, it isn't...
I've now had a proper look at your analysis of the code - thanks.
I agree that the code is unclear (at best) and that if it were still upstream I would want to fix it. However is does function correctly.
As you say, when min > max, the result of
clamp(val, min, max)depends on the order of comparison, and we know what the order of comparison is because we can look at the code forclamp().Currently it is
((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))which will use max when max is below val and min.
Previously it wasmin((typeof(val))max(val, lo), hi)which also will use max when it is below val and min
Before that it was
#define clamp_t(type, val, min, max) ({ \ type __val = (val); \ type __min = (min); \ type __max = (max); \ __val = __val < __min ? __min: __val; \ __val > __max ? __max: __val; })which also uses max when that is less that val and min.
Quoted 8 lines
So I think the nfsd code has always worked correctly. That is not sufficient for mainline - there we want it to also be robust and maintainable. But for stable kernels it should be sufficient. Adding a patch to "stable" kernels which causes working code to fail to compile does not seem, to me, to be in the spirit of "stability". (Have the "clamp" checking in mainline, finding problems there, and backporting the fixes to stable seems to me to be the best way to use these checking improvements to improve "stable").
I agree with Neil. The LTS code was building and working rather universally until recently. The less risky approach is to leave this code unchanged and seek another remedy for the OP.
Signature · 1 line
Chuck Lever
Quoted 110 lines · On Sat, 8 Nov 2025 10:49:34 -0500 Chuck Lever <[email protected]> wrote:
Quoted 100 lines · NeilBrown
Quoted 62 lines · David Laight
Quoted 59 lines · On Fri, 07 Nov 2025 22:17:20 +1100 NeilBrown <[email protected]> wrote:
Quoted 57 lines · David Laight
Quoted 3 lines · On Thu, 6 Nov 2025 09:33:28 -0500 Chuck Lever <[email protected]> wrote:
FYI
Ugg - that code is horrid.
It seems to have got deleted since, but it is:u32 slotsize = slot_bytes(ca); u32 num = ca->maxreqs; unsigned long avail, total_avail; unsigned int scale_factor; spin_lock(&nfsd_drc_lock); if (nfsd_drc_max_mem > nfsd_drc_mem_used) total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; else /* We have handed out more space than we chose in * set_max_drc() to allow. That isn't really a * problem as long as that doesn't make us think we * have lots more due to integer overflow. */ total_avail = 0; avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail); /* * Never use more than a fraction of the remaining memory, * unless it's the only way to give this client a slot. * The chosen fraction is either 1/8 or 1/number of threads, * whichever is smaller. This ensures there are adequate * slots to support multiple clients per thread. * Give the client one slot even if that would require * over-allocation--it is better than failure. */ scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads); avail = clamp_t(unsigned long, avail, slotsize, total_avail/scale_factor); num = min_t(int, num, avail / slotsize); num = max_t(int, num, 1);Lets rework it a bit...
if (nfsd_drc_max_mem > nfsd_drc_mem_used) { total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used; avail = min(NFSD_MAX_MEM_PER_SESSION, total_avail); avail = clamp(avail, n + sizeof(xxx), total_avail/8) } else { total_avail = 0; avail = 0; avail = clamp(0, n + sizeof(xxx), 0); }Neither of those
clamp()are sane at all - should beclamp(val, lo, hi)with 'lo <= hi' otherwise the result is dependant on the order of the comparisons.
The compiler sees the second one and rightly bleats.Quoted 8 lines
Quoted 1 line
In fact only gcc-9 bleats.
That is probably why it didn't get picked up earlier.
Quoted 2 lines
gcc-7 gcc-10 gcc-13 gcc-15
all seem to think it is fine.Which, of course, it isn't...
I've now had a proper look at your analysis of the code - thanks.
I agree that the code is unclear (at best) and that if it were still upstream I would want to fix it. However is does function correctly.
As you say, when min > max, the result of
clamp(val, min, max)depends on the order of comparison, and we know what the order of comparison is because we can look at the code forclamp().Currently it is
((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))which will use max when max is below val and min.
Previously it wasmin((typeof(val))max(val, lo), hi)which also will use max when it is below val and min
Before that it was
#define clamp_t(type, val, min, max) ({ \ type __val = (val); \ type __min = (min); \ type __max = (max); \ __val = __val < __min ? __min: __val; \ __val > __max ? __max: __val; })which also uses max when that is less that val and min.
Quoted 8 lines
So I think the nfsd code has always worked correctly. That is not sufficient for mainline - there we want it to also be robust and maintainable. But for stable kernels it should be sufficient. Adding a patch to "stable" kernels which causes working code to fail to compile does not seem, to me, to be in the spirit of "stability". (Have the "clamp" checking in mainline, finding problems there, and backporting the fixes to stable seems to me to be the best way to use these checking improvements to improve "stable").
Quoted 3 lines
I agree with Neil. The LTS code was building and working rather universally until recently. The less risky approach is to leave this code unchanged and seek another remedy for the OP.
IIRC minmax.h was backported to allow other changes be backported. So something has to give.
Changing the:
avail = clamp_t(unsigned long, avail, slotsize, total_avail/scale_factor); to avail = min(avail, total_avail/scale_factor); fixes everything and leaves this code behaving the same way. The later: num = max_t(int, num, 1);
has the effect that the lower bound of the clamp_t() was expected to have.
I've just looked through the old versions.
The comment when the clamp_t() was added is:
* Never use more than a third of the remaining memory,
* unless it's the only way to give this client a slot.
The intention was clearly that 'avail' would be at least 'slotsize', even though clamp() has never worked that way (and relying on the order of the comparisons is a bug in itself).
This got 'fixed' by the extra check that ensures 'num' is at least one, but the code clearly wasn't read properly at that time.
(It has also suffered because clamp_t() was used and truncated significant bits of the values.)
I've no sympathy for this buggy code that has been bodge fixed several times.
David
Mike-SPC writes via Kernel.org Bugzilla:
Hi there,
at first: Thanks for patching and investigation.
I've tested the patch mentioned in
https://lkml.org/lkml/2025/11/10/12
and it works (for me).
Thanks again and regards,
Michael
View: https://bugzilla.kernel.org/show_bug.cgi?id=220745#c17
You can reply to this message to join the discussion.
Signature · 2 lines
Deet-doot-dot, I am a bot. Kernel.org Bugzilla (bugspray 0.1-dev)