* [PATCH 0/8] bonding: Several fixes, new hash mode @ 2007-12-07 7:40 Jay Vosburgh 2007-12-07 7:40 ` [PATCH 1/8] bonding: Remove trailing NULs from sysfs interface Jay Vosburgh 0 siblings, 1 reply; 10+ messages in thread From: Jay Vosburgh @ 2007-12-07 7:40 UTC (permalink / raw) To: netdev; +Cc: jgarzik Patch series to fix some bugs, fix coding style, and add a new hash mode for balance-xor/802.3ad modes. Jeff: please apply to upstream. Patch 8 should arguably go in to 2.6.24, as it's a bug in the locking fixes added there and can cause an oops; is it too late for that? [PATCH 1/8] bonding: Remove trailing NULs from sysfs interface. [PATCH 2/8] bonding: Return nothing for not applicable values [PATCH 3/8] bonding: Purely cosmetic: rename a local variable [PATCH 4/8] bonding: Coding style: break line after the if condition [PATCH 5/8] bonding: Allow setting and querying xmit policy regardless of mode [PATCH 6/8] bonding: Fix time comparison [PATCH 7/8] bonding: Add new layer2+3 hash for xor/802.3ad modes [PATCH 8/8] bonding: Fix race at module unload -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/8] bonding: Remove trailing NULs from sysfs interface. 2007-12-07 7:40 [PATCH 0/8] bonding: Several fixes, new hash mode Jay Vosburgh @ 2007-12-07 7:40 ` Jay Vosburgh 2007-12-07 7:40 ` [PATCH 2/8] bonding: Return nothing for not applicable values Jay Vosburgh 2007-12-07 20:02 ` [PATCH 1/8] bonding: Remove trailing NULs from sysfs interface Jeff Garzik 0 siblings, 2 replies; 10+ messages in thread From: Jay Vosburgh @ 2007-12-07 7:40 UTC (permalink / raw) To: netdev; +Cc: jgarzik, Wagner Ferenc From: Wagner Ferenc <wferi@niif.hu> From: Wagner Ferenc <wferi@niif.hu> Also remove trailing spaces from multivalued files. This fixes output like for example: $ od -c /sys/class/net/bond0/bonding/slaves 0000000 e t h - l e f t e t h - r i g 0000020 h t \n \0 0000025 It mostly entails deleting '+1'-s after sprintf() calls: the return value of sprintf is the number of characters printed, without the closing NUL, ie. exactly what the sysfs interface requires. The three multivalue cases are different, because they also have to swallow back a trailing space. Signed-off-by: Ferenc Wagner <wferi@niif.hu> Acked-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_sysfs.c | 66 +++++++++++++++++-------------------- 1 files changed, 30 insertions(+), 36 deletions(-) diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index b29330d..a3f1b4a 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -86,14 +86,13 @@ static ssize_t bonding_show_bonds(struct class *cls, char *buffer) /* not enough space for another interface name */ if ((PAGE_SIZE - res) > 10) res = PAGE_SIZE - 10; - res += sprintf(buffer + res, "++more++"); + res += sprintf(buffer + res, "++more++ "); break; } res += sprintf(buffer + res, "%s ", bond->dev->name); } - res += sprintf(buffer + res, "\n"); - res++; + if (res) buffer[res-1] = '\n'; /* eat the leftover space */ up_read(&(bonding_rwsem)); return res; } @@ -235,14 +234,13 @@ static ssize_t bonding_show_slaves(struct device *d, /* not enough space for another interface name */ if ((PAGE_SIZE - res) > 10) res = PAGE_SIZE - 10; - res += sprintf(buf + res, "++more++"); + res += sprintf(buf + res, "++more++ "); break; } res += sprintf(buf + res, "%s ", slave->dev->name); } read_unlock(&bond->lock); - res += sprintf(buf + res, "\n"); - res++; + if (res) buf[res-1] = '\n'; /* eat the leftover space */ return res; } @@ -406,7 +404,7 @@ static ssize_t bonding_show_mode(struct device *d, return sprintf(buf, "%s %d\n", bond_mode_tbl[bond->params.mode].modename, - bond->params.mode) + 1; + bond->params.mode); } static ssize_t bonding_store_mode(struct device *d, @@ -463,11 +461,11 @@ static ssize_t bonding_show_xmit_hash(struct device *d, if ((bond->params.mode != BOND_MODE_XOR) && (bond->params.mode != BOND_MODE_8023AD)) { // Not Applicable - count = sprintf(buf, "NA\n") + 1; + count = sprintf(buf, "NA\n"); } else { count = sprintf(buf, "%s %d\n", xmit_hashtype_tbl[bond->params.xmit_policy].modename, - bond->params.xmit_policy) + 1; + bond->params.xmit_policy); } return count; @@ -527,7 +525,7 @@ static ssize_t bonding_show_arp_validate(struct device *d, return sprintf(buf, "%s %d\n", arp_validate_tbl[bond->params.arp_validate].modename, - bond->params.arp_validate) + 1; + bond->params.arp_validate); } static ssize_t bonding_store_arp_validate(struct device *d, @@ -627,7 +625,7 @@ static ssize_t bonding_show_arp_interval(struct device *d, { struct bonding *bond = to_bond(d); - return sprintf(buf, "%d\n", bond->params.arp_interval) + 1; + return sprintf(buf, "%d\n", bond->params.arp_interval); } static ssize_t bonding_store_arp_interval(struct device *d, @@ -711,10 +709,7 @@ static ssize_t bonding_show_arp_targets(struct device *d, res += sprintf(buf + res, "%u.%u.%u.%u ", NIPQUAD(bond->params.arp_targets[i])); } - if (res) - res--; /* eat the leftover space */ - res += sprintf(buf + res, "\n"); - res++; + if (res) buf[res-1] = '\n'; /* eat the leftover space */ return res; } @@ -815,7 +810,7 @@ static ssize_t bonding_show_downdelay(struct device *d, { struct bonding *bond = to_bond(d); - return sprintf(buf, "%d\n", bond->params.downdelay * bond->params.miimon) + 1; + return sprintf(buf, "%d\n", bond->params.downdelay * bond->params.miimon); } static ssize_t bonding_store_downdelay(struct device *d, @@ -872,7 +867,7 @@ static ssize_t bonding_show_updelay(struct device *d, { struct bonding *bond = to_bond(d); - return sprintf(buf, "%d\n", bond->params.updelay * bond->params.miimon) + 1; + return sprintf(buf, "%d\n", bond->params.updelay * bond->params.miimon); } @@ -936,7 +931,7 @@ static ssize_t bonding_show_lacp(struct device *d, return sprintf(buf, "%s %d\n", bond_lacp_tbl[bond->params.lacp_fast].modename, - bond->params.lacp_fast) + 1; + bond->params.lacp_fast); } static ssize_t bonding_store_lacp(struct device *d, @@ -992,7 +987,7 @@ static ssize_t bonding_show_miimon(struct device *d, { struct bonding *bond = to_bond(d); - return sprintf(buf, "%d\n", bond->params.miimon) + 1; + return sprintf(buf, "%d\n", bond->params.miimon); } static ssize_t bonding_store_miimon(struct device *d, @@ -1083,9 +1078,9 @@ static ssize_t bonding_show_primary(struct device *d, struct bonding *bond = to_bond(d); if (bond->primary_slave) - count = sprintf(buf, "%s\n", bond->primary_slave->dev->name) + 1; + count = sprintf(buf, "%s\n", bond->primary_slave->dev->name); else - count = sprintf(buf, "\n") + 1; + count = sprintf(buf, "\n"); return count; } @@ -1149,7 +1144,7 @@ static ssize_t bonding_show_carrier(struct device *d, { struct bonding *bond = to_bond(d); - return sprintf(buf, "%d\n", bond->params.use_carrier) + 1; + return sprintf(buf, "%d\n", bond->params.use_carrier); } static ssize_t bonding_store_carrier(struct device *d, @@ -1198,9 +1193,9 @@ static ssize_t bonding_show_active_slave(struct device *d, read_unlock(&bond->curr_slave_lock); if (USES_PRIMARY(bond->params.mode) && curr) - count = sprintf(buf, "%s\n", curr->dev->name) + 1; + count = sprintf(buf, "%s\n", curr->dev->name); else - count = sprintf(buf, "\n") + 1; + count = sprintf(buf, "\n"); return count; } @@ -1295,7 +1290,7 @@ static ssize_t bonding_show_mii_status(struct device *d, curr = bond->curr_active_slave; read_unlock(&bond->curr_slave_lock); - return sprintf(buf, "%s\n", (curr) ? "up" : "down") + 1; + return sprintf(buf, "%s\n", (curr) ? "up" : "down"); } static DEVICE_ATTR(mii_status, S_IRUGO, bonding_show_mii_status, NULL); @@ -1312,10 +1307,10 @@ static ssize_t bonding_show_ad_aggregator(struct device *d, if (bond->params.mode == BOND_MODE_8023AD) { struct ad_info ad_info; - count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0 : ad_info.aggregator_id) + 1; + count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0 : ad_info.aggregator_id); } else - count = sprintf(buf, "\n") + 1; + count = sprintf(buf, "\n"); return count; } @@ -1334,10 +1329,10 @@ static ssize_t bonding_show_ad_num_ports(struct device *d, if (bond->params.mode == BOND_MODE_8023AD) { struct ad_info ad_info; - count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0: ad_info.ports) + 1; + count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0: ad_info.ports); } else - count = sprintf(buf, "\n") + 1; + count = sprintf(buf, "\n"); return count; } @@ -1356,10 +1351,10 @@ static ssize_t bonding_show_ad_actor_key(struct device *d, if (bond->params.mode == BOND_MODE_8023AD) { struct ad_info ad_info; - count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0 : ad_info.actor_key) + 1; + count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0 : ad_info.actor_key); } else - count = sprintf(buf, "\n") + 1; + count = sprintf(buf, "\n"); return count; } @@ -1378,10 +1373,10 @@ static ssize_t bonding_show_ad_partner_key(struct device *d, if (bond->params.mode == BOND_MODE_8023AD) { struct ad_info ad_info; - count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0 : ad_info.partner_key) + 1; + count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0 : ad_info.partner_key); } else - count = sprintf(buf, "\n") + 1; + count = sprintf(buf, "\n"); return count; } @@ -1403,12 +1398,11 @@ static ssize_t bonding_show_ad_partner_mac(struct device *d, struct ad_info ad_info; if (!bond_3ad_get_active_agg_info(bond, &ad_info)) { count = sprintf(buf,"%s\n", - print_mac(mac, ad_info.partner_system)) - + 1; + print_mac(mac, ad_info.partner_system)); } } else - count = sprintf(buf, "\n") + 1; + count = sprintf(buf, "\n"); return count; } -- 1.5.3.4.206.g58ba4-dirty ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/8] bonding: Return nothing for not applicable values 2007-12-07 7:40 ` [PATCH 1/8] bonding: Remove trailing NULs from sysfs interface Jay Vosburgh @ 2007-12-07 7:40 ` Jay Vosburgh 2007-12-07 7:40 ` [PATCH 3/8] bonding: Purely cosmetic: rename a local variable Jay Vosburgh 2007-12-07 20:02 ` [PATCH 1/8] bonding: Remove trailing NULs from sysfs interface Jeff Garzik 1 sibling, 1 reply; 10+ messages in thread From: Jay Vosburgh @ 2007-12-07 7:40 UTC (permalink / raw) To: netdev; +Cc: jgarzik, Wagner Ferenc From: Wagner Ferenc <wferi@niif.hu> From: Wagner Ferenc <wferi@niif.hu> The previous code returned '\n' (that is, a single empty line) from most files, with one exception (xmit_hash_policy), where it returned 'NA\n'. This patch consolidates each file to return nothing at all if not applicable, not even a '\n'. I find this behaviour more usual, more useful, more efficient and shorter to code from both sides. Signed-off-by: Ferenc Wagner <wferi@niif.hu> Acked-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_sysfs.c | 25 ++++--------------------- 1 files changed, 4 insertions(+), 21 deletions(-) diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index a3f1b4a..6bb91e2 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -455,14 +455,11 @@ static ssize_t bonding_show_xmit_hash(struct device *d, struct device_attribute *attr, char *buf) { - int count; + int count = 0; struct bonding *bond = to_bond(d); - if ((bond->params.mode != BOND_MODE_XOR) && - (bond->params.mode != BOND_MODE_8023AD)) { - // Not Applicable - count = sprintf(buf, "NA\n"); - } else { + if ((bond->params.mode == BOND_MODE_XOR) || + (bond->params.mode == BOND_MODE_8023AD)) { count = sprintf(buf, "%s %d\n", xmit_hashtype_tbl[bond->params.xmit_policy].modename, bond->params.xmit_policy); @@ -1079,8 +1076,6 @@ static ssize_t bonding_show_primary(struct device *d, if (bond->primary_slave) count = sprintf(buf, "%s\n", bond->primary_slave->dev->name); - else - count = sprintf(buf, "\n"); return count; } @@ -1186,7 +1181,7 @@ static ssize_t bonding_show_active_slave(struct device *d, { struct slave *curr; struct bonding *bond = to_bond(d); - int count; + int count = 0; read_lock(&bond->curr_slave_lock); curr = bond->curr_active_slave; @@ -1194,8 +1189,6 @@ static ssize_t bonding_show_active_slave(struct device *d, if (USES_PRIMARY(bond->params.mode) && curr) count = sprintf(buf, "%s\n", curr->dev->name); - else - count = sprintf(buf, "\n"); return count; } @@ -1309,8 +1302,6 @@ static ssize_t bonding_show_ad_aggregator(struct device *d, struct ad_info ad_info; count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0 : ad_info.aggregator_id); } - else - count = sprintf(buf, "\n"); return count; } @@ -1331,8 +1322,6 @@ static ssize_t bonding_show_ad_num_ports(struct device *d, struct ad_info ad_info; count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0: ad_info.ports); } - else - count = sprintf(buf, "\n"); return count; } @@ -1353,8 +1342,6 @@ static ssize_t bonding_show_ad_actor_key(struct device *d, struct ad_info ad_info; count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0 : ad_info.actor_key); } - else - count = sprintf(buf, "\n"); return count; } @@ -1375,8 +1362,6 @@ static ssize_t bonding_show_ad_partner_key(struct device *d, struct ad_info ad_info; count = sprintf(buf, "%d\n", (bond_3ad_get_active_agg_info(bond, &ad_info)) ? 0 : ad_info.partner_key); } - else - count = sprintf(buf, "\n"); return count; } @@ -1401,8 +1386,6 @@ static ssize_t bonding_show_ad_partner_mac(struct device *d, print_mac(mac, ad_info.partner_system)); } } - else - count = sprintf(buf, "\n"); return count; } -- 1.5.3.4.206.g58ba4-dirty ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/8] bonding: Purely cosmetic: rename a local variable 2007-12-07 7:40 ` [PATCH 2/8] bonding: Return nothing for not applicable values Jay Vosburgh @ 2007-12-07 7:40 ` Jay Vosburgh 2007-12-07 7:40 ` [PATCH 4/8] bonding: Coding style: break line after the if condition Jay Vosburgh 0 siblings, 1 reply; 10+ messages in thread From: Jay Vosburgh @ 2007-12-07 7:40 UTC (permalink / raw) To: netdev; +Cc: jgarzik, Wagner Ferenc From: Wagner Ferenc <wferi@niif.hu> From: Wagner Ferenc <wferi@niif.hu> Code for rendering multivalue sysfs files occurs three times in this module. Rename 'buffer' to 'buf' in the first, for the sake of consistency. Signed-off-by: Ferenc Wagner <wferi@niif.hu> Acked-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_sysfs.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index 6bb91e2..5c31f5c 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -74,7 +74,7 @@ struct rw_semaphore bonding_rwsem; * "show" function for the bond_masters attribute. * The class parameter is ignored. */ -static ssize_t bonding_show_bonds(struct class *cls, char *buffer) +static ssize_t bonding_show_bonds(struct class *cls, char *buf) { int res = 0; struct bonding *bond; @@ -86,13 +86,12 @@ static ssize_t bonding_show_bonds(struct class *cls, char *buffer) /* not enough space for another interface name */ if ((PAGE_SIZE - res) > 10) res = PAGE_SIZE - 10; - res += sprintf(buffer + res, "++more++ "); + res += sprintf(buf + res, "++more++ "); break; } - res += sprintf(buffer + res, "%s ", - bond->dev->name); + res += sprintf(buf + res, "%s ", bond->dev->name); } - if (res) buffer[res-1] = '\n'; /* eat the leftover space */ + if (res) buf[res-1] = '\n'; /* eat the leftover space */ up_read(&(bonding_rwsem)); return res; } -- 1.5.3.4.206.g58ba4-dirty ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/8] bonding: Coding style: break line after the if condition 2007-12-07 7:40 ` [PATCH 3/8] bonding: Purely cosmetic: rename a local variable Jay Vosburgh @ 2007-12-07 7:40 ` Jay Vosburgh 2007-12-07 7:40 ` [PATCH 5/8] bonding: Allow setting and querying xmit policy regardless of mode Jay Vosburgh 0 siblings, 1 reply; 10+ messages in thread From: Jay Vosburgh @ 2007-12-07 7:40 UTC (permalink / raw) To: netdev; +Cc: jgarzik, Wagner Ferenc From: Wagner Ferenc <wferi@niif.hu> From: Wagner Ferenc <wferi@niif.hu> Adhere to coding style: break line after the if condition Signed-off-by: Ferenc Wagner <wferi@niif.hu> Acked-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_sysfs.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index 5c31f5c..9de2c52 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -91,7 +91,8 @@ static ssize_t bonding_show_bonds(struct class *cls, char *buf) } res += sprintf(buf + res, "%s ", bond->dev->name); } - if (res) buf[res-1] = '\n'; /* eat the leftover space */ + if (res) + buf[res-1] = '\n'; /* eat the leftover space */ up_read(&(bonding_rwsem)); return res; } @@ -239,7 +240,8 @@ static ssize_t bonding_show_slaves(struct device *d, res += sprintf(buf + res, "%s ", slave->dev->name); } read_unlock(&bond->lock); - if (res) buf[res-1] = '\n'; /* eat the leftover space */ + if (res) + buf[res-1] = '\n'; /* eat the leftover space */ return res; } @@ -705,7 +707,8 @@ static ssize_t bonding_show_arp_targets(struct device *d, res += sprintf(buf + res, "%u.%u.%u.%u ", NIPQUAD(bond->params.arp_targets[i])); } - if (res) buf[res-1] = '\n'; /* eat the leftover space */ + if (res) + buf[res-1] = '\n'; /* eat the leftover space */ return res; } -- 1.5.3.4.206.g58ba4-dirty ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/8] bonding: Allow setting and querying xmit policy regardless of mode 2007-12-07 7:40 ` [PATCH 4/8] bonding: Coding style: break line after the if condition Jay Vosburgh @ 2007-12-07 7:40 ` Jay Vosburgh 2007-12-07 7:40 ` [PATCH 6/8] bonding: Fix time comparison Jay Vosburgh 0 siblings, 1 reply; 10+ messages in thread From: Jay Vosburgh @ 2007-12-07 7:40 UTC (permalink / raw) To: netdev; +Cc: jgarzik, Wagner Ferenc From: Wagner Ferenc <wferi@niif.hu> From: Wagner Ferenc <wferi@niif.hu> For consistency with the behaviour of the arp_ip_target option, let /sys/class/net/bond0/bonding/xmit_hash_policy accept and report current policy even if the bonding mode in effect does not use it. Signed-off-by: Ferenc Wagner <wferi@niif.hu> Acked-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_sysfs.c | 21 +++------------------ 1 files changed, 3 insertions(+), 18 deletions(-) diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index 9de2c52..11b76b3 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -456,17 +456,11 @@ static ssize_t bonding_show_xmit_hash(struct device *d, struct device_attribute *attr, char *buf) { - int count = 0; struct bonding *bond = to_bond(d); - if ((bond->params.mode == BOND_MODE_XOR) || - (bond->params.mode == BOND_MODE_8023AD)) { - count = sprintf(buf, "%s %d\n", - xmit_hashtype_tbl[bond->params.xmit_policy].modename, - bond->params.xmit_policy); - } - - return count; + return sprintf(buf, "%s %d\n", + xmit_hashtype_tbl[bond->params.xmit_policy].modename, + bond->params.xmit_policy); } static ssize_t bonding_store_xmit_hash(struct device *d, @@ -484,15 +478,6 @@ static ssize_t bonding_store_xmit_hash(struct device *d, goto out; } - if ((bond->params.mode != BOND_MODE_XOR) && - (bond->params.mode != BOND_MODE_8023AD)) { - printk(KERN_ERR DRV_NAME - "%s: Transmit hash policy is irrelevant in this mode.\n", - bond->dev->name); - ret = -EPERM; - goto out; - } - new_value = bond_parse_parm((char *)buf, xmit_hashtype_tbl); if (new_value < 0) { printk(KERN_ERR DRV_NAME -- 1.5.3.4.206.g58ba4-dirty ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 6/8] bonding: Fix time comparison 2007-12-07 7:40 ` [PATCH 5/8] bonding: Allow setting and querying xmit policy regardless of mode Jay Vosburgh @ 2007-12-07 7:40 ` Jay Vosburgh 2007-12-07 7:40 ` [PATCH 7/8] bonding: Add new layer2+3 hash for xor/802.3ad modes Jay Vosburgh 0 siblings, 1 reply; 10+ messages in thread From: Jay Vosburgh @ 2007-12-07 7:40 UTC (permalink / raw) To: netdev; +Cc: jgarzik, David Sterba From: David Sterba <dsterba@suse.cz> From: David Sterba <dsterba@suse.cz> Use macros for comparing jiffies. Jiffies' wrap caused missed events and hangs. Module reinsert was needed to make bonding work again. Signed-off-by: David Sterba <dsterba@suse.cz> Acked-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_main.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 423298c..e4a4714 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -74,6 +74,7 @@ #include <linux/ethtool.h> #include <linux/if_vlan.h> #include <linux/if_bonding.h> +#include <linux/jiffies.h> #include <net/route.h> #include <net/net_namespace.h> #include "bonding.h" @@ -2722,8 +2723,8 @@ void bond_loadbalance_arp_mon(struct work_struct *work) */ bond_for_each_slave(bond, slave, i) { if (slave->link != BOND_LINK_UP) { - if (((jiffies - slave->dev->trans_start) <= delta_in_ticks) && - ((jiffies - slave->dev->last_rx) <= delta_in_ticks)) { + if (time_before_eq(jiffies, slave->dev->trans_start + delta_in_ticks) && + time_before_eq(jiffies, slave->dev->last_rx + delta_in_ticks)) { slave->link = BOND_LINK_UP; slave->state = BOND_STATE_ACTIVE; @@ -2754,8 +2755,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 (((jiffies - slave->dev->trans_start) >= (2*delta_in_ticks)) || - (((jiffies - slave->dev->last_rx) >= (2*delta_in_ticks)) && + if (time_after_eq(jiffies, slave->dev->trans_start + 2*delta_in_ticks) || + (time_after_eq(jiffies, slave->dev->last_rx + 2*delta_in_ticks) && bond_has_ip(bond))) { slave->link = BOND_LINK_DOWN; @@ -2848,8 +2849,8 @@ void bond_activebackup_arp_mon(struct work_struct *work) */ bond_for_each_slave(bond, slave, i) { if (slave->link != BOND_LINK_UP) { - if ((jiffies - slave_last_rx(bond, slave)) <= - delta_in_ticks) { + if (time_before_eq(jiffies, + slave_last_rx(bond, slave) + delta_in_ticks)) { slave->link = BOND_LINK_UP; @@ -2858,7 +2859,7 @@ void bond_activebackup_arp_mon(struct work_struct *work) write_lock_bh(&bond->curr_slave_lock); if ((!bond->curr_active_slave) && - ((jiffies - slave->dev->trans_start) <= delta_in_ticks)) { + time_before_eq(jiffies, slave->dev->trans_start + delta_in_ticks)) { bond_change_active_slave(bond, slave); bond->current_arp_slave = NULL; } else if (bond->curr_active_slave != slave) { @@ -2897,7 +2898,7 @@ void bond_activebackup_arp_mon(struct work_struct *work) if ((slave != bond->curr_active_slave) && (!bond->current_arp_slave) && - (((jiffies - slave_last_rx(bond, slave)) >= 3*delta_in_ticks) && + (time_after_eq(jiffies, slave_last_rx(bond, slave) + 3*delta_in_ticks) && bond_has_ip(bond))) { /* a backup slave has gone down; three times * the delta allows the current slave to be @@ -2943,10 +2944,10 @@ void bond_activebackup_arp_mon(struct work_struct *work) * before being taken out. if a primary is being used, check * if it is up and needs to take over as the curr_active_slave */ - if ((((jiffies - slave->dev->trans_start) >= (2*delta_in_ticks)) || - (((jiffies - slave_last_rx(bond, slave)) >= (2*delta_in_ticks)) && - bond_has_ip(bond))) && - ((jiffies - slave->jiffies) >= 2*delta_in_ticks)) { + if ((time_after_eq(jiffies, slave->dev->trans_start + 2*delta_in_ticks) || + (time_after_eq(jiffies, slave_last_rx(bond, slave) + 2*delta_in_ticks) && + bond_has_ip(bond))) && + time_after_eq(jiffies, slave->jiffies + 2*delta_in_ticks)) { slave->link = BOND_LINK_DOWN; -- 1.5.3.4.206.g58ba4-dirty ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 7/8] bonding: Add new layer2+3 hash for xor/802.3ad modes 2007-12-07 7:40 ` [PATCH 6/8] bonding: Fix time comparison Jay Vosburgh @ 2007-12-07 7:40 ` Jay Vosburgh 2007-12-07 7:40 ` [PATCH 8/8] bonding: Fix race at module unload Jay Vosburgh 0 siblings, 1 reply; 10+ messages in thread From: Jay Vosburgh @ 2007-12-07 7:40 UTC (permalink / raw) To: netdev; +Cc: jgarzik, Glenn Griffin, Jay Vosburgh Add new hash for balance-xor and 802.3ad modes. Originally submitted by "Glenn Griffin" <ggriffin.kernel@gmail.com>; modified by Jay Vosburgh to move setting of hash policy out of line, tweak the documentation update and add version update to 3.2.2. Glenn's original comment follows: Included is a patch for a new xmit_hash_policy for the bonding driver that selects slaves based on MAC and IP information. This is a middle ground between what currently exists in the layer2 only policy and the layer3+4 policy. This policy strives to be fully 802.3ad compliant by transmitting every packet of any particular flow over the same link. As documented the layer3+4 policy is not fully compliant for extreme cases such as ip fragmentation, so this policy is a nice compromise for environments that require full compliance but desire more than the layer2 only policy. Signed-off-by: "Glenn Griffin" <ggriffin.kernel@gmail.com> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> --- Documentation/networking/bonding.txt | 29 +++++++++++++++++++- drivers/net/bonding/bond_main.c | 48 ++++++++++++++++++++++++++------- drivers/net/bonding/bonding.h | 4 +- include/linux/if_bonding.h | 3 +- 4 files changed, 69 insertions(+), 15 deletions(-) diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt index eda0f06..a0cda06 100644 --- a/Documentation/networking/bonding.txt +++ b/Documentation/networking/bonding.txt @@ -559,6 +559,30 @@ xmit_hash_policy This algorithm is 802.3ad compliant. + layer2+3 + + This policy uses a combination of layer2 and layer3 + protocol information to generate the hash. + + Uses XOR of hardware MAC addresses and IP addresses to + generate the hash. The formula is + + (((source IP XOR dest IP) AND 0xffff) XOR + ( source MAC XOR destination MAC )) + modulo slave count + + This algorithm will place all traffic to a particular + network peer on the same slave. For non-IP traffic, + the formula is the same as for the layer2 transmit + hash policy. + + This policy is intended to provide a more balanced + distribution of traffic than layer2 alone, especially + in environments where a layer3 gateway device is + required to reach most destinations. + + This algorithm is 802.3ad complient. + layer3+4 This policy uses upper layer protocol information, @@ -594,8 +618,9 @@ xmit_hash_policy or may not tolerate this noncompliance. The default value is layer2. This option was added in bonding -version 2.6.3. In earlier versions of bonding, this parameter does -not exist, and the layer2 policy is the only policy. + version 2.6.3. In earlier versions of bonding, this parameter + does not exist, and the layer2 policy is the only policy. The + layer2+3 value was added for bonding version 3.2.2. 3. Configuring Bonding Devices diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index e4a4714..08879d5 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -175,6 +175,7 @@ struct bond_parm_tbl bond_mode_tbl[] = { struct bond_parm_tbl xmit_hashtype_tbl[] = { { "layer2", BOND_XMIT_POLICY_LAYER2}, { "layer3+4", BOND_XMIT_POLICY_LAYER34}, +{ "layer2+3", BOND_XMIT_POLICY_LAYER23}, { NULL, -1}, }; @@ -3605,6 +3606,24 @@ void bond_unregister_arp(struct bonding *bond) /*---------------------------- Hashing Policies -----------------------------*/ /* + * Hash for the output device based upon layer 2 and layer 3 data. If + * the packet is not IP mimic bond_xmit_hash_policy_l2() + */ +static int bond_xmit_hash_policy_l23(struct sk_buff *skb, + struct net_device *bond_dev, int count) +{ + struct ethhdr *data = (struct ethhdr *)skb->data; + struct iphdr *iph = ip_hdr(skb); + + if (skb->protocol == __constant_htons(ETH_P_IP)) { + return ((ntohl(iph->saddr ^ iph->daddr) & 0xffff) ^ + (data->h_dest[5] ^ bond_dev->dev_addr[5])) % count; + } + + return (data->h_dest[5] ^ bond_dev->dev_addr[5]) % count; +} + +/* * Hash for the output device based upon layer 3 and layer 4 data. If * the packet is a frag or not TCP or UDP, just use layer 3 data. If it is * altogether not IP, mimic bond_xmit_hash_policy_l2() @@ -4306,6 +4325,22 @@ out: /*------------------------- Device initialization ---------------------------*/ +static void bond_set_xmit_hash_policy(struct bonding *bond) +{ + switch (bond->params.xmit_policy) { + case BOND_XMIT_POLICY_LAYER23: + bond->xmit_hash_policy = bond_xmit_hash_policy_l23; + break; + case BOND_XMIT_POLICY_LAYER34: + bond->xmit_hash_policy = bond_xmit_hash_policy_l34; + break; + case BOND_XMIT_POLICY_LAYER2: + default: + bond->xmit_hash_policy = bond_xmit_hash_policy_l2; + break; + } +} + /* * set bond mode specific net device operations */ @@ -4322,10 +4357,7 @@ void bond_set_mode_ops(struct bonding *bond, int mode) break; case BOND_MODE_XOR: bond_dev->hard_start_xmit = bond_xmit_xor; - if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34) - bond->xmit_hash_policy = bond_xmit_hash_policy_l34; - else - bond->xmit_hash_policy = bond_xmit_hash_policy_l2; + bond_set_xmit_hash_policy(bond); break; case BOND_MODE_BROADCAST: bond_dev->hard_start_xmit = bond_xmit_broadcast; @@ -4333,10 +4365,7 @@ void bond_set_mode_ops(struct bonding *bond, int mode) case BOND_MODE_8023AD: bond_set_master_3ad_flags(bond); bond_dev->hard_start_xmit = bond_3ad_xmit_xor; - if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34) - bond->xmit_hash_policy = bond_xmit_hash_policy_l34; - else - bond->xmit_hash_policy = bond_xmit_hash_policy_l2; + bond_set_xmit_hash_policy(bond); break; case BOND_MODE_ALB: bond_set_master_alb_flags(bond); @@ -4498,8 +4527,7 @@ int bond_parse_parm(char *mode_arg, struct bond_parm_tbl *tbl) for (i = 0; tbl[i].modename; i++) { if ((isdigit(*mode_arg) && tbl[i].mode == simple_strtol(mode_arg, NULL, 0)) || - (strncmp(mode_arg, tbl[i].modename, - strlen(tbl[i].modename)) == 0)) { + (strcmp(mode_arg, tbl[i].modename) == 0)) { return tbl[i].mode; } } diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index 61c1b45..ccafc74 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -22,8 +22,8 @@ #include "bond_3ad.h" #include "bond_alb.h" -#define DRV_VERSION "3.2.1" -#define DRV_RELDATE "October 15, 2007" +#define DRV_VERSION "3.2.2" +#define DRV_RELDATE "December 6, 2007" #define DRV_NAME "bonding" #define DRV_DESCRIPTION "Ethernet Channel Bonding Driver" diff --git a/include/linux/if_bonding.h b/include/linux/if_bonding.h index 84598fa..65c2d24 100644 --- a/include/linux/if_bonding.h +++ b/include/linux/if_bonding.h @@ -85,7 +85,8 @@ /* hashing types */ #define BOND_XMIT_POLICY_LAYER2 0 /* layer 2 (MAC only), default */ -#define BOND_XMIT_POLICY_LAYER34 1 /* layer 3+4 (IP ^ MAC) */ +#define BOND_XMIT_POLICY_LAYER34 1 /* layer 3+4 (IP ^ (TCP || UDP)) */ +#define BOND_XMIT_POLICY_LAYER23 2 /* layer 2+3 (IP ^ MAC) */ typedef struct ifbond { __s32 bond_mode; -- 1.5.3.4.206.g58ba4-dirty ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 8/8] bonding: Fix race at module unload 2007-12-07 7:40 ` [PATCH 7/8] bonding: Add new layer2+3 hash for xor/802.3ad modes Jay Vosburgh @ 2007-12-07 7:40 ` Jay Vosburgh 0 siblings, 0 replies; 10+ messages in thread From: Jay Vosburgh @ 2007-12-07 7:40 UTC (permalink / raw) To: netdev; +Cc: jgarzik, Jay Vosburgh Fixes a race condition in module unload. Without this change, workqueue events may fire while bonding data structures are partially freed but before bond_close() is invoked by unregister_netdevice(). Update version to 3.2.3. Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_main.c | 43 ++++++++++++++++++++------------------- drivers/net/bonding/bonding.h | 2 +- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 08879d5..b0b2603 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -4492,6 +4492,27 @@ static void bond_deinit(struct net_device *bond_dev) #endif } +static void bond_work_cancel_all(struct bonding *bond) +{ + write_lock_bh(&bond->lock); + bond->kill_timers = 1; + write_unlock_bh(&bond->lock); + + if (bond->params.miimon && delayed_work_pending(&bond->mii_work)) + cancel_delayed_work(&bond->mii_work); + + if (bond->params.arp_interval && delayed_work_pending(&bond->arp_work)) + cancel_delayed_work(&bond->arp_work); + + if (bond->params.mode == BOND_MODE_ALB && + delayed_work_pending(&bond->alb_work)) + cancel_delayed_work(&bond->alb_work); + + if (bond->params.mode == BOND_MODE_8023AD && + delayed_work_pending(&bond->ad_work)) + cancel_delayed_work(&bond->ad_work); +} + /* Unregister and free all bond devices. * Caller must hold rtnl_lock. */ @@ -4502,6 +4523,7 @@ static void bond_free_all(void) list_for_each_entry_safe(bond, nxt, &bond_dev_list, bond_list) { struct net_device *bond_dev = bond->dev; + bond_work_cancel_all(bond); bond_mc_list_destroy(bond); /* Release the bonded slaves */ bond_release_all(bond_dev); @@ -4902,27 +4924,6 @@ out_rtnl: return res; } -static void bond_work_cancel_all(struct bonding *bond) -{ - write_lock_bh(&bond->lock); - bond->kill_timers = 1; - write_unlock_bh(&bond->lock); - - if (bond->params.miimon && delayed_work_pending(&bond->mii_work)) - cancel_delayed_work(&bond->mii_work); - - if (bond->params.arp_interval && delayed_work_pending(&bond->arp_work)) - cancel_delayed_work(&bond->arp_work); - - if (bond->params.mode == BOND_MODE_ALB && - delayed_work_pending(&bond->alb_work)) - cancel_delayed_work(&bond->alb_work); - - if (bond->params.mode == BOND_MODE_8023AD && - delayed_work_pending(&bond->ad_work)) - cancel_delayed_work(&bond->ad_work); -} - static int __init bonding_init(void) { int i; diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index ccafc74..e1e4734 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -22,7 +22,7 @@ #include "bond_3ad.h" #include "bond_alb.h" -#define DRV_VERSION "3.2.2" +#define DRV_VERSION "3.2.3" #define DRV_RELDATE "December 6, 2007" #define DRV_NAME "bonding" #define DRV_DESCRIPTION "Ethernet Channel Bonding Driver" -- 1.5.3.4.206.g58ba4-dirty ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/8] bonding: Remove trailing NULs from sysfs interface. 2007-12-07 7:40 ` [PATCH 1/8] bonding: Remove trailing NULs from sysfs interface Jay Vosburgh 2007-12-07 7:40 ` [PATCH 2/8] bonding: Return nothing for not applicable values Jay Vosburgh @ 2007-12-07 20:02 ` Jeff Garzik 1 sibling, 0 replies; 10+ messages in thread From: Jeff Garzik @ 2007-12-07 20:02 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev, Wagner Ferenc Jay Vosburgh wrote: > From: Wagner Ferenc <wferi@niif.hu> > > From: Wagner Ferenc <wferi@niif.hu> > > Also remove trailing spaces from multivalued files. > > This fixes output like for example: > > $ od -c /sys/class/net/bond0/bonding/slaves > 0000000 e t h - l e f t e t h - r i g > 0000020 h t \n \0 > 0000025 > > It mostly entails deleting '+1'-s after sprintf() calls: the return value > of sprintf is the number of characters printed, without the closing NUL, > ie. exactly what the sysfs interface requires. The three multivalue > cases are different, because they also have to swallow back a trailing > space. > > Signed-off-by: Ferenc Wagner <wferi@niif.hu> > Acked-by: Jay Vosburgh <fubar@us.ibm.com> > --- > drivers/net/bonding/bond_sysfs.c | 66 +++++++++++++++++-------------------- > 1 files changed, 30 insertions(+), 36 deletions(-) applied 1-8 to #upstream-fixes Your script is duplicating the "From: " line twice ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2007-12-07 20:02 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-12-07 7:40 [PATCH 0/8] bonding: Several fixes, new hash mode Jay Vosburgh 2007-12-07 7:40 ` [PATCH 1/8] bonding: Remove trailing NULs from sysfs interface Jay Vosburgh 2007-12-07 7:40 ` [PATCH 2/8] bonding: Return nothing for not applicable values Jay Vosburgh 2007-12-07 7:40 ` [PATCH 3/8] bonding: Purely cosmetic: rename a local variable Jay Vosburgh 2007-12-07 7:40 ` [PATCH 4/8] bonding: Coding style: break line after the if condition Jay Vosburgh 2007-12-07 7:40 ` [PATCH 5/8] bonding: Allow setting and querying xmit policy regardless of mode Jay Vosburgh 2007-12-07 7:40 ` [PATCH 6/8] bonding: Fix time comparison Jay Vosburgh 2007-12-07 7:40 ` [PATCH 7/8] bonding: Add new layer2+3 hash for xor/802.3ad modes Jay Vosburgh 2007-12-07 7:40 ` [PATCH 8/8] bonding: Fix race at module unload Jay Vosburgh 2007-12-07 20:02 ` [PATCH 1/8] bonding: Remove trailing NULs from sysfs interface Jeff Garzik
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).