2005-09-22 04:04:04

by Florin Malita

[permalink] [raw]
Subject: [PATCH] channel bonding: add support for device-indexed parameters

While originally I was interested in being able to set a different
primary interface for each bond device (same primary for all bond
devices doesn't make any sense), most parameters deserve the same
treatement.

This patch adds support for device indexed module parameter
arrays instead of the old plain scalars. Mostly module_param
substitutions and parameter parsing logic tweaking.


Signed-off-by: Florin Malita <[email protected]>
---
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -487,6 +487,8 @@
* * Added xmit_hash_policy_layer34()
* - Modified by Jay Vosburgh <[email protected]> to also support mode 4.
* Set version to 2.6.3.
+ * 2005/09/20 - Florin Malita <fmalita at gmail dot com>
+ * - Added support for device-indexed module parameters.
*/

//#define BONDING_DEBUG 1
@@ -545,38 +547,47 @@
#define BOND_LINK_ARP_INTERV 0

static int max_bonds = BOND_DEFAULT_MAX_BONDS;
-static int miimon = BOND_LINK_MON_INTERV;
-static int updelay = 0;
-static int downdelay = 0;
-static int use_carrier = 1;
-static char *mode = NULL;
-static char *primary = NULL;
-static char *lacp_rate = NULL;
-static char *xmit_hash_policy = NULL;
-static int arp_interval = BOND_LINK_ARP_INTERV;
+static int miimon[BOND_MAX_PARMS];
+static int num_miimon;
+static int updelay[BOND_MAX_PARMS];
+static int num_updelay;
+static int downdelay[BOND_MAX_PARMS];
+static int num_downdelay;
+static int use_carrier[BOND_MAX_PARMS];
+static int num_use_carrier;
+static char *mode[BOND_MAX_PARMS];
+static int num_mode;
+static char *primary[BOND_MAX_PARMS];
+static int num_primary;
+static char *lacp_rate[BOND_MAX_PARMS];
+static int num_lacp_rate;
+static char *xmit_hash_policy[BOND_MAX_PARMS];
+static int num_xmit_hash_policy;
+static int arp_interval[BOND_MAX_PARMS];
+static int num_arp_interval;
static char *arp_ip_target[BOND_MAX_ARP_TARGETS] = { NULL, };

-module_param(max_bonds, int, 0);
+module_param(max_bonds, int, S_IRUGO);
MODULE_PARM_DESC(max_bonds, "Max number of bonded devices");
-module_param(miimon, int, 0);
+module_param_array(miimon, int, &num_miimon, S_IRUGO);
MODULE_PARM_DESC(miimon, "Link check interval in milliseconds");
-module_param(updelay, int, 0);
+module_param_array(updelay, int, &num_updelay, S_IRUGO);
MODULE_PARM_DESC(updelay, "Delay before considering link up, in milliseconds");
-module_param(downdelay, int, 0);
+module_param_array(downdelay, int, &num_downdelay, S_IRUGO);
MODULE_PARM_DESC(downdelay, "Delay before considering link down, in milliseconds");
-module_param(use_carrier, int, 0);
+module_param_array(use_carrier, int, &num_use_carrier, S_IRUGO);
MODULE_PARM_DESC(use_carrier, "Use netif_carrier_ok (vs MII ioctls) in miimon; 0 for off, 1 for on (default)");
-module_param(mode, charp, 0);
+module_param_array(mode, charp, &num_mode, S_IRUGO);
MODULE_PARM_DESC(mode, "Mode of operation : 0 for round robin, 1 for active-backup, 2 for xor");
-module_param(primary, charp, 0);
+module_param_array(primary, charp, &num_primary, S_IRUGO);
MODULE_PARM_DESC(primary, "Primary network device to use");
-module_param(lacp_rate, charp, 0);
+module_param_array(lacp_rate, charp, &num_lacp_rate, S_IRUGO);
MODULE_PARM_DESC(lacp_rate, "LACPDU tx rate to request from 802.3ad partner (slow/fast)");
-module_param(xmit_hash_policy, charp, 0);
+module_param_array(xmit_hash_policy, charp, &num_xmit_hash_policy, S_IRUGO);
MODULE_PARM_DESC(xmit_hash_policy, "XOR hashing method : 0 for layer 2 (default), 1 for layer 3+4");
-module_param(arp_interval, int, 0);
+module_param_array(arp_interval, int, &num_arp_interval, S_IRUGO);
MODULE_PARM_DESC(arp_interval, "arp interval in milliseconds");
-module_param_array(arp_ip_target, charp, NULL, 0);
+module_param_array(arp_ip_target, charp, NULL, S_IRUGO);
MODULE_PARM_DESC(arp_ip_target, "arp targets in n.n.n.n form");

/*----------------------------- Global variables ----------------------------*/
@@ -592,9 +603,6 @@ static struct proc_dir_entry *bond_proc_

static u32 arp_target[BOND_MAX_ARP_TARGETS] = { 0, } ;
static int arp_ip_count = 0;
-static int bond_mode = BOND_MODE_ROUNDROBIN;
-static int xmit_hashtype= BOND_XMIT_POLICY_LAYER2;
-static int lacp_fast = 0;
static int app_abi_ver = 0;
static int orig_app_abi_ver = -1; /* This is used to save the first ABI version
* we receive from the application. Once set,
@@ -4714,51 +4722,62 @@ static inline int bond_parse_parm(char *
return -1;
}

-static int bond_check_params(struct bond_params *params)
+static int bond_check_params(struct bond_params *params, int bond)
{
+ params->miimon = (bond < num_miimon) ? miimon[bond] : BOND_LINK_MON_INTERV;
+ params->updelay = (bond < num_updelay) ? updelay[bond] : 0;
+ params->downdelay = (bond < num_downdelay) ? downdelay[bond] : 0;
+ params->use_carrier = (bond < num_use_carrier) ? use_carrier[bond] : 1;
+ params->arp_interval = (bond < num_arp_interval) ? arp_interval[bond] : BOND_LINK_ARP_INTERV;
+
+ params->mode = BOND_MODE_ROUNDROBIN;
+ params->primary[0] = '\0';
+ params->lacp_fast = 0;
+ params->xmit_policy = BOND_XMIT_POLICY_LAYER2;
+
+
/*
* Convert string parameters.
*/
- if (mode) {
- bond_mode = bond_parse_parm(mode, bond_mode_tbl);
- if (bond_mode == -1) {
+ if (bond < num_mode) {
+ params->mode = bond_parse_parm(mode[bond], bond_mode_tbl);
+ if (params->mode == -1) {
printk(KERN_ERR DRV_NAME
": Error: Invalid bonding mode \"%s\"\n",
- mode == NULL ? "NULL" : mode);
+ mode[bond]);
return -EINVAL;
}
}

- if (xmit_hash_policy) {
- if ((bond_mode != BOND_MODE_XOR) &&
- (bond_mode != BOND_MODE_8023AD)) {
+ if (bond < num_xmit_hash_policy) {
+ if ((params->mode != BOND_MODE_XOR) &&
+ (params->mode != BOND_MODE_8023AD)) {
printk(KERN_INFO DRV_NAME
": xor_mode param is irrelevant in mode %s\n",
- bond_mode_name(bond_mode));
+ bond_mode_name(params->mode));
} else {
- xmit_hashtype = bond_parse_parm(xmit_hash_policy,
+ params->xmit_policy = bond_parse_parm(xmit_hash_policy[bond],
xmit_hashtype_tbl);
- if (xmit_hashtype == -1) {
+ if (params->xmit_policy == -1) {
printk(KERN_ERR DRV_NAME
": Error: Invalid xmit_hash_policy \"%s\"\n",
- xmit_hash_policy == NULL ? "NULL" :
- xmit_hash_policy);
+ xmit_hash_policy[bond]);
return -EINVAL;
}
}
}

- if (lacp_rate) {
- if (bond_mode != BOND_MODE_8023AD) {
+ if (bond < num_lacp_rate) {
+ if (params->mode != BOND_MODE_8023AD) {
printk(KERN_INFO DRV_NAME
": lacp_rate param is irrelevant in mode %s\n",
- bond_mode_name(bond_mode));
+ bond_mode_name(params->mode));
} else {
- lacp_fast = bond_parse_parm(lacp_rate, bond_lacp_tbl);
- if (lacp_fast == -1) {
+ params->lacp_fast = bond_parse_parm(lacp_rate[bond], bond_lacp_tbl);
+ if (params->lacp_fast == -1) {
printk(KERN_ERR DRV_NAME
": Error: Invalid lacp rate \"%s\"\n",
- lacp_rate == NULL ? "NULL" : lacp_rate);
+ lacp_rate[bond]);
return -EINVAL;
}
}
@@ -4772,77 +4791,77 @@ static int bond_check_params(struct bond
max_bonds = BOND_DEFAULT_MAX_BONDS;
}

- if (miimon < 0) {
+ if (params->miimon < 0) {
printk(KERN_WARNING DRV_NAME
": Warning: miimon module parameter (%d), "
"not in range 0-%d, so it was reset to %d\n",
- miimon, INT_MAX, BOND_LINK_MON_INTERV);
- miimon = BOND_LINK_MON_INTERV;
+ params->miimon, INT_MAX, BOND_LINK_MON_INTERV);
+ params->miimon = BOND_LINK_MON_INTERV;
}

- if (updelay < 0) {
+ if (params->updelay < 0) {
printk(KERN_WARNING DRV_NAME
": Warning: updelay module parameter (%d), "
"not in range 0-%d, so it was reset to 0\n",
- updelay, INT_MAX);
- updelay = 0;
+ params->updelay, INT_MAX);
+ params->updelay = 0;
}

- if (downdelay < 0) {
+ if (params->downdelay < 0) {
printk(KERN_WARNING DRV_NAME
": Warning: downdelay module parameter (%d), "
"not in range 0-%d, so it was reset to 0\n",
- downdelay, INT_MAX);
- downdelay = 0;
+ params->downdelay, INT_MAX);
+ params->downdelay = 0;
}

- if ((use_carrier != 0) && (use_carrier != 1)) {
+ if ((params->use_carrier != 0) && (params->use_carrier != 1)) {
printk(KERN_WARNING DRV_NAME
": Warning: use_carrier module parameter (%d), "
"not of valid value (0/1), so it was set to 1\n",
- use_carrier);
- use_carrier = 1;
+ params->use_carrier);
+ params->use_carrier = 1;
}

/* reset values for 802.3ad */
- if (bond_mode == BOND_MODE_8023AD) {
- if (!miimon) {
+ if (params->mode == BOND_MODE_8023AD) {
+ if (!params->miimon) {
printk(KERN_WARNING DRV_NAME
": Warning: miimon must be specified, "
"otherwise bonding will not detect link "
"failure, speed and duplex which are "
"essential for 802.3ad operation\n");
printk(KERN_WARNING "Forcing miimon to 100msec\n");
- miimon = 100;
+ params->miimon = 100;
}
}

/* reset values for TLB/ALB */
- if ((bond_mode == BOND_MODE_TLB) ||
- (bond_mode == BOND_MODE_ALB)) {
- if (!miimon) {
+ if ((params->mode == BOND_MODE_TLB) ||
+ (params->mode == BOND_MODE_ALB)) {
+ if (!params->miimon) {
printk(KERN_WARNING DRV_NAME
": Warning: miimon must be specified, "
"otherwise bonding will not detect link "
"failure and link speed which are essential "
"for TLB/ALB load balancing\n");
printk(KERN_WARNING "Forcing miimon to 100msec\n");
- miimon = 100;
+ params->miimon = 100;
}
}

- if (bond_mode == BOND_MODE_ALB) {
+ if (params->mode == BOND_MODE_ALB) {
printk(KERN_NOTICE DRV_NAME
": In ALB mode you might experience client "
"disconnections upon reconnection of a link if the "
"bonding module updelay parameter (%d msec) is "
"incompatible with the forwarding delay time of the "
"switch\n",
- updelay);
+ params->updelay);
}

- if (!miimon) {
- if (updelay || downdelay) {
+ if (!params->miimon) {
+ if (params->updelay || params->downdelay) {
/* just warn the user the up/down delay will have
* no effect since miimon is zero...
*/
@@ -4851,45 +4870,45 @@ static int bond_check_params(struct bond
"and updelay (%d) or downdelay (%d) module "
"parameter is set; updelay and downdelay have "
"no effect unless miimon is set\n",
- updelay, downdelay);
+ params->updelay, params->downdelay);
}
} else {
/* don't allow arp monitoring */
- if (arp_interval) {
+ if (params->arp_interval) {
printk(KERN_WARNING DRV_NAME
": Warning: miimon (%d) and arp_interval (%d) "
"can't be used simultaneously, disabling ARP "
"monitoring\n",
- miimon, arp_interval);
- arp_interval = 0;
+ params->miimon, params->arp_interval);
+ params->arp_interval = 0;
}

- if ((updelay % miimon) != 0) {
+ if ((params->updelay % params->miimon) != 0) {
printk(KERN_WARNING DRV_NAME
": Warning: updelay (%d) is not a multiple "
"of miimon (%d), updelay rounded to %d ms\n",
- updelay, miimon, (updelay / miimon) * miimon);
+ params->updelay, params->miimon, (params->updelay / params->miimon) * params->miimon);
}

- updelay /= miimon;
+ params->updelay /= params->miimon;

- if ((downdelay % miimon) != 0) {
+ if ((params->downdelay % params->miimon) != 0) {
printk(KERN_WARNING DRV_NAME
": Warning: downdelay (%d) is not a multiple "
"of miimon (%d), downdelay rounded to %d ms\n",
- downdelay, miimon,
- (downdelay / miimon) * miimon);
+ params->downdelay, params->miimon,
+ (params->downdelay / params->miimon) * params->miimon);
}

- downdelay /= miimon;
+ params->downdelay /= params->miimon;
}

- if (arp_interval < 0) {
+ if (params->arp_interval < 0) {
printk(KERN_WARNING DRV_NAME
": Warning: arp_interval module parameter (%d) "
", not in range 0-%d, so it was reset to %d\n",
- arp_interval, INT_MAX, BOND_LINK_ARP_INTERV);
- arp_interval = BOND_LINK_ARP_INTERV;
+ params->arp_interval, INT_MAX, BOND_LINK_ARP_INTERV);
+ params->arp_interval = BOND_LINK_ARP_INTERV;
}

for (arp_ip_count = 0;
@@ -4902,33 +4921,33 @@ static int bond_check_params(struct bond
": Warning: bad arp_ip_target module parameter "
"(%s), ARP monitoring will not be performed\n",
arp_ip_target[arp_ip_count]);
- arp_interval = 0;
+ params->arp_interval = 0;
} else {
u32 ip = in_aton(arp_ip_target[arp_ip_count]);
arp_target[arp_ip_count] = ip;
}
}

- if (arp_interval && !arp_ip_count) {
+ if (params->arp_interval && !arp_ip_count) {
/* don't allow arping if no arp_ip_target given... */
printk(KERN_WARNING DRV_NAME
": Warning: arp_interval module parameter (%d) "
"specified without providing an arp_ip_target "
"parameter, arp_interval was reset to 0\n",
- arp_interval);
- arp_interval = 0;
+ params->arp_interval);
+ params->arp_interval = 0;
}

- if (miimon) {
+ if (params->miimon) {
printk(KERN_INFO DRV_NAME
": MII link monitoring set to %d ms\n",
- miimon);
- } else if (arp_interval) {
+ params->miimon);
+ } else if (params->arp_interval) {
int i;

printk(KERN_INFO DRV_NAME
": ARP monitoring set to %d ms with %d target(s):",
- arp_interval, arp_ip_count);
+ params->arp_interval, arp_ip_count);

for (i = 0; i < arp_ip_count; i++)
printk (" %s", arp_ip_target[i]);
@@ -4945,32 +4964,20 @@ static int bond_check_params(struct bond
"otherwise bonding will not detect link failures! see "
"bonding.txt for details.\n");
}
-
- if (primary && !USES_PRIMARY(bond_mode)) {
+
+ if (bond < num_primary) {
/* currently, using a primary only makes sense
* in active backup, TLB or ALB modes
*/
- printk(KERN_WARNING DRV_NAME
- ": Warning: %s primary device specified but has no "
- "effect in %s mode\n",
- primary, bond_mode_name(bond_mode));
- primary = NULL;
- }
-
- /* fill params struct with the proper values */
- params->mode = bond_mode;
- params->xmit_policy = xmit_hashtype;
- params->miimon = miimon;
- params->arp_interval = arp_interval;
- params->updelay = updelay;
- params->downdelay = downdelay;
- params->use_carrier = use_carrier;
- params->lacp_fast = lacp_fast;
- params->primary[0] = 0;
-
- if (primary) {
- strncpy(params->primary, primary, IFNAMSIZ);
- params->primary[IFNAMSIZ - 1] = 0;
+ if (USES_PRIMARY(params->mode)) {
+ strncpy(params->primary, primary[bond], IFNAMSIZ);
+ params->primary[IFNAMSIZ - 1] = 0;
+ } else {
+ printk(KERN_WARNING DRV_NAME
+ ": Warning: %s primary device specified but has no "
+ "effect in %s mode\n",
+ primary[bond], bond_mode_name(params->mode));
+ }
}

memcpy(params->arp_targets, arp_target, sizeof(arp_target));
@@ -4981,16 +4988,12 @@ static int bond_check_params(struct bond
static int __init bonding_init(void)
{
struct bond_params params;
+ struct net_device *bond_dev;
int i;
int res;

printk(KERN_INFO "%s", version);

- res = bond_check_params(&params);
- if (res) {
- return res;
- }
-
rtnl_lock();

#ifdef CONFIG_PROC_FS
@@ -4998,8 +5001,6 @@ static int __init bonding_init(void)
#endif

for (i = 0; i < max_bonds; i++) {
- struct net_device *bond_dev;
-
bond_dev = alloc_netdev(sizeof(struct bonding), "", ether_setup);
if (!bond_dev) {
res = -ENOMEM;
@@ -5008,8 +5009,11 @@ static int __init bonding_init(void)

res = dev_alloc_name(bond_dev, "bond%d");
if (res < 0) {
- free_netdev(bond_dev);
- goto out_err;
+ goto out_dev;
+ }
+
+ if ((res = bond_check_params(&params, i)) < 0){
+ goto out_dev;
}

/* bond_init() must be called after dev_alloc_name() (for the
@@ -5018,8 +5022,7 @@ static int __init bonding_init(void)
*/
res = bond_init(bond_dev, &params);
if (res < 0) {
- free_netdev(bond_dev);
- goto out_err;
+ goto out_dev;
}

SET_MODULE_OWNER(bond_dev);
@@ -5027,8 +5030,7 @@ static int __init bonding_init(void)
res = register_netdevice(bond_dev);
if (res < 0) {
bond_deinit(bond_dev);
- free_netdev(bond_dev);
- goto out_err;
+ goto out_dev;
}
}

@@ -5038,6 +5040,8 @@ static int __init bonding_init(void)

return 0;

+out_dev:
+ free_netdev(bond_dev);
out_err:
/*
* rtnl_unlock() will run netdev_run_todo(), putting the
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -46,6 +46,7 @@
#define DRV_DESCRIPTION "Ethernet Channel Bonding Driver"

#define BOND_MAX_ARP_TARGETS 16
+#define BOND_MAX_PARMS 16

#ifdef BONDING_DEBUG
#define dprintk(fmt, args...) \


2005-09-22 06:03:54

by Jason R. Martin

[permalink] [raw]
Subject: Re: [PATCH] channel bonding: add support for device-indexed parameters

On 9/21/05, Florin Malita <[email protected]> wrote:
> While originally I was interested in being able to set a different
> primary interface for each bond device (same primary for all bond
> devices doesn't make any sense), most parameters deserve the same
> treatement.
>
> This patch adds support for device indexed module parameter
> arrays instead of the old plain scalars. Mostly module_param
> substitutions and parameter parsing logic tweaking.
[snip]

Personally I think working to get the sysfs support finished in
bonding and stop relying on module parameters to configure bonds would
be better, since bonds will truly be independent of each other and be
able to be added and removed on the fly. Having worked with a
previous attempt to set per-bond values through module parameters
(http://marc.theaimsgroup.com/?t=110558187800001&r=1&w=2), it's easy
to get pretty crazy. For example, you can have more than one
arp_ip_target, and they really should be per bond as well, so how do
you divvy those up via module parameters?

Jason

2005-09-22 13:19:26

by Florin Malita

[permalink] [raw]
Subject: Re: [PATCH] channel bonding: add support for device-indexed parameters

On Wed, 21 Sep 2005 23:03:53 -0700
"Jason R. Martin" <[email protected]> wrote:
> Personally I think working to get the sysfs support finished in
> bonding and stop relying on module parameters to configure bonds would
> be better, since bonds will truly be independent of each other and be
> able to be added and removed on the fly. Having worked with a
> previous attempt to set per-bond values through module parameters
> (http://marc.theaimsgroup.com/?t=110558187800001&r=1&w=2), it's easy
> to get pretty crazy.

Agreed - that would be a better configuration interface, but I don't see
why we couldn't support module parameter arrays too. Especially since
the changes are minimal and don't break the ABI/ifenslave
compatibility/etc.

IMHO the "primary" semantics are completely broken right now and this
is a possible fix for it.

> For example, you can have more than one
> arp_ip_target, and they really should be per bond as well, so how do
> you divvy those up via module parameters?

Yup, arp_ip_target is one parameter which doesn't lend itself to this
scheme and this is exactly why the patch doesn't try to fix it.

Florin

2005-09-26 23:58:55

by Jay Vosburgh

[permalink] [raw]
Subject: Re: [PATCH] channel bonding: add support for device-indexed parameters

Florin Malita <[email protected]> wrote:

>On Wed, 21 Sep 2005 23:03:53 -0700
>"Jason R. Martin" <[email protected]> wrote:
>> Personally I think working to get the sysfs support finished in
>> bonding and stop relying on module parameters to configure bonds would
>> be better
[...]
>Agreed - that would be a better configuration interface, but I don't see
>why we couldn't support module parameter arrays too. Especially since
>the changes are minimal and don't break the ABI/ifenslave
>compatibility/etc.
>
>IMHO the "primary" semantics are completely broken right now and this
>is a possible fix for it.

The distro provided network init scripts are, as far as I know,
really the main user of the bonding module parameters. Right now, the
init scripts will generally load the bonding module multiple times when
creating multiple bonds with differing parameters. This works tolerably
well, although I recall that some users have run afoul of Fedora Core
kernels that could or would not load any module multiple times. I don't
know if that's still the case today.

In any event, your patch does not provide enough flexibility to
allow the distro scripts to switch to it (it omits arp_ip_target), so
the init scripts will be unable to switch. Given that, I'm not sure I
see the real value.

Additionally, as Jason mentions, the sysfs API is looming, and
I'd frankly rather have the init scripts switch to sysfs as it provide
greater flexibility (in particular, it removes the dependency on bonding
being compiled as a module).

-J

---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]

2005-09-27 05:23:54

by Florin Malita

[permalink] [raw]
Subject: Re: [PATCH] channel bonding: add support for device-indexed parameters

On Mon, 26 Sep 2005 16:58:22 -0700
Jay Vosburgh <[email protected]> wrote:

> Florin Malita <[email protected]> wrote:
> >IMHO the "primary" semantics are completely broken right now and this
> >is a possible fix for it.
>
> The distro provided network init scripts are, as far as I know,
> really the main user of the bonding module parameters. Right now, the
> init scripts will generally load the bonding module multiple times when
> creating multiple bonds with differing parameters. This works tolerably
> well, although I recall that some users have run afoul of Fedora Core
> kernels that could or would not load any module multiple times. I don't
> know if that's still the case today.

How can you load a module multiple times on _any_ distro?
More specifically, how exactly do you get past this check in module.c:
http://lxr.linux.no/source/kernel/module.c#L1534

if (find_module(mod->name)) {
err = -EEXIST;
goto free_mod;
}

That means there can only be one bonding instance => there's only one
"primary" parameter for all bonding devices => the semantics are broken.

eth0 \ eth2 \
bond0 bond1
eth1 / eth3 /

current semantics: modprobe bonding max_bonds=2 primary=eth0 ...
=> bond0(primary==eth0), bond1(primary==eth0)(!!!)

new/patch semantics: modprobe bonding max_bonds=2 primary=eth0,eth2 ...
=> bond0(primary==eth0), bond1(primary==eth2)

Currently it's impossible to implement multiple
prioritized active/backup bonds - or am I missing something?

> In any event, your patch does not provide enough flexibility to
> allow the distro scripts to switch to it (it omits arp_ip_target), so
> the init scripts will be unable to switch.

The patch is not forcing the scripts to switch since the old syntax/ABI
still works (one reason I didn't touch arp_ip_target was to preserve
that). It's simply providing an additional (saner) approach to bonding
configuration which can easily co-exist with the sysfs solution.

> I'm not sure I see the real value.

I'm not sure I see the real value in bonding _without_ the capabilities
provided by this patch 8)

Not being able to set a (different) preferred
interface/primary for each bond device makes it unacceptable for
deployment in our environment.

Florin

2005-09-27 07:12:24

by Jay Vosburgh

[permalink] [raw]
Subject: Re: [PATCH] channel bonding: add support for device-indexed parameters


Florin Malita <[email protected]> wrote:
[...]
>How can you load a module multiple times on _any_ distro?

modprobe -obond0 bonding mode=your-favorite-mode
modprobe -obond1 bonding mode=some-other-mode

and so on. This is in the modprobe man page, and is described
in the bonding documentation (found in the kernel documentation or at
http://sourceforge.net/projects/bonding). It is admittedly somewhat
grotty, but it works. The current planned API path that allow one
module load (or compiling bonding into the kernel) to create multiple
discrete bonding devices is sysfs.

[...]
>> In any event, your patch does not provide enough flexibility to
>> allow the distro scripts to switch to it (it omits arp_ip_target), so
>> the init scripts will be unable to switch.
>
>The patch is not forcing the scripts to switch since the old syntax/ABI
>still works (one reason I didn't touch arp_ip_target was to preserve
>that). It's simply providing an additional (saner) approach to bonding
>configuration which can easily co-exist with the sysfs solution.

It's not about forcing use; it's about adding an incomplete and
inconsistent feature, and the attendant maintenance and documentation
load.

In general, I can see the value of allowing users to specify all
of the parameters for some number of bonding devices on a single module
load line. However, the syntax provided by your patch does not parse
the arp_ip_target parameter consistently with the other parameters, and
has no backwards-compatible way to add support for all possible bonding
configurations that I can think of right offhand.

I don't see the value in a new module parameter syntax that is
not both backwards compatible and allows for specifying any possible set
of bonding module options.

>Not being able to set a (different) preferred
>interface/primary for each bond device makes it unacceptable for
>deployment in our environment.

How are you configuring bonding? The current SuSE distros, for
example, will do the multiple module load stuff automatically in the
sysconfig scripts. This is described in the current bonding
documentation.

-J

---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]

2005-09-27 13:44:21

by Florin Malita

[permalink] [raw]
Subject: Re: [PATCH] channel bonding: add support for device-indexed parameters

On Tue, 27 Sep 2005 00:11:56 -0700
Jay Vosburgh <[email protected]> wrote:
>
> Florin Malita <[email protected]> wrote:
> [...]
> >How can you load a module multiple times on _any_ distro?
>
> modprobe -obond0 bonding mode=your-favorite-mode
> modprobe -obond1 bonding mode=some-other-mode
>
> and so on. This is in the modprobe man page, and is described
> in the bonding documentation (found in the kernel documentation or at
> http://sourceforge.net/projects/bonding). It is admittedly somewhat
> grotty, but it works.

OK, I see this capability has been in module-init-tools since the 0.8
days. Doesn't apply to any 2.4/modutils based system tough.

>
> >Not being able to set a (different) preferred
> >interface/primary for each bond device makes it unacceptable for
> >deployment in our environment.
>
> How are you configuring bonding? The current SuSE distros, for
> example, will do the multiple module load stuff automatically in the
> sysconfig scripts. This is described in the current bonding
> documentation.

Our systems are RHEL3 based so unfortunately the naming trick above
doesn't work.

But it does work on RHEL4 so admittedly, having this workaround
available for recent distros removes the urgency for a fix.

Thanks
Florin

2005-09-27 13:54:56

by Eric Paris

[permalink] [raw]
Subject: Re: [Bonding-devel] Re: [PATCH] channel bonding: add support for device-indexed parameters

It dos work with RHEL3. In modules.conf you just need

alias bond0 bonding
options bond0 -o bonding0
alias bond1 bonding
options bond1 -o bonding1

You can add your mode and mii_mon and such on the options lines. It
does work I've used it.

-Eric


On Tue, 2005-09-27 at 09:40 -0400, Florin Malita wrote:
> On Tue, 27 Sep 2005 00:11:56 -0700
> Jay Vosburgh <[email protected]> wrote:
> >
> > Florin Malita <[email protected]> wrote:
> > [...]
> > >How can you load a module multiple times on _any_ distro?
> >
> > modprobe -obond0 bonding mode=your-favorite-mode
> > modprobe -obond1 bonding mode=some-other-mode
> >
> > and so on. This is in the modprobe man page, and is described
> > in the bonding documentation (found in the kernel documentation or at
> > http://sourceforge.net/projects/bonding). It is admittedly somewhat
> > grotty, but it works.
>
> OK, I see this capability has been in module-init-tools since the 0.8
> days. Doesn't apply to any 2.4/modutils based system tough.
>
> >
> > >Not being able to set a (different) preferred
> > >interface/primary for each bond device makes it unacceptable for
> > >deployment in our environment.
> >
> > How are you configuring bonding? The current SuSE distros, for
> > example, will do the multiple module load stuff automatically in the
> > sysconfig scripts. This is described in the current bonding
> > documentation.
>
> Our systems are RHEL3 based so unfortunately the naming trick above
> doesn't work.
>
> But it does work on RHEL4 so admittedly, having this workaround
> available for recent distros removes the urgency for a fix.
>
> Thanks
> Florin
>
>
> -------------------------------------------------------
> SF.Net email is sponsored by:
> Tame your development challenges with Apache's Geronimo App Server.
> Download it for free - -and be entered to win a 42" plasma tv or your very
> own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
> _______________________________________________
> Bonding-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/bonding-devel

2005-09-27 14:13:50

by Florin Malita

[permalink] [raw]
Subject: Re: [Bonding-devel] Re: [PATCH] channel bonding: add support for device-indexed parameters

On Tue, 27 Sep 2005 09:54:29 -0400
Eric Paris <[email protected]> wrote:

> It dos work with RHEL3. In modules.conf you just need
[...]
> You can add your mode and mii_mon and such on the options lines. It
> does work I've used it.

Verified - I completely missed "-o" functionality in insmod & friends.

Sorry for the noise, I'll crawl back under my rock now :)