Hello, On Fri, 6 Dec 2024, David Laight wrote: > From: Julian Anastasov > > Sent: 06 December 2024 16:23 > ... > > 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 :) > > > 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... > > 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 <ja@xxxxxx>
