2005-01-13 02:02:35

by Radheka Godse

[permalink] [raw]
Subject: [Bonding-devel] [PATCH] Support to configure multiple bonds with different module params

The attached patches add support to the channel bonding module for configuring
multiple bonds at load time.
This eliminates the need to reload the module for each new
differently-configured?bond. A maximum of 16 bonds is supported.

This patch has been peer reviewed by our Linux software engineering team,
and the fix has been verified by our test labs.

Summary of changes made from 2.6.1 to 2.7.0?:
?o?Removed?most global variables.
?o Use module_param_array() to define/set defaults for module params
?o Added new module param arp_ip_count to specify how many
?? ip targets in the arp_ip_target array belong to each bond.
? ?If arp_ip_count is not specified, to preserve backward
? ?compatibility all arp targets go to the first bond.?
?o Modified bonding_init() to use sub-functions to:
?? ?? - parse and separate arp parameters
??? ? - fill insmod parameters
- sanitize (and enforce defaults if needed) on parameters
????? - create and configure bond with specified parameters
?o Updated bonding.txt documentation and added examples to configure
?? bonds with different parameters.
o Also this patch has initial plumbing work to prepare for adding
? sysfs interface in future (i.e. next set of patches).

Status: Tested on 2.6.11-rc1
Signed-off-by: Radheka Godse [email protected]

Note: bond-patch-2.7.0-bonding.txt must be applied last!


Attachments:
bond-patch-2.7.0 (42.53 kB)
bond-patch-2.7.0
bond-patch-2.7.0-bonding.txt (12.49 kB)
bond-patch-2.7.0-bonding.txt
Download all attachments

2005-01-13 06:25:53

by Willy Tarreau

[permalink] [raw]
Subject: Re: [Bonding-devel] [PATCH] Support to configure multiple bonds with different module params

Hi Radheka,

On Thu, Jan 13, 2005 at 05:46:34PM -0800, Radheka Godse wrote:
> The attached patches add support to the channel bonding module for
> configuring multiple bonds at load time.
> This eliminates the need to reload the module for each new
> differently-configured?bond. A maximum of 16 bonds is supported.

This is very good news. I've been hoping for this for a long time (since 2.2)
but never found time to implement it. I might try to backport this to 2.4 if
I find some time.

I just have a few comments on the code, which are not problems at all, but
which could be cleaned up before merging :
- there are a few changes that you made for debugging purpose that were
left in (see below)
- some tabs have been randomly replaced with spaces, possibly because of
some copy-paste (see below too). We often try to avoid this because it
later annoys people who post new patches. I think that the simplest
way to fix this would be to replace leading spaces with tabs (vi/sed)
and visually check for the resulting indentation.

Overall, I'm very interested in this work. And BTW, I really appreciate
that you took the time to fix quite a number of typos in the documentation.

Thanks,
Willy

PS: see below for comments.

> #ifdef BONDING_DEBUG
> #define dprintk(fmt, args...) \
> - printk(KERN_DEBUG \
> + printk(KERN_ERR \
> DRV_NAME ": %s() %d: " fmt, __FUNCTION__, __LINE__ , ## args )

here : KERN_ERR ?

> @@ -203,6 +210,8 @@
> struct bond_params params;
> struct list_head vlan_list;
> struct vlan_group *vlgrp;
> + u32 my_ip;
> + struct kobject kobj;

here: mixed spaces and tabs

> +#define BOND_PARAM_STR(X, S, D) \
> + static char __initdata *X[BOND_MAX_UNITS +1] = BOND_PARAM_INIT(D); \
> + static int num_##X; \
> + module_param_array(X, charp, &num_##X, 0); \
> + MODULE_PARM_DESC(X, S);

here: mixed spaces and tabs

if (bond->slave_cnt == 0) {
> + dprintk("last slave removed\n");
> /* if the last slave was removed, zero the mac address

here: mixed spaces and tabs

> @@ -3146,6 +3186,8 @@
> {
> struct bonding *bond = seq->private;
> struct slave *curr;
> + int i;
> + u32 target;

here: mixed spaces and tabs

>
> read_lock(&bond->curr_slave_lock);
> curr = bond->curr_active_slave;
> @@ -3170,6 +3212,24 @@
> seq_printf(seq, "Down Delay (ms): %d\n",
> bond->params.downdelay * bond->params.miimon);
>
> +
> + // ARP information
> + if(bond->params.arp_interval > 0)
> + {

here: mixed spaces and tabs

> - dprintk("bond_ioctl: master=%s, cmd=%d\n",
> - bond_dev->name, cmd);
> +// dprintk("bond_ioctl: master=%s, cmd=%d\n",
> +// bond_dev->name, cmd);

here : debug code commented out. This was already debug anyway. If it's too
verbose, perhaps it should be removed ?

> + struct bonding *bond;
> + bond = bond_dev->priv;

here: mixed spaces and tabs

> + struct net_device *bond_dev;
> + int res;

here: mixed spaces and tabs

> +
> + bond_dev = alloc_netdev(sizeof(struct bonding), name, ether_setup);
> + if (!bond_dev) {

here: mixed spaces and tabs

> + printk(KERN_ERR "eek! can't alloc netdev!\n");
> + return -ENOMEM;
> + }

here: mixed spaces and tabs

> +
> + /* bond_init() must be called after dev_alloc_name() (for the
> + * /proc files), but before register_netdevice(), because we
> + * need to set function pointers.
> + */
> +
> + res = bond_init(bond_dev, params);
> + if (res < 0) {
> + free_netdev(bond_dev);
> + return res;
> + }
> +

here: mixed spaces and tabs

> + SET_MODULE_OWNER(bond_dev);
> +
> + res = register_netdevice(bond_dev);
> + if (res < 0) {
> + bond_deinit(bond_dev);
> + free_netdev(bond_dev);
> + return res;
> + }

here: mixed spaces and tabs

> + if (newbond) *newbond = bond_dev->priv;
> + return res;
> +}

here: mixed spaces and tabs

> + {
> + //printk(KERN_ERR"Freeing bond %s\n", bond->dev->name);
> + bond_destroy_dev(bond->dev);
> + }

here: mixed spaces and tabs

> +
> + // set input values
> + if(miimon[position] != BOND_LINK_MON_INTERV)
> + {

here: mixed spaces and tabs

> + if(use_carrier[position] != BOND_DEF_USE_CARRIER)
> + params->use_carrier = use_carrier[position];
> + if(updelay[position] != BOND_DEF_DELAY)
> + params->updelay = updelay[position];
> + if(downdelay[position] != BOND_DEF_DELAY)
> + params->downdelay = downdelay[position];

here: mixed spaces and tabs

> - There is no limit.
> + Up to 16 max bond devices can be created and configured
> + by specifying commandline options to modprobe.

here: mixed spaces and tabs

That's all.