| Message ID | 20241017070445.4013745-3-gnaaman@drivenets.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Improve neigh_flush_dev performance | expand |
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 *))
> Note about subjects: all your patches should have an appropriate subject > prefix. It looks like it could just be "net: neighbour:" for every patch. As in: git --subject-prefix="PATCH net-next: neighbour v5" Or independently, after the prefix, as in: [PATCH net-next vX A/B] neighbour: REST OF 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.) Noted, will revert this change and rename the added macro. Thank you for your time.
Gilad Naaman <gnaaman@drivenets.com> writes: >> Note about subjects: all your patches should have an appropriate subject >> prefix. It looks like it could just be "net: neighbour:" for every patch. > > Or independently, after the prefix, as in: > > [PATCH net-next vX A/B] neighbour: REST OF PATCH This one.
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) +{ + 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(); +} + 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 *))
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(-)