| Message ID | 1375981079-2936-6-git-send-email-vfalico@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 08/08/2013 06:57 PM, Veaceslav Falico wrote: > RFC -> v1: use the new __vlan_find_dev_next(), also release rcu_read_lock() > only after we stop using the vlan_dev. > v1 -> v2: no change. > > Instead of looping through bond->vlan_list, loop through > bond->dev->vlan_info via __vlan_find_dev_next() under rcu_read_lock(). > > CC: Jay Vosburgh <fubar@us.ibm.com> > CC: Andy Gospodarek <andy@greyhouse.net> > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > --- > drivers/net/bonding/bond_main.c | 29 +++++++++++++---------------- > 1 files changed, 13 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index e52e2d5..f536d05 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -2440,11 +2440,10 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op, __be32 dest_ > > static void bond_arp_send_all(struct bonding *bond, struct slave *slave) > { > - int i, vlan_id; > - __be32 *targets = bond->params.arp_targets; > - struct vlan_entry *vlan; > - struct net_device *vlan_dev = NULL; > + struct net_device *vlan_dev; > struct rtable *rt; > + __be32 *targets = bond->params.arp_targets; > + int i; > Style nitpick: maybe move them longest -> shortest. > for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) { > __be32 addr; > @@ -2486,28 +2485,26 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave) > continue; > } > > - vlan_id = 0; > - list_for_each_entry(vlan, &bond->vlan_list, vlan_list) { > - rcu_read_lock(); > - vlan_dev = __vlan_find_dev_deep(bond->dev, > - htons(ETH_P_8021Q), > - vlan->vlan_id); > - rcu_read_unlock(); > + vlan_dev = NULL; > + > + rcu_read_lock(); > + while ((vlan_dev = __vlan_find_dev_next(bond->dev, vlan_dev))) > if (vlan_dev == rt->dst.dev) { > - vlan_id = vlan->vlan_id; > pr_debug("basa: vlan match on %s %d\n", > - vlan_dev->name, vlan_id); > + vlan_dev->name, > + vlan_dev_vlan_id(vlan_dev)); > break; > } > - } > > - if (vlan_id && vlan_dev) { > + if (vlan_dev) { > ip_rt_put(rt); > addr = bond_confirm_addr(vlan_dev, targets[i], 0); > bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], > - addr, vlan_id); > + addr, vlan_dev_vlan_id(vlan_dev)); > + rcu_read_unlock(); > continue; > } > + rcu_read_unlock(); > I think these lines can be re-arranged to something shorter like: rcu_read_lock(); vlan_dev = NULL; while ((vlan_dev = __vlan_find_dev_next(bond->dev, vlan_dev))) if (vlan_dev == rt->dst.dev) { pr_debug("basa: vlan match on %s %d\n", vlan_dev->name, vlan_dev_vlan_id(vlan_dev)); break; } if (vlan_dev) { ip_rt_put(rt); addr = bond_confirm_addr(vlan_dev, targets[i], 0); bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], addr, vlan_dev_vlan_id(vlan_dev)); } else { if (net_ratelimit()) pr_warning("%s: no path to arp_ip_target %pI4 via rt.dev %s\n", bond->dev->name, &targets[i], rt->dst.dev ? rt->dst.dev->name : "NULL"); ip_rt_put(rt); } rcu_read_unlock(); But either way is fine, I just think this one is more readable. Cheers, Nik -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index e52e2d5..f536d05 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2440,11 +2440,10 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op, __be32 dest_ static void bond_arp_send_all(struct bonding *bond, struct slave *slave) { - int i, vlan_id; - __be32 *targets = bond->params.arp_targets; - struct vlan_entry *vlan; - struct net_device *vlan_dev = NULL; + struct net_device *vlan_dev; struct rtable *rt; + __be32 *targets = bond->params.arp_targets; + int i; for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) { __be32 addr; @@ -2486,28 +2485,26 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave) continue; } - vlan_id = 0; - list_for_each_entry(vlan, &bond->vlan_list, vlan_list) { - rcu_read_lock(); - vlan_dev = __vlan_find_dev_deep(bond->dev, - htons(ETH_P_8021Q), - vlan->vlan_id); - rcu_read_unlock(); + vlan_dev = NULL; + + rcu_read_lock(); + while ((vlan_dev = __vlan_find_dev_next(bond->dev, vlan_dev))) if (vlan_dev == rt->dst.dev) { - vlan_id = vlan->vlan_id; pr_debug("basa: vlan match on %s %d\n", - vlan_dev->name, vlan_id); + vlan_dev->name, + vlan_dev_vlan_id(vlan_dev)); break; } - } - if (vlan_id && vlan_dev) { + if (vlan_dev) { ip_rt_put(rt); addr = bond_confirm_addr(vlan_dev, targets[i], 0); bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i], - addr, vlan_id); + addr, vlan_dev_vlan_id(vlan_dev)); + rcu_read_unlock(); continue; } + rcu_read_unlock(); if (net_ratelimit()) { pr_warning("%s: no path to arp_ip_target %pI4 via rt.dev %s\n",
RFC -> v1: use the new __vlan_find_dev_next(), also release rcu_read_lock() only after we stop using the vlan_dev. v1 -> v2: no change. Instead of looping through bond->vlan_list, loop through bond->dev->vlan_info via __vlan_find_dev_next() under rcu_read_lock(). CC: Jay Vosburgh <fubar@us.ibm.com> CC: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> --- drivers/net/bonding/bond_main.c | 29 +++++++++++++---------------- 1 files changed, 13 insertions(+), 16 deletions(-)