Re: [PATCH nf-next 2/5] ipvs: add resizable hash tables

	Hello,

On Mon, 2 Mar 2026, Florian Westphal wrote:

> Julian Anastasov <[email protected]> wrote:
> > > Julian Anastasov <[email protected]> wrote:
> > > > +/**
> > > > + * ip_vs_rht_for_bucket_retry() - Retry bucket if entries are moved
> > > > + * @t:		current table, used as cursor, struct ip_vs_rht *var
> > > > + * @bucket:	index of current bucket or hash key
> > > > + * @sc:		temp seqcount_t *var
> > > > + * @retry:	temp int var
> > > > + */
> > > > +#define ip_vs_rht_for_bucket_retry(t, bucket, sc, seq, retry)		\
> > > 
> > > This triggers a small kdoc warning:
> > > 
> > > Warning: include/net/ip_vs.h:554 function parameter 'seq' not described in 'ip_vs_rht_for_bucket_retry'
> > 
> > 	Will fix it, thanks! Just let me know if more comments
> > are expected before sending next version...
> 
> There is some checkpatch noise in patch 1:
> 
> CHECK: Alignment should match open parenthesis
> #42: FILE: include/linux/rculist_bl.h:24:
> +       rcu_assign_pointer(hlist_bl_first_rcu(h),
>                 (struct hlist_bl_node *)((unsigned long)n | LIST_BL_LOCKMASK));

	I don't change here any alignment and I didn't fixed it
because I'm not sure how to make it better :)

> CHECK: Macro argument 'member' may be better as '(member)' to avoid precedence issues
> #97: FILE: include/linux/rculist_bl.h:126:
> +#define hlist_bl_for_each_entry_continue_rcu(tpos, pos, member)                \
> +       for (pos = rcu_dereference_raw(hlist_bl_next_rcu(&(tpos)->member)); \
> +            pos &&                                                     \
> +            ({ tpos = hlist_bl_entry(pos, typeof(*tpos), member); 1; }); \
> +            pos = rcu_dereference_raw(hlist_bl_next_rcu(pos)))

	I think, this matches how all such macros use the member
field.

> And patch 2:
> ERROR: Macros with complex values should be enclosed in parentheses
> #147: FILE: include/net/ip_vs.h:583:
> +#define ip_vs_rht_walk_buckets_rcu(table, head)                                \
> +       ip_vs_rht_for_each_table_rcu(table, _t, _p)                     \
> +               ip_vs_rht_for_each_bucket(_t, _bucket, head)            \
> +                       ip_vs_rht_for_bucket_retry(_t, _bucket, _sc,    \
> +                                                  _seq, _retry)

	Yep, here we use complex macros to help the callers
use a loop with their statement/body. In the previous versions
I checked the checkpatch output and due to the advanced macro
usage I'm not sure what can be made better...

> 
> BUT SEE:
> 
>    do {} while (0) advice is over-stated in a few situations:
> 
>    The more obvious case is macros, like MODULE_PARM_DESC, invoked at
> 
> [ there is more ]
> 
> ... but I don't really care, you can handle/ignore it as you see fit.
> 
> I only have a question wrt. the hash table choice.
> 
> Why are you not re-using rhashtables and instead roll your own?
> 
> No requirement, but might make sense to mention the rationale
> in the commit message.

	I found the rhashtable_remove_fast operation slow by using 
hashing+lookup. Also, IPVS needs to rehash (move) single entry when
its key changes (cport in ip_vs_conn_fill_cport) and I don't see
public method in rht for this. Things get complicated when we add double 
conn hashing, it needs careful move operation from one/two chains. Also, 
in part 4 of the changes we allow customizations for the load factor,
in case the defaults are not suitable for the setup.

	May be I can add more info in the commit message about this.

Regards

--
Julian Anastasov <[email protected]>