|
Hello,
On Mon, 2 Mar 2026, Florian Westphal wrote:
> Julian Anastasov <ja@xxxxxx> wrote:
> > > Julian Anastasov <ja@xxxxxx> 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 <ja@xxxxxx>
|
