* [PATCH net-next 1/6] bonding: verify if bond has ip only once on arp validate @ 2013-06-19 17:34 Veaceslav Falico 2013-06-19 17:50 ` Jay Vosburgh 0 siblings, 1 reply; 3+ messages in thread From: Veaceslav Falico @ 2013-06-19 17:34 UTC (permalink / raw) To: netdev; +Cc: vfalico, fubar, andy, davem, linux, nicolas.2p.debian, rick.jones2 It's extra work to verify bond's ip presence for every slave, so take it out of the loop. Signed-off-by: Veaceslav Falico <vfalico@redhat•com> --- drivers/net/bonding/bond_main.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index bc1246f..3d8b5ba 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2602,13 +2602,16 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32 int i; __be32 *targets = bond->params.arp_targets; + if (!bond_has_this_ip(bond, tip)) { + pr_debug("bva: tip %pI4 not found\n", &tip); + return; + } + for (i = 0; (i < BOND_MAX_ARP_TARGETS) && targets[i]; i++) { - pr_debug("bva: sip %pI4 tip %pI4 t[%d] %pI4 bhti(tip) %d\n", - &sip, &tip, i, &targets[i], - bond_has_this_ip(bond, tip)); + pr_debug("bva: sip %pI4 tip %pI4 t[%d] %pI4 bhti(tip)\n", + &sip, &tip, i, &targets[i]); if (sip == targets[i]) { - if (bond_has_this_ip(bond, tip)) - slave->last_arp_rx = jiffies; + slave->last_arp_rx = jiffies; return; } } -- 1.7.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next 1/6] bonding: verify if bond has ip only once on arp validate 2013-06-19 17:34 [PATCH net-next 1/6] bonding: verify if bond has ip only once on arp validate Veaceslav Falico @ 2013-06-19 17:50 ` Jay Vosburgh 2013-06-19 18:48 ` Veaceslav Falico 0 siblings, 1 reply; 3+ messages in thread From: Jay Vosburgh @ 2013-06-19 17:50 UTC (permalink / raw) To: Veaceslav Falico Cc: netdev, andy, davem, linux, nicolas.2p.debian, rick.jones2 Veaceslav Falico <vfalico@redhat•com> wrote: >It's extra work to verify bond's ip presence for every slave, so take it >out of the loop. The current code doesn't verify for every slave (target address, actually). The call to bond_has_this_ip() happens at most once, if the sip (source IP) matches the arp_ip_target being inspected, after which the function returns. I can see that the patch will bypass the loop entirely if the bond lacks the IP, but I'm not sure that's a meaningful improvement, since it changes from calling bond_has_this_ip 0 or 1 times to always 1 time. -J >Signed-off-by: Veaceslav Falico <vfalico@redhat•com> >--- > drivers/net/bonding/bond_main.c | 13 ++++++++----- > 1 files changed, 8 insertions(+), 5 deletions(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index bc1246f..3d8b5ba 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -2602,13 +2602,16 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32 > int i; > __be32 *targets = bond->params.arp_targets; > >+ if (!bond_has_this_ip(bond, tip)) { >+ pr_debug("bva: tip %pI4 not found\n", &tip); >+ return; >+ } >+ > for (i = 0; (i < BOND_MAX_ARP_TARGETS) && targets[i]; i++) { >- pr_debug("bva: sip %pI4 tip %pI4 t[%d] %pI4 bhti(tip) %d\n", >- &sip, &tip, i, &targets[i], >- bond_has_this_ip(bond, tip)); >+ pr_debug("bva: sip %pI4 tip %pI4 t[%d] %pI4 bhti(tip)\n", >+ &sip, &tip, i, &targets[i]); > if (sip == targets[i]) { >- if (bond_has_this_ip(bond, tip)) >- slave->last_arp_rx = jiffies; >+ slave->last_arp_rx = jiffies; > return; > } > } >-- >1.7.1 > --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us•ibm.com ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net-next 1/6] bonding: verify if bond has ip only once on arp validate 2013-06-19 17:50 ` Jay Vosburgh @ 2013-06-19 18:48 ` Veaceslav Falico 0 siblings, 0 replies; 3+ messages in thread From: Veaceslav Falico @ 2013-06-19 18:48 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev, andy, davem, linux, nicolas.2p.debian, rick.jones2 On Wed, Jun 19, 2013 at 10:50:24AM -0700, Jay Vosburgh wrote: >Veaceslav Falico <vfalico@redhat•com> wrote: > >>It's extra work to verify bond's ip presence for every slave, so take it >>out of the loop. > > The current code doesn't verify for every slave (target address, >actually). The call to bond_has_this_ip() happens at most once, if the >sip (source IP) matches the arp_ip_target being inspected, after which >the function returns. > > I can see that the patch will bypass the loop entirely if the >bond lacks the IP, but I'm not sure that's a meaningful improvement, >since it changes from calling bond_has_this_ip 0 or 1 times to always 1 >time. Yep, you're right, I've misread the code. It's just not worth it, will drop this patch in v2. > > -J > > >>Signed-off-by: Veaceslav Falico <vfalico@redhat•com> >>--- >> drivers/net/bonding/bond_main.c | 13 ++++++++----- >> 1 files changed, 8 insertions(+), 5 deletions(-) >> >>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >>index bc1246f..3d8b5ba 100644 >>--- a/drivers/net/bonding/bond_main.c >>+++ b/drivers/net/bonding/bond_main.c >>@@ -2602,13 +2602,16 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32 >> int i; >> __be32 *targets = bond->params.arp_targets; >> >>+ if (!bond_has_this_ip(bond, tip)) { >>+ pr_debug("bva: tip %pI4 not found\n", &tip); >>+ return; >>+ } >>+ >> for (i = 0; (i < BOND_MAX_ARP_TARGETS) && targets[i]; i++) { >>- pr_debug("bva: sip %pI4 tip %pI4 t[%d] %pI4 bhti(tip) %d\n", >>- &sip, &tip, i, &targets[i], >>- bond_has_this_ip(bond, tip)); >>+ pr_debug("bva: sip %pI4 tip %pI4 t[%d] %pI4 bhti(tip)\n", >>+ &sip, &tip, i, &targets[i]); >> if (sip == targets[i]) { >>- if (bond_has_this_ip(bond, tip)) >>- slave->last_arp_rx = jiffies; >>+ slave->last_arp_rx = jiffies; >> return; >> } >> } >>-- >>1.7.1 >> > >--- > -Jay Vosburgh, IBM Linux Technology Center, fubar@us•ibm.com > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-06-19 18:49 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-06-19 17:34 [PATCH net-next 1/6] bonding: verify if bond has ip only once on arp validate Veaceslav Falico 2013-06-19 17:50 ` Jay Vosburgh 2013-06-19 18:48 ` Veaceslav Falico
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox