Hello,

On Fri, 6 Dec 2024, David Laight wrote:

From: Julian Anastasov

quoted

Sent: 06 December 2024 16:23

...

quoted

	I'm not sure how much memory we can see in small system,
IMHO, problem should not be possible in practice:

- nobody expects 0 from totalram_pages() in the code

- order_base_2(sizeof(struct ip_vs_conn)) is probably 8 on 32-bit

It is 0x120 bytes on 64bit, so 8 could well be right.

	That is already for 9 :)

quoted

quoted

Is all stems from order_base_2(totalram_pages()).
order_base_2(n) is 'n > 1 ? ilog2(n - 1) + 1 : 0'.
And the compiler generates two copies of the code that follows
for the 'constant zero' and ilog2() values.
And the 'zero' case compiles clamp(20, 8, 0) which is errored.
Note that it is only executed if totalram_pages() is zero,
but it is always compiled 'just in case'.

	I'm confused with these compiler issues,

The compiler is just doing its job.
Consider this expression:
	(x >= 1 ? 2 * x : 1) - 1
It is likely to get converted to:
	(x >= 1 ? 2 * x - 1 : 0)
to avoid the subtract when x < 1.

The same thing is happening here.
order_base_2() has a (condition ? fn() : 0) in it.
All the +/- constants get moved inside, on 64bit that is +12 -2 -1 -9 = 0.
Then the clamp() with constants gets moved inside:
	(condition ? clamp(27, 8, fn() + 0) : clamp(27, 8, 0 + 0))
Now, at runtime, we know that 'condition' is true and (fn() >= 8)
so the first clamp() is valid and the second one never used.
But this isn't known by the compiler and clamp() detects the invalid
call and generates a warning.

	I see, such optimizations are beyond my expectations,
I used max_avail var to separate the operations between
different macro calls but in the end they are mixed together...

quoted

if you
think we should go with the patch just decide if it is a
net or net-next material. Your change is safer for bad
max_avail values but I don't expect to see problem while
running without the change, except the building bugs.

	Also, please use nf/nf-next tag to avoid any
confusion with upstreaming...

I've copied Andrew M - he's taken the minmax.h change into his mm tree.
This is one of the build breakages.

It probably only needs to go into next for now (via some route).
But I can image the minmax.h changes getting backported a bit.

	OK, then can you send v2 with Fixes header, precised comments
and nf tag, it fixes a recent commit, so it can be backported easily...

Regards

--
Julian Anastasov [off-list ref]