Re: [PATCH net-next v2 1/2] Convert neighbour-table to use hlist

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Gilad Naaman <gnaaman@drivenets.com>
Date: Tue,  8 Oct 2024 07:38:55 +0000
> > > @@ -388,21 +366,20 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev,
> > >  
> > >  	for (i = 0; i < (1 << nht->hash_shift); i++) {
> > >  		struct neighbour *n;
> > > -		struct neighbour __rcu **np = &nht->hash_buckets[i];
> > > +		struct neighbour __rcu **np =
> > > +			(struct neighbour __rcu **)&nht->hash_buckets[i].first;
> > 
> > This will be no longer needed for doubly linked list,
> 
> This is not as-necessary with a doubly-linked list, but unfortunately
> I cannot eliminate it completely, as the `n` might be released in the loop
> body.
> 
> I can convert this function to use a `struct neighour *next` instead,
> if it is more palatable.

Yes, using hlist_for_each_entry_safe() is more preferable.

Mixing for() and while() is harder to read.


[...]
> > > @@ -693,11 +666,10 @@ ___neigh_create(struct neigh_table *tbl, const void *pkey,
> > >  		goto out_tbl_unlock;
> > >  	}
> > >  
> > > -	for (n1 = rcu_dereference_protected(nht->hash_buckets[hash_val],
> > > -					    lockdep_is_held(&tbl->lock));
> > > -	     n1 != NULL;
> > > -	     n1 = rcu_dereference_protected(n1->next,
> > > -			lockdep_is_held(&tbl->lock))) {
> > > +	hlist_for_each_entry_rcu(n1,
> > > +				 &nht->hash_buckets[hash_val],
> > > +				 list,
> > > +				 lockdep_is_held(&tbl->lock)) {
> > 
> > Let's define hlist_for_each_entry_rcu() as neigh-specific macro.
> 
> Can you elaborate on this?
> Do you want the `list` parameter to be eliminated?

I mean like

#define neigh_for_each(...)		\
	hlist_for_each_entry(...)

#define neigh_for_each_rcu(...)		\
	hlist_for_each_entry_rcu(...)

are better if there's repeated arguments.