Re: [PATCH net-next v5 2/6] Define neigh_for_each

[Petr Machata <petrm@nvidia.com>]

Note about subjects: all your patches should have an appropriate subject
prefix. It looks like it could just be "net: neighbour:" for every patch.

Also giving this patch a subject "define neigh_for_each" is odd, that
function already is defined. Below I argue that reusing the name
neigh_for_each for the new helper is inappropriate. If you accept that,
you can add the helper in a separate patch and convert the open-coded
sites right away, which would be in 2/6. Then 3/6 would be the patch
that moves neigh_for_each to mlxsw and renames. (Though below I also
argue that perhaps it would be better to keep it where it is now.)

Gilad Naaman <gnaaman@drivenets.com> writes:

> Define neigh_for_each in neighbour.h and move old definition
> to its only point of usage within the mlxsw driver.
>
> Signed-off-by: Gilad Naaman <gnaaman@drivenets.com>
> ---
>  .../ethernet/mellanox/mlxsw/spectrum_router.c | 24 +++++++++++++++++--
>  include/net/neighbour.h                       |  4 ++--
>  net/core/neighbour.c                          | 22 -----------------
>  3 files changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> index 800dfb64ec83..de62587c5a63 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> @@ -3006,6 +3006,26 @@ static void mlxsw_sp_neigh_rif_made_sync_each(struct neighbour *n, void *data)
>  		rms->err = -ENOMEM;
>  }
>  
> +static void mlxsw_sp_neigh_for_each(struct neigh_table *tbl,
> +				    void *cookie)

This is still named as if it were a generic walker, but actually it's
hardcoding the mlxsw_sp_neigh_rif_made_sync_each callback. The name
should reflect that.

E.g. mlxsw_sp_neigh_rif_made_sync_neighs.

Then it would also be good to rename mlxsw_sp_neigh_rif_made_sync_each
to mlxsw_sp_neigh_rif_made_sync_neigh as well.

> +{
> +	struct neigh_hash_table *nht;
> +	int chain;
> +
> +	rcu_read_lock();
> +	nht = rcu_dereference(tbl->nht);
> +
> +	read_lock_bh(&tbl->lock); /* avoid resizes */
> +	for (chain = 0; chain < (1 << nht->hash_shift); chain++) {
> +		struct neighbour *n;
> +
> +		neigh_for_each(n, &nht->hash_heads[chain])
> +			mlxsw_sp_neigh_rif_made_sync_each(n, cookie);
> +	}
> +	read_unlock_bh(&tbl->lock);
> +	rcu_read_unlock();
> +}
> +

All this stuff looks like it's involved in private details of the
neighbor table implementation that IMHO a client of that module
shouldn't (have to) touch.

I'm not really sure why the function cannot stay where it is, under the
existing name, and the new function is not added under a different name.

Reusing neigh_for_each() seems inappropriate anyway, the name says "for
each neighbor", but in fact you are supposed to wrap it in this loop
over individual heads. Wouldn't it make sense to keep the existing
neigh_for_each(), and add a new helper, neigh_chain_for_each() or
something like that? And OK, call this new helper from neigh_for_each()?

Then this patch doesn't even need to exist.

>  static int mlxsw_sp_neigh_rif_made_sync(struct mlxsw_sp *mlxsw_sp,
>  					struct mlxsw_sp_rif *rif)
>  {
> @@ -3014,12 +3034,12 @@ static int mlxsw_sp_neigh_rif_made_sync(struct mlxsw_sp *mlxsw_sp,
>  		.rif = rif,
>  	};
>  
> -	neigh_for_each(&arp_tbl, mlxsw_sp_neigh_rif_made_sync_each, &rms);
> +	mlxsw_sp_neigh_for_each(&arp_tbl, &rms);
>  	if (rms.err)
>  		goto err_arp;
>  
>  #if IS_ENABLED(CONFIG_IPV6)
> -	neigh_for_each(&nd_tbl, mlxsw_sp_neigh_rif_made_sync_each, &rms);
> +	mlxsw_sp_neigh_for_each(&nd_tbl, &rms);
>  #endif
>  	if (rms.err)
>  		goto err_nd;
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 0402447854c7..37303656ab65 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -277,6 +277,8 @@ static inline void *neighbour_priv(const struct neighbour *n)
>  
>  extern const struct nla_policy nda_policy[];
>  
> +#define neigh_for_each(pos, head) hlist_for_each_entry(pos, head, hash)
> +
>  static inline bool neigh_key_eq32(const struct neighbour *n, const void *pkey)
>  {
>  	return *(const u32 *)n->primary_key == *(const u32 *)pkey;
> @@ -390,8 +392,6 @@ static inline struct net *pneigh_net(const struct pneigh_entry *pneigh)
>  }
>  
>  void neigh_app_ns(struct neighbour *n);
> -void neigh_for_each(struct neigh_table *tbl,
> -		    void (*cb)(struct neighbour *, void *), void *cookie);
>  void __neigh_for_each_release(struct neigh_table *tbl,
>  			      int (*cb)(struct neighbour *));
>  int neigh_xmit(int fam, struct net_device *, const void *, struct sk_buff *);
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 45c8df801dfb..d9c458e6f627 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -3120,28 +3120,6 @@ static int neigh_get(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>  	return err;
>  }
>  
> -void neigh_for_each(struct neigh_table *tbl, void (*cb)(struct neighbour *, void *), void *cookie)
> -{
> -	int chain;
> -	struct neigh_hash_table *nht;
> -
> -	rcu_read_lock();
> -	nht = rcu_dereference(tbl->nht);
> -
> -	read_lock_bh(&tbl->lock); /* avoid resizes */
> -	for (chain = 0; chain < (1 << nht->hash_shift); chain++) {
> -		struct neighbour *n;
> -
> -		for (n = rcu_dereference(nht->hash_buckets[chain]);
> -		     n != NULL;
> -		     n = rcu_dereference(n->next))
> -			cb(n, cookie);
> -	}
> -	read_unlock_bh(&tbl->lock);
> -	rcu_read_unlock();
> -}
> -EXPORT_SYMBOL(neigh_for_each);
> -
>  /* The tbl->lock must be held as a writer and BH disabled. */
>  void __neigh_for_each_release(struct neigh_table *tbl,
>  			      int (*cb)(struct neighbour *))