| Message ID | 20241015165929.3203216-4-gnaaman@drivenets.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Improve neigh_flush_dev performance | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/apply | fail | Patch does not apply to net-next-1 |
From: Gilad Naaman <gnaaman@drivenets.com> Date: Tue, 15 Oct 2024 16:59:23 +0000 > Convert seq_file-related neighbour functionality to use neighbour::hash > and the related for_each macro. > > Signed-off-by: Gilad Naaman <gnaaman@drivenets.com> > --- > include/net/neighbour.h | 4 ++++ > net/core/neighbour.c | 26 ++++++++++---------------- > 2 files changed, 14 insertions(+), 16 deletions(-) > > diff --git a/include/net/neighbour.h b/include/net/neighbour.h > index 2f4cb9e51364..7dc0d4d6a4a8 100644 > --- a/include/net/neighbour.h > +++ b/include/net/neighbour.h > @@ -279,6 +279,10 @@ 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) > +#define neigh_hlist_entry(n) hlist_entry_safe(n, struct neighbour, hash) > +#define neigh_first_rcu(bucket) \ > + neigh_hlist_entry(rcu_dereference(hlist_first_rcu(bucket))) No RCU helpers are needed, seq file is under RCU & tbl->lock #define neigh_first_entry(bucket) \ hlist_entry_safe((bucket)->first, struct neighbour, hash) > + nit: unnecessary newline. > > static inline bool neigh_key_eq32(const struct neighbour *n, const void *pkey) > { > diff --git a/net/core/neighbour.c b/net/core/neighbour.c > index e91105a4c5ee..4bdf7649ca57 100644 > --- a/net/core/neighbour.c > +++ b/net/core/neighbour.c > @@ -3207,25 +3207,21 @@ static struct neighbour *neigh_get_first(struct seq_file *seq) > > state->flags &= ~NEIGH_SEQ_IS_PNEIGH; > for (bucket = 0; bucket < (1 << nht->hash_shift); bucket++) { > - n = rcu_dereference(nht->hash_buckets[bucket]); > - > - while (n) { > + neigh_for_each(n, &nht->hash_heads[bucket]) { > if (!net_eq(dev_net(n->dev), net)) > - goto next; > + continue; > if (state->neigh_sub_iter) { > loff_t fakep = 0; > void *v; > > v = state->neigh_sub_iter(state, n, &fakep); > if (!v) > - goto next; > + continue; > } > if (!(state->flags & NEIGH_SEQ_SKIP_NOARP)) > break; Let's avoid double-break and use goto just before setting bucket like: out: state->bucket = bucket > if (READ_ONCE(n->nud_state) & ~NUD_NOARP) > break; Same here. > -next: > - n = rcu_dereference(n->next); > } > > if (n) Then, this null check & break will be unnecessary. > @@ -3249,34 +3245,32 @@ static struct neighbour *neigh_get_next(struct seq_file *seq, > if (v) > return n; > } > - n = rcu_dereference(n->next); > > while (1) { > - while (n) { > + hlist_for_each_entry_continue_rcu(n, hash) { > if (!net_eq(dev_net(n->dev), net)) > - goto next; > + continue; > if (state->neigh_sub_iter) { > void *v = state->neigh_sub_iter(state, n, pos); > if (v) > return n; > - goto next; > + continue; > } > if (!(state->flags & NEIGH_SEQ_SKIP_NOARP)) > break; > > if (READ_ONCE(n->nud_state) & ~NUD_NOARP) > break; Same remark here. > -next: > - n = rcu_dereference(n->next); > } > > if (n) > break; and here. > > - if (++state->bucket >= (1 << nht->hash_shift)) > - break; Let's keep this, > + while (!n && ++state->bucket < (1 << nht->hash_shift)) > + n = neigh_first_rcu(&nht->hash_heads[state->bucket]); > > - n = rcu_dereference(nht->hash_buckets[state->bucket]); and simply fetch neigh_first_entry(). Then, we can let the next loop handle the termination condition and keep the code simple. > + if (!n) > + break; > } > > if (n && pos) > -- > 2.46.0
> > - if (++state->bucket >= (1 << nht->hash_shift)) > > - break; > > Let's keep this, > > > > + while (!n && ++state->bucket < (1 << nht->hash_shift)) > > + n = neigh_first_rcu(&nht->hash_heads[state->bucket]); > > > > - n = rcu_dereference(nht->hash_buckets[state->bucket]); > > and simply fetch neigh_first_entry(). > > Then, we can let the next loop handle the termination condition > and keep the code simple. Unfortunately `hlist_for_each_entry_continue_rcu` dereferences `n` first thing using `hlist_next_rcu`, before checking for NULL: #define hlist_for_each_entry_continue_rcu(pos, member) \ for (pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu( \ &(pos)->member)), typeof(*(pos)), member); \ pos; \ pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu( \ &(pos)->member)), typeof(*(pos)), member)) If I'm using it, I have to add a null-check after calling `neigh_first_entry`. Another alternative is to use `hlist_for_each_entry_from_rcu`, but it'll require calling `next` one time before the loop. Is this preferable?
From: Gilad Naaman <gnaaman@drivenets.com> Date: Wed, 16 Oct 2024 09:11:52 +0000 > > > - if (++state->bucket >= (1 << nht->hash_shift)) > > > - break; > > > > Let's keep this, > > > > > > > + while (!n && ++state->bucket < (1 << nht->hash_shift)) > > > + n = neigh_first_rcu(&nht->hash_heads[state->bucket]); > > > > > > - n = rcu_dereference(nht->hash_buckets[state->bucket]); > > > > and simply fetch neigh_first_entry(). > > > > Then, we can let the next loop handle the termination condition > > and keep the code simple. > > Unfortunately `hlist_for_each_entry_continue_rcu` dereferences `n` > first thing using `hlist_next_rcu`, before checking for NULL: Right, and I noticed we even can't use neigh_first_entry() after checking against NULL because the first entry will be skipped by hlist_for_each_entry_continue_rcu(). > > #define hlist_for_each_entry_continue_rcu(pos, member) \ > for (pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu( \ > &(pos)->member)), typeof(*(pos)), member); \ > pos; \ > pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu( \ > &(pos)->member)), typeof(*(pos)), member)) > > If I'm using it, I have to add a null-check after calling `neigh_first_entry`. > > Another alternative is to use `hlist_for_each_entry_from_rcu`, > but it'll require calling `next` one time before the loop. > > Is this preferable? How about factorising the operations inside loops and use hlist_for_each_entry_continue() and call neigh_get_first() in neigh_get_next() ? completely not tested: ---8<--- static struct neighbour *neigh_get_valid(struct seq_file *seq, struct neighbour *n, loff_t *pos) { struct net *net = seq_file_net(seq); if (!net_eq(dev_net(n->dev), net)) return NULL; if (state->neigh_sub_iter) { loff_t fakep = 0; void *v; v = state->neigh_sub_iter(state, n, pos ? pos : &fakep); if (!v) return NULL; if (pos) return v; } if (!(state->flags & NEIGH_SEQ_SKIP_NOARP)) return n; if (READ_ONCE(n->nud_state) & ~NUD_NOARP) return n; return NULL; } static struct neighbour *neigh_get_first(struct seq_file *seq) { struct neigh_seq_state *state = seq->private; struct neigh_hash_table *nht = state->nht; struct net *net = seq_file_net(seq); struct neighbour *n, *tmp; state->flags &= ~NEIGH_SEQ_IS_PNEIGH; while (++state->bucket < (1 << nht->hash_shift)) { neigh_for_each(n, &nht->hash_heads[state->bucket]) { tmp = neigh_get_valid(seq, n, pos); if (tmp) return tmp; } } return NULL; } static struct neighbour *neigh_get_next(struct seq_file *seq, struct neighbour *n, loff_t *pos) { struct neigh_seq_state *state = seq->private; struct neigh_hash_table *nht = state->nht; struct net *net = seq_file_net(seq); struct neighbour *tmp; if (state->neigh_sub_iter) { void *v = state->neigh_sub_iter(state, n, pos); if (v) return n; } hlist_for_each_entry_continue(n, hash) { tmp = neigh_get_valid(seq, n, pos); if (tmp) { n = tmp; goto out; } } n = neigh_get_first(seq); out: if (n && pos) --(*pos); return n; } ---8<---
> How about factorising the operations inside loops and use > hlist_for_each_entry_continue() and call neigh_get_first() > in neigh_get_next() ? Yes, this works. (Just had to initialize buckets to `-1` since `neigh_get_first` always advances it, and otherwise the first bucket would have been skipped)
diff --git a/include/net/neighbour.h b/include/net/neighbour.h index 2f4cb9e51364..7dc0d4d6a4a8 100644 --- a/include/net/neighbour.h +++ b/include/net/neighbour.h @@ -279,6 +279,10 @@ 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) +#define neigh_hlist_entry(n) hlist_entry_safe(n, struct neighbour, hash) +#define neigh_first_rcu(bucket) \ + neigh_hlist_entry(rcu_dereference(hlist_first_rcu(bucket))) + static inline bool neigh_key_eq32(const struct neighbour *n, const void *pkey) { diff --git a/net/core/neighbour.c b/net/core/neighbour.c index e91105a4c5ee..4bdf7649ca57 100644 --- a/net/core/neighbour.c +++ b/net/core/neighbour.c @@ -3207,25 +3207,21 @@ static struct neighbour *neigh_get_first(struct seq_file *seq) state->flags &= ~NEIGH_SEQ_IS_PNEIGH; for (bucket = 0; bucket < (1 << nht->hash_shift); bucket++) { - n = rcu_dereference(nht->hash_buckets[bucket]); - - while (n) { + neigh_for_each(n, &nht->hash_heads[bucket]) { if (!net_eq(dev_net(n->dev), net)) - goto next; + continue; if (state->neigh_sub_iter) { loff_t fakep = 0; void *v; v = state->neigh_sub_iter(state, n, &fakep); if (!v) - goto next; + continue; } if (!(state->flags & NEIGH_SEQ_SKIP_NOARP)) break; if (READ_ONCE(n->nud_state) & ~NUD_NOARP) break; -next: - n = rcu_dereference(n->next); } if (n) @@ -3249,34 +3245,32 @@ static struct neighbour *neigh_get_next(struct seq_file *seq, if (v) return n; } - n = rcu_dereference(n->next); while (1) { - while (n) { + hlist_for_each_entry_continue_rcu(n, hash) { if (!net_eq(dev_net(n->dev), net)) - goto next; + continue; if (state->neigh_sub_iter) { void *v = state->neigh_sub_iter(state, n, pos); if (v) return n; - goto next; + continue; } if (!(state->flags & NEIGH_SEQ_SKIP_NOARP)) break; if (READ_ONCE(n->nud_state) & ~NUD_NOARP) break; -next: - n = rcu_dereference(n->next); } if (n) break; - if (++state->bucket >= (1 << nht->hash_shift)) - break; + while (!n && ++state->bucket < (1 << nht->hash_shift)) + n = neigh_first_rcu(&nht->hash_heads[state->bucket]); - n = rcu_dereference(nht->hash_buckets[state->bucket]); + if (!n) + break; } if (n && pos)
Convert seq_file-related neighbour functionality to use neighbour::hash and the related for_each macro. Signed-off-by: Gilad Naaman <gnaaman@drivenets.com> --- include/net/neighbour.h | 4 ++++ net/core/neighbour.c | 26 ++++++++++---------------- 2 files changed, 14 insertions(+), 16 deletions(-)