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]>
