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

[Florian Westphal <fw@strlen.de>]

Julian Anastasov <ja@ssi.bg> wrote:
> > Julian Anastasov <ja@ssi.bg> 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));

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


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)

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.

Thanks.