* [net-next] neigh: remove duplicate check for same neigh
@ 2016-11-29 8:22 Zhang Shengju
2016-11-29 16:22 ` David Ahern
0 siblings, 1 reply; 4+ messages in thread
From: Zhang Shengju @ 2016-11-29 8:22 UTC (permalink / raw)
To: netdev, dsa
Currently loop index 'idx' is used as the index in the neigh list of interest.
It's increased only when the neigh is dumped. It's not the absolute index in
the list. Because there is no info to record which neigh has already be scanned
by previous loop. This will cause the filtered out neighs to be scanned mulitple
times.
This patch make idx as the absolute index in the list, it will increase no matter
whether the neigh is filtered. This will prevent the above problem.
And this is in line with other dump functions.
Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com>
---
net/core/neighbour.c | 39 ++++++++++++++++++---------------------
1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 2ae929f..ce32e9c 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2256,6 +2256,16 @@ static bool neigh_ifindex_filtered(struct net_device *dev, int filter_idx)
return false;
}
+static bool neigh_dump_filtered(struct net_device *dev, int filter_idx,
+ int filter_master_idx)
+{
+ if (neigh_ifindex_filtered(dev, filter_idx) ||
+ neigh_master_filtered(dev, filter_master_idx))
+ return true;
+
+ return false;
+}
+
static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
struct netlink_callback *cb)
{
@@ -2285,20 +2295,15 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
rcu_read_lock_bh();
nht = rcu_dereference_bh(tbl->nht);
- for (h = s_h; h < (1 << nht->hash_shift); h++) {
- if (h > s_h)
- s_idx = 0;
+ for (h = s_h; h < (1 << nht->hash_shift); h++, s_idx = 0) {
for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx = 0;
n != NULL;
- n = rcu_dereference_bh(n->next)) {
- if (!net_eq(dev_net(n->dev), net))
- continue;
- if (neigh_ifindex_filtered(n->dev, filter_idx))
+ n = rcu_dereference_bh(n->next), idx++) {
+ if (idx < s_idx || !net_eq(dev_net(n->dev), net))
continue;
- if (neigh_master_filtered(n->dev, filter_master_idx))
+ if (neigh_dump_filtered(n->dev, filter_idx,
+ filter_master_idx))
continue;
- if (idx < s_idx)
- goto next;
if (neigh_fill_info(skb, n, NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq,
RTM_NEWNEIGH,
@@ -2306,8 +2311,6 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
rc = -1;
goto out;
}
-next:
- idx++;
}
}
rc = skb->len;
@@ -2328,14 +2331,10 @@ static int pneigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
read_lock_bh(&tbl->lock);
- for (h = s_h; h <= PNEIGH_HASHMASK; h++) {
- if (h > s_h)
- s_idx = 0;
- for (n = tbl->phash_buckets[h], idx = 0; n; n = n->next) {
- if (pneigh_net(n) != net)
+ for (h = s_h; h <= PNEIGH_HASHMASK; h++, s_idx = 0) {
+ for (n = tbl->phash_buckets[h], idx = 0; n; n = n->next, idx++) {
+ if (idx < s_idx || pneigh_net(n) != net)
continue;
- if (idx < s_idx)
- goto next;
if (pneigh_fill_info(skb, n, NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq,
RTM_NEWNEIGH,
@@ -2344,8 +2343,6 @@ static int pneigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
rc = -1;
goto out;
}
- next:
- idx++;
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [net-next] neigh: remove duplicate check for same neigh
2016-11-29 8:22 [net-next] neigh: remove duplicate check for same neigh Zhang Shengju
@ 2016-11-29 16:22 ` David Ahern
2016-11-30 1:27 ` 张胜举
0 siblings, 1 reply; 4+ messages in thread
From: David Ahern @ 2016-11-29 16:22 UTC (permalink / raw)
To: Zhang Shengju, netdev
On 11/29/16 1:22 AM, Zhang Shengju wrote:
> @@ -2285,20 +2295,15 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
> rcu_read_lock_bh();
> nht = rcu_dereference_bh(tbl->nht);
>
> - for (h = s_h; h < (1 << nht->hash_shift); h++) {
> - if (h > s_h)
> - s_idx = 0;
> + for (h = s_h; h < (1 << nht->hash_shift); h++, s_idx = 0) {
> for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx = 0;
> n != NULL;
> - n = rcu_dereference_bh(n->next)) {
> - if (!net_eq(dev_net(n->dev), net))
> - continue;
> - if (neigh_ifindex_filtered(n->dev, filter_idx))
> + n = rcu_dereference_bh(n->next), idx++) {
incrementing idx here ...
> + if (idx < s_idx || !net_eq(dev_net(n->dev), net))
> continue;
> - if (neigh_master_filtered(n->dev, filter_master_idx))
> + if (neigh_dump_filtered(n->dev, filter_idx,
> + filter_master_idx))
> continue;
> - if (idx < s_idx)
> - goto next;
> if (neigh_fill_info(skb, n, NETLINK_CB(cb->skb).portid,
> cb->nlh->nlmsg_seq,
> RTM_NEWNEIGH,
... causes a missed entry when an error happens here
idx is only incremented when a neigh has been successfully added to the skb.
Your intention is to save a few cycles by making idx an absolute index within the hash bucket and jumping to the next entry to be dumped without evaluating any filters per entry.
So why not keep it simple and do this:
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 2ae929f9bd06..2e49a85e696a 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2291,14 +2291,12 @@ static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx = 0;
n != NULL;
n = rcu_dereference_bh(n->next)) {
- if (!net_eq(dev_net(n->dev), net))
- continue;
- if (neigh_ifindex_filtered(n->dev, filter_idx))
- continue;
- if (neigh_master_filtered(n->dev, filter_master_idx))
- continue;
if (idx < s_idx)
goto next;
+ if (!net_eq(dev_net(n->dev), net) ||
+ neigh_ifindex_filtered(n->dev, filter_idx) ||
+ neigh_master_filtered(n->dev, filter_master_idx))
+ goto next;
if (neigh_fill_info(skb, n, NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq,
RTM_NEWNEIGH,
^ permalink raw reply related [flat|nested] 4+ messages in thread* RE: [net-next] neigh: remove duplicate check for same neigh
2016-11-29 16:22 ` David Ahern
@ 2016-11-30 1:27 ` 张胜举
2016-11-30 1:56 ` David Ahern
0 siblings, 1 reply; 4+ messages in thread
From: 张胜举 @ 2016-11-30 1:27 UTC (permalink / raw)
To: 'David Ahern', netdev
> -----Original Message-----
> From: David Ahern [mailto:dsa@cumulusnetworks.com]
> Sent: Wednesday, November 30, 2016 12:23 AM
> To: Zhang Shengju <zhangshengju@cmss.chinamobile.com>;
> netdev@vger.kernel.org
> Subject: Re: [net-next] neigh: remove duplicate check for same neigh
>
> On 11/29/16 1:22 AM, Zhang Shengju wrote:
> > @@ -2285,20 +2295,15 @@ static int neigh_dump_table(struct neigh_table
> *tbl, struct sk_buff *skb,
> > rcu_read_lock_bh();
> > nht = rcu_dereference_bh(tbl->nht);
> >
> > - for (h = s_h; h < (1 << nht->hash_shift); h++) {
> > - if (h > s_h)
> > - s_idx = 0;
> > + for (h = s_h; h < (1 << nht->hash_shift); h++, s_idx = 0) {
> > for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx = 0;
> > n != NULL;
> > - n = rcu_dereference_bh(n->next)) {
> > - if (!net_eq(dev_net(n->dev), net))
> > - continue;
> > - if (neigh_ifindex_filtered(n->dev, filter_idx))
> > + n = rcu_dereference_bh(n->next), idx++) {
>
> incrementing idx here ...
>
> > + if (idx < s_idx || !net_eq(dev_net(n->dev), net))
> > continue;
> > - if (neigh_master_filtered(n->dev,
filter_master_idx))
> > + if (neigh_dump_filtered(n->dev, filter_idx,
> > + filter_master_idx))
> > continue;
> > - if (idx < s_idx)
> > - goto next;
> > if (neigh_fill_info(skb, n, NETLINK_CB(cb-
> >skb).portid,
> > cb->nlh->nlmsg_seq,
> > RTM_NEWNEIGH,
>
> ... causes a missed entry when an error happens here
>
> idx is only incremented when a neigh has been successfully added to the
skb.
If an error happens, we just goto 'out' and exit this loop, this will skip
the 'idx++'.
So no missed entry, right?
>
>
> Your intention is to save a few cycles by making idx an absolute index
within
> the hash bucket and jumping to the next entry to be dumped without
> evaluating any filters per entry.
>
> So why not keep it simple and do this:
This has less changes, but I preferred to add a new function to do the
filter logic, this is the same as your code @rtnl_dump_ifinfo().
And for the 'idx', it should be increased when neigh entry is filter out or
is successfully added to the skb. Isn't it straight-forward to put it in for
loop?
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c index
> 2ae929f9bd06..2e49a85e696a 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2291,14 +2291,12 @@ static int neigh_dump_table(struct neigh_table
> *tbl, struct sk_buff *skb,
> for (n = rcu_dereference_bh(nht->hash_buckets[h]), idx =
0;
> n != NULL;
> n = rcu_dereference_bh(n->next)) {
> - if (!net_eq(dev_net(n->dev), net))
> - continue;
> - if (neigh_ifindex_filtered(n->dev, filter_idx))
> - continue;
> - if (neigh_master_filtered(n->dev,
filter_master_idx))
> - continue;
> if (idx < s_idx)
> goto next;
> + if (!net_eq(dev_net(n->dev), net) ||
> + neigh_ifindex_filtered(n->dev, filter_idx) ||
> + neigh_master_filtered(n->dev,
filter_master_idx))
> + goto next;
> if (neigh_fill_info(skb, n,
NETLINK_CB(cb->skb).portid,
> cb->nlh->nlmsg_seq,
> RTM_NEWNEIGH,
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [net-next] neigh: remove duplicate check for same neigh
2016-11-30 1:27 ` 张胜举
@ 2016-11-30 1:56 ` David Ahern
0 siblings, 0 replies; 4+ messages in thread
From: David Ahern @ 2016-11-30 1:56 UTC (permalink / raw)
To: 张胜举, netdev
On 11/29/16 6:27 PM, 张胜举 wrote:
> This has less changes, but I preferred to add a new function to do the
> filter logic, this is the same as your code @rtnl_dump_ifinfo().
I'd prefer it to stay in line as it is now. You can save the cycles of filtering on already viewed entries without an overhaul of this code.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-30 1:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-29 8:22 [net-next] neigh: remove duplicate check for same neigh Zhang Shengju
2016-11-29 16:22 ` David Ahern
2016-11-30 1:27 ` 张胜举
2016-11-30 1:56 ` David Ahern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox