Re: [PATCH 2.6.12-rc2 0/17] bonding: sysfs interface...

Re: [PATCH 2.6.12-rc2 0/17] bonding: sysfs interface...

[Stephen Hemminger]

On Fri, 8 Apr 2005 16:15:24 -0700 (PDT)
Radheka Godse <radheka.godse@intel.com> wrote:

> 
> This patch set implements the sysfs interface for bonding and has a couple 
> bug fixes. We have done a full test pass and consider these patches ready for 
> inclusion upstream. We have submitted them to bonding-devel several 
> times and have incorporated feedback and fixes all known issues.
> 
> The interface is pretty simple. Here are some notes on how it could be 
> used:
> 
>   The file /sys/class/net/bonding_masters contains the names of all the
> active bonds. To add or remove bonds, write a whitespace-delimited list of
> interface names to this file.  For example:
>  	echo "bond1 bond2 bond3" > /sys/class/net/bonding_masters
>  		will create three bonds with the given names.  If any other
>   		bonds exist, they will be deleted.
>   	echo "bond0 bond2 bond3" > /sys/class/net/bonding_masters
>   		would then create bond0 and remove bond1.
> 
>   For each bond, there is a directory /sys/class/net/<bondname>/bonding.
> In this directory are a number of files which control the bond.  The names 
> of these files match the existing module parameters and do the same things.
> 
>   The slaves file contains a whitespace-delimited list of interface names,
>   which are slaves to the bond.  This file behaves much the same as the
>   "bonding_masters" file; just write a list of your desired interfaces to 
> this file.
> 
>   Likewise, the arp_targets file contains a whitespace-delimited list of IP
>   addresses and should be written to in a similar fashion.
> 
>   The other files contain single values(numeric and/or mnemonic), except 
> for stat, which duplicates the slave information in the proc file.
> 
>   Some caveats:
>    - slaves can only be assigned when the interface is up
>    - mode can only be changed when the interface is down
> 
>   Warnings and status messages will be logged and can be viewed with dmesg.
> 
>   Example:
>   modprobe bonding
>   echo "bond0 bond1" > /sys/class/net/bonding_masters
>   echo "6" > /sys/class/net/bond0/bonding/mode
>   echo "1000" > /sys/class/net/bond0/bonding/miimon
>   ifconfig bond0 192.168.0.1
>   echo "eth0 eth1" > /sys/class/net/bond0/bonding/slaves
>   # bond0 is now ready to use
>   echo "1" > /sys/class/net/bond1/bonding/mode
>   # ... and so on for bond1
> 
> 
>   These patches apply cleanly to kernel 2.6.12-rc2.
> 
> - Mitch Williams
> - Radheka Godse

I'm not adverse to sysfs per say, but you are using like /proc with multiple values
per file.  This is not what the original design was.

Examples:

+static ssize_t bonding_show_stat(struct class_device *cd, char *buf)
+{
+	struct slave *curr;
+	int i, len = 0;
+	struct bonding *bond = to_bond(cd);
+
+	down_read(&(bonding_rwsem));
+	read_lock_bh(&bond->lock);
+	bond_for_each_slave(bond, curr, i) {
+		len += sprintf(buf + len, "\nSlave Interface: %s\n",
+			       curr->dev->name);
+		len += sprintf(buf + len, "MII Status: %s\n",
+			       (curr->link == BOND_LINK_UP) ? "up" : "down");
+		len += sprintf(buf + len, "Link Failure Count: %d\n",
+			       curr->link_failure_count);
+
+		len += sprintf(buf + len,
+			       "Permanent HW addr: %02x:%02x:%02x:%02x:%02x:%02x\n",
+			       curr->perm_hwaddr[0], curr->perm_hwaddr[1],
+			       curr->perm_hwaddr[2], curr->perm_hwaddr[3],
+			       curr->perm_hwaddr[4], curr->perm_hwaddr[5]);
+
+		if (bond->params.mode == BOND_MODE_8023AD) {
+			const struct aggregator *agg =
+				SLAVE_AD_INFO(curr).port.aggregator;
+
+			if (agg) {
+				len += sprintf(buf + len, "Aggregator ID: %d\n",
+					       agg->aggregator_identifier);
+			} else {
+				len += sprintf(buf + len,
+					       "Aggregator ID: N/A\n");
+			}
+		}

You need to break this up into symlinks and individual files with single value.
Look at bridging as an example.


Also: how do you intend to use the ABI version exported? Unless you have  a clear
plan, it is frowned upon to drop random version numbers around hoping to be able
to have compatibility.

Re: [PATCH 2.6.12-rc2 0/17] bonding: sysfs interface...

[Jay Vosburgh]

Stephen Hemminger <shemminger@osdl.org> wrote:

[...]
>Also: how do you intend to use the ABI version exported? Unless you have  a clear
>plan, it is frowned upon to drop random version numbers around hoping to be able
>to have compatibility.

	If by "ABI version exported," you're referring to this (and the
associated calls):

+int bond_check_abi_ver(void)
+{
+	int retval = 1;
+ +	if (orig_app_abi_ver == -1) {
+		orig_app_abi_ver  = BOND_ABI_VERSION;
+		app_abi_ver = BOND_ABI_VERSION;
+	}
+	else {
+		if (app_abi_ver == 0)
+			retval = 0;
+	}
+
+	return retval;
+}
+

	This enforces the already existing ABI version control stuff in
bonding (which deals with different versions of the ifenslave control
program).  This should preserve the existing ifenslave (ioctl based)
ABI.  I've only skimmed the patch, so I haven't tested it out as yet,
but at first glance it looks ok.

	Or did you mean something else?

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 2.6.12-rc2 0/17] bonding: sysfs interface...

