* [PATCH net-next 0/2] bonding: cleanup arp_interval related time math
@ 2013-08-03 1:50 Veaceslav Falico
2013-08-03 1:50 ` [PATCH net-next 1/2] bonding: call slave_last_rx() only once per slave Veaceslav Falico
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Veaceslav Falico @ 2013-08-03 1:50 UTC (permalink / raw)
To: netdev; +Cc: Jay Vosburgh, Andy Gospodarek, Veaceslav Falico
Current implementation uses heavily time_in_range() with similar, lengthy,
params. By moving it to a separate function we can simplify the code a lot.
First patch saves a possibly heavy slave_last_rx() function to a variable
and uses the variable in the calculation.
Second patch introduces helper function bond_time_in_interval() to make the
arp_interval math and uses it instead of lengthy time_in_range() calls.
Both of them don't change anything logically and in terms of speed and are
purely cleanup patches.
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 | 88 +++++++++++++++------------------------
1 files changed, 34 insertions(+), 54 deletions(-)
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH net-next 1/2] bonding: call slave_last_rx() only once per slave
2013-08-03 1:50 [PATCH net-next 0/2] bonding: cleanup arp_interval related time math Veaceslav Falico
@ 2013-08-03 1:50 ` Veaceslav Falico
2013-08-03 1:50 ` [PATCH net-next 2/2] bonding: add bond_time_in_interval() and use it for time comparison Veaceslav Falico
2013-08-05 19:20 ` [PATCH net-next 0/2] bonding: cleanup arp_interval related time math David Miller
2 siblings, 0 replies; 4+ messages in thread
From: Veaceslav Falico @ 2013-08-03 1:50 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, Jay Vosburgh, Andy Gospodarek
Simple cleanup to not call slave_last_rx() on every time function. It won't
give any measurable boost - but looks cleaner and easier to understand.
There are no time-consuming functions in between these calls, so it's safe
to call it in the beginning only once.
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 | 15 ++++++++-------
1 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 476df7d..cc13bfe 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2764,7 +2764,7 @@ re_arm:
*/
static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
{
- unsigned long trans_start;
+ unsigned long trans_start, last_rx;
struct slave *slave;
int extra_ticks;
int commit = 0;
@@ -2778,11 +2778,12 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
bond_for_each_slave(bond, slave) {
slave->new_link = BOND_LINK_NOCHANGE;
+ last_rx = slave_last_rx(bond, slave);
if (slave->link != BOND_LINK_UP) {
if (time_in_range(jiffies,
- slave_last_rx(bond, slave) - delta_in_ticks,
- slave_last_rx(bond, slave) + delta_in_ticks + extra_ticks)) {
+ last_rx - delta_in_ticks,
+ last_rx + delta_in_ticks + extra_ticks)) {
slave->new_link = BOND_LINK_UP;
commit++;
@@ -2817,8 +2818,8 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
if (!bond_is_active_slave(slave) &&
!bond->current_arp_slave &&
!time_in_range(jiffies,
- slave_last_rx(bond, slave) - delta_in_ticks,
- slave_last_rx(bond, slave) + 3 * delta_in_ticks + extra_ticks)) {
+ last_rx - delta_in_ticks,
+ last_rx + 3 * delta_in_ticks + extra_ticks)) {
slave->new_link = BOND_LINK_DOWN;
commit++;
@@ -2836,8 +2837,8 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
trans_start - delta_in_ticks,
trans_start + 2 * delta_in_ticks + extra_ticks) ||
!time_in_range(jiffies,
- slave_last_rx(bond, slave) - delta_in_ticks,
- slave_last_rx(bond, slave) + 2 * delta_in_ticks + extra_ticks))) {
+ last_rx - delta_in_ticks,
+ last_rx + 2 * delta_in_ticks + extra_ticks))) {
slave->new_link = BOND_LINK_DOWN;
commit++;
--
1.7.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH net-next 2/2] bonding: add bond_time_in_interval() and use it for time comparison
2013-08-03 1:50 [PATCH net-next 0/2] bonding: cleanup arp_interval related time math Veaceslav Falico
2013-08-03 1:50 ` [PATCH net-next 1/2] bonding: call slave_last_rx() only once per slave Veaceslav Falico
@ 2013-08-03 1:50 ` Veaceslav Falico
2013-08-05 19:20 ` [PATCH net-next 0/2] bonding: cleanup arp_interval related time math David Miller
2 siblings, 0 replies; 4+ messages in thread
From: Veaceslav Falico @ 2013-08-03 1:50 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, Jay Vosburgh, Andy Gospodarek
Currently we use a lot of time comparison math for arp_interval
comparisons, which are sometimes quite hard to read and understand.
All the time comparisons have one pattern:
(time - arp_interval_jiffies) <= jiffies <= (time + mod *
arp_interval_jiffies + arp_interval_jiffies/2)
Introduce a new helper - bond_time_in_interval(), which will do the math in
one place and, thus, will clean up the logical code. This helper introduces
a bit of overhead (by always calculating the jiffies from arp_interval),
however it's really not visible, considering that functions using it
usually run once in arp_interval milliseconds.
There are several lines slightly over 80 chars, however breaking them would
result in more hard-to-read code than several character after the 80 mark.
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 | 85 +++++++++++++++------------------------
1 files changed, 32 insertions(+), 53 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index cc13bfe..d58237b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2635,6 +2635,20 @@ out_unlock:
return RX_HANDLER_ANOTHER;
}
+/* function to verify if we're in the arp_interval timeslice, returns true if
+ * (last_act - arp_interval) <= jiffies <= (last_act + mod * arp_interval +
+ * arp_interval/2) . the arp_interval/2 is needed for really fast networks.
+ */
+static bool bond_time_in_interval(struct bonding *bond, unsigned long last_act,
+ int mod)
+{
+ int delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);
+
+ return time_in_range(jiffies,
+ last_act - delta_in_ticks,
+ last_act + mod * delta_in_ticks + delta_in_ticks/2);
+}
+
/*
* this function is called regularly to monitor each slave's link
* ensuring that traffic is being sent and received when arp monitoring
@@ -2648,13 +2662,9 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
arp_work.work);
struct slave *slave, *oldcurrent;
int do_failover = 0;
- int delta_in_ticks, extra_ticks;
read_lock(&bond->lock);
- delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);
- extra_ticks = delta_in_ticks / 2;
-
if (list_empty(&bond->slave_list))
goto re_arm;
@@ -2671,12 +2681,8 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
unsigned long trans_start = dev_trans_start(slave->dev);
if (slave->link != BOND_LINK_UP) {
- if (time_in_range(jiffies,
- trans_start - delta_in_ticks,
- trans_start + delta_in_ticks + extra_ticks) &&
- time_in_range(jiffies,
- slave->dev->last_rx - delta_in_ticks,
- slave->dev->last_rx + delta_in_ticks + extra_ticks)) {
+ if (bond_time_in_interval(bond, trans_start, 1) &&
+ bond_time_in_interval(bond, slave->dev->last_rx, 1)) {
slave->link = BOND_LINK_UP;
bond_set_active_slave(slave);
@@ -2704,12 +2710,8 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
* when the source ip is 0, so don't take the link down
* if we don't know our ip yet
*/
- if (!time_in_range(jiffies,
- trans_start - delta_in_ticks,
- trans_start + 2 * delta_in_ticks + extra_ticks) ||
- !time_in_range(jiffies,
- slave->dev->last_rx - delta_in_ticks,
- slave->dev->last_rx + 2 * delta_in_ticks + extra_ticks)) {
+ if (!bond_time_in_interval(bond, trans_start, 2) ||
+ !bond_time_in_interval(bond, slave->dev->last_rx, 2)) {
slave->link = BOND_LINK_DOWN;
bond_set_backup_slave(slave);
@@ -2749,7 +2751,8 @@ void bond_loadbalance_arp_mon(struct work_struct *work)
re_arm:
if (bond->params.arp_interval)
- queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks);
+ queue_delayed_work(bond->wq, &bond->arp_work,
+ msecs_to_jiffies(bond->params.arp_interval));
read_unlock(&bond->lock);
}
@@ -2762,33 +2765,21 @@ re_arm:
*
* Called with bond->lock held for read.
*/
-static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
+static int bond_ab_arp_inspect(struct bonding *bond)
{
unsigned long trans_start, last_rx;
struct slave *slave;
- int extra_ticks;
int commit = 0;
- /* All the time comparisons below need some extra time. Otherwise, on
- * fast networks the ARP probe/reply may arrive within the same jiffy
- * as it was sent. Then, the next time the ARP monitor is run, one
- * arp_interval will already have passed in the comparisons.
- */
- extra_ticks = delta_in_ticks / 2;
-
bond_for_each_slave(bond, slave) {
slave->new_link = BOND_LINK_NOCHANGE;
last_rx = slave_last_rx(bond, slave);
if (slave->link != BOND_LINK_UP) {
- if (time_in_range(jiffies,
- last_rx - delta_in_ticks,
- last_rx + delta_in_ticks + extra_ticks)) {
-
+ if (bond_time_in_interval(bond, last_rx, 1)) {
slave->new_link = BOND_LINK_UP;
commit++;
}
-
continue;
}
@@ -2797,9 +2788,7 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
* active. This avoids bouncing, as the last receive
* times need a full ARP monitor cycle to be updated.
*/
- if (time_in_range(jiffies,
- slave->jiffies - delta_in_ticks,
- slave->jiffies + 2 * delta_in_ticks + extra_ticks))
+ if (bond_time_in_interval(bond, slave->jiffies, 2))
continue;
/*
@@ -2817,10 +2806,7 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
*/
if (!bond_is_active_slave(slave) &&
!bond->current_arp_slave &&
- !time_in_range(jiffies,
- last_rx - delta_in_ticks,
- last_rx + 3 * delta_in_ticks + extra_ticks)) {
-
+ !bond_time_in_interval(bond, last_rx, 3)) {
slave->new_link = BOND_LINK_DOWN;
commit++;
}
@@ -2833,13 +2819,8 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
*/
trans_start = dev_trans_start(slave->dev);
if (bond_is_active_slave(slave) &&
- (!time_in_range(jiffies,
- trans_start - delta_in_ticks,
- trans_start + 2 * delta_in_ticks + extra_ticks) ||
- !time_in_range(jiffies,
- last_rx - delta_in_ticks,
- last_rx + 2 * delta_in_ticks + extra_ticks))) {
-
+ (!bond_time_in_interval(bond, trans_start, 2) ||
+ !bond_time_in_interval(bond, last_rx, 2))) {
slave->new_link = BOND_LINK_DOWN;
commit++;
}
@@ -2854,7 +2835,7 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
*
* Called with RTNL and bond->lock for read.
*/
-static void bond_ab_arp_commit(struct bonding *bond, int delta_in_ticks)
+static void bond_ab_arp_commit(struct bonding *bond)
{
unsigned long trans_start;
struct slave *slave;
@@ -2866,11 +2847,9 @@ static void bond_ab_arp_commit(struct bonding *bond, int delta_in_ticks)
case BOND_LINK_UP:
trans_start = dev_trans_start(slave->dev);
- if ((!bond->curr_active_slave &&
- time_in_range(jiffies,
- trans_start - delta_in_ticks,
- trans_start + delta_in_ticks + delta_in_ticks / 2)) ||
- bond->curr_active_slave != slave) {
+ if (bond->curr_active_slave != slave ||
+ (!bond->curr_active_slave &&
+ bond_time_in_interval(bond, trans_start, 1))) {
slave->link = BOND_LINK_UP;
if (bond->current_arp_slave) {
bond_set_slave_inactive_flags(
@@ -3011,7 +2990,7 @@ void bond_activebackup_arp_mon(struct work_struct *work)
should_notify_peers = bond_should_notify_peers(bond);
- if (bond_ab_arp_inspect(bond, delta_in_ticks)) {
+ if (bond_ab_arp_inspect(bond)) {
read_unlock(&bond->lock);
/* Race avoidance with bond_close flush of workqueue */
@@ -3024,7 +3003,7 @@ void bond_activebackup_arp_mon(struct work_struct *work)
read_lock(&bond->lock);
- bond_ab_arp_commit(bond, delta_in_ticks);
+ bond_ab_arp_commit(bond);
read_unlock(&bond->lock);
rtnl_unlock();
--
1.7.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next 0/2] bonding: cleanup arp_interval related time math
2013-08-03 1:50 [PATCH net-next 0/2] bonding: cleanup arp_interval related time math Veaceslav Falico
2013-08-03 1:50 ` [PATCH net-next 1/2] bonding: call slave_last_rx() only once per slave Veaceslav Falico
2013-08-03 1:50 ` [PATCH net-next 2/2] bonding: add bond_time_in_interval() and use it for time comparison Veaceslav Falico
@ 2013-08-05 19:20 ` David Miller
2 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2013-08-05 19:20 UTC (permalink / raw)
To: vfalico; +Cc: netdev, fubar, andy
From: Veaceslav Falico <vfalico@redhat.com>
Date: Sat, 3 Aug 2013 03:50:34 +0200
> Current implementation uses heavily time_in_range() with similar, lengthy,
> params. By moving it to a separate function we can simplify the code a lot.
>
> First patch saves a possibly heavy slave_last_rx() function to a variable
> and uses the variable in the calculation.
>
> Second patch introduces helper function bond_time_in_interval() to make the
> arp_interval math and uses it instead of lengthy time_in_range() calls.
>
> Both of them don't change anything logically and in terms of speed and are
> purely cleanup patches.
Series applied, thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-08-05 19:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-03 1:50 [PATCH net-next 0/2] bonding: cleanup arp_interval related time math Veaceslav Falico
2013-08-03 1:50 ` [PATCH net-next 1/2] bonding: call slave_last_rx() only once per slave Veaceslav Falico
2013-08-03 1:50 ` [PATCH net-next 2/2] bonding: add bond_time_in_interval() and use it for time comparison Veaceslav Falico
2013-08-05 19:20 ` [PATCH net-next 0/2] bonding: cleanup arp_interval related time math David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).