Hello, On Fri, 6 Dec 2024, David Laight wrote: > The intention of the code seems to be that the minimum table > size should be 256 (1 << min). > However the code uses max = clamp(20, 5, max_avail) which implies Actually, it tries to reduce max=20 (max possible) below max_avail: [8 .. max_avail]. Not sure what 5 is here... > the author thought max_avail could be less than 5. > But clamp(val, min, max) is only well defined for max >= min. > If max < min whether is returns min or max depends on the order of > the comparisons. Looks like max_avail goes below 8 ? What value you see for such small system? > Change to clamp(max_avail, 5, 20) which has the expected behaviour. It should be clamp(max_avail, 8, 20) > > Replace the clamp_val() on the line below with clamp(). > clamp_val() is just 'an accident waiting to happen' and not needed here. OK > Fixes: 4f325e26277b6 > (Although I actually doubt the code is used on small memory systems.) > > Detected by compile time checks added to clamp(), specifically: > minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp() Existing or new check? Does it happen that max_avail is a constant, so that a compile check triggers? > > Signed-off-by: David Laight <david.laight@xxxxxxxxxx> The code below looks ok to me but can you change the comments above to more correctly specify the values and if the problem is that max_avail goes below 8 (min). > --- > net/netfilter/ipvs/ip_vs_conn.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c > index 98d7dbe3d787..c0289f83f96d 100644 > --- a/net/netfilter/ipvs/ip_vs_conn.c > +++ b/net/netfilter/ipvs/ip_vs_conn.c > @@ -1495,8 +1495,8 @@ int __init ip_vs_conn_init(void) > max_avail -= 2; /* ~4 in hash row */ > max_avail -= 1; /* IPVS up to 1/2 of mem */ > max_avail -= order_base_2(sizeof(struct ip_vs_conn)); More likely we can additionally clamp max_avail here: max_avail = max(min, max_avail); But your solution solves the problem with less lines. > - max = clamp(max, min, max_avail); > - ip_vs_conn_tab_bits = clamp_val(ip_vs_conn_tab_bits, min, max); > + max = clamp(max_avail, min, max); > + ip_vs_conn_tab_bits = clamp(ip_vs_conn_tab_bits, min, max); > ip_vs_conn_tab_size = 1 << ip_vs_conn_tab_bits; > ip_vs_conn_tab_mask = ip_vs_conn_tab_size - 1; > > -- > 2.17.1 Regards -- Julian Anastasov <ja@xxxxxx>