[John W. Linville]

On Fri, Apr 08, 2005 at 04:15:24PM -0700, Radheka Godse wrote:
> 
> This patch set implements the sysfs interface for bonding and has a couple 
> bug fixes. We have done a full test pass and consider these patches ready 
> for inclusion upstream. We have submitted them to bonding-devel several 
> times and have incorporated feedback and fixes all known issues.

I, for one, view these patches as very welcome.  I intend to comment
further later, but for now...

FC3 users are invited to use the test kernels here:

	http://people.redhat.com/linville/kernels/fc3/

I have incorporated the bonding driver from 2.6.12-rc2 plus the bonding
sysfs patches (along with some unrelated patches) to the current FC3
kernel sources.  If you are an FC3 user and you want to experiment w/
these changes, this is the easiest way for you to do so.

I'd love to hear feedback, either on the lists or directly to this
address.

Thanks,

John
-- 
John W. Linville
linville@redhat.com

[PATCH 2.6.12-rc2 0/17] bonding: sysfs interface...

[Radheka Godse]

This patch set implements the sysfs interface for bonding and has a couple 
bug fixes. We have done a full test pass and consider these patches ready for 
inclusion upstream. We have submitted them to bonding-devel several 
times and have incorporated feedback and fixes all known issues.

The interface is pretty simple. Here are some notes on how it could be 
used:

  The file /sys/class/net/bonding_masters contains the names of all the
active bonds. To add or remove bonds, write a whitespace-delimited list of
interface names to this file.  For example:
 	echo "bond1 bond2 bond3" > /sys/class/net/bonding_masters
 		will create three bonds with the given names.  If any other
  		bonds exist, they will be deleted.
  	echo "bond0 bond2 bond3" > /sys/class/net/bonding_masters
  		would then create bond0 and remove bond1.

  For each bond, there is a directory /sys/class/net/<bondname>/bonding.
In this directory are a number of files which control the bond.  The names 
of these files match the existing module parameters and do the same things.

  The slaves file contains a whitespace-delimited list of interface names,
  which are slaves to the bond.  This file behaves much the same as the
  "bonding_masters" file; just write a list of your desired interfaces to 
this file.

  Likewise, the arp_targets file contains a whitespace-delimited list of IP
  addresses and should be written to in a similar fashion.

  The other files contain single values(numeric and/or mnemonic), except 
for stat, which duplicates the slave information in the proc file.

  Some caveats:
   - slaves can only be assigned when the interface is up
   - mode can only be changed when the interface is down

  Warnings and status messages will be logged and can be viewed with dmesg.

  Example:
  modprobe bonding
  echo "bond0 bond1" > /sys/class/net/bonding_masters
  echo "6" > /sys/class/net/bond0/bonding/mode
  echo "1000" > /sys/class/net/bond0/bonding/miimon
  ifconfig bond0 192.168.0.1
  echo "eth0 eth1" > /sys/class/net/bond0/bonding/slaves
  # bond0 is now ready to use
  echo "1" > /sys/class/net/bond1/bonding/mode
  # ... and so on for bond1


  These patches apply cleanly to kernel 2.6.12-rc2.

- Mitch Williams
- Radheka Godse