PATCH Re: [Bonding-devel] Re: Bonding driver unreliable under high CPUload

PATCH Re: [Bonding-devel] Re: Bonding driver unreliable under high CPUload

[Jay Vosburgh]

[-- Attachment #1: Type: text/plain, Size: 278 bytes --]



      Ok, here is a patch for bonding against 2.4.20-pre7.  This gets rid
of the MII_whatever macros, wraps ioctl() to make it less bad, and replaces
a couple of magic numbers with friendly labels.

      Comments?

      -J

(See attached file: bonding-2.4.20-pre7-20020918)

[-- Attachment #2: bonding-2.4.20-pre7-20020918 --]
[-- Type: application/octet-stream, Size: 6302 bytes --]

diff -urN linux-2.4.20-pre7-virgin/drivers/net/bonding.c linux-2.4.20-pre7-bond-0918/drivers/net/bonding.c
--- linux-2.4.20-pre7-virgin/drivers/net/bonding.c	Wed Sep 18 16:55:23 2002
+++ linux-2.4.20-pre7-bond-0918/drivers/net/bonding.c	Wed Sep 18 16:55:34 2002
@@ -186,6 +186,11 @@
  *       also added text to distinguish type of load balancing (rr or xor)
  *     - change arp_ip_target module param from "1-12s" (array of 12 ptrs)
  *       to "s" (a single ptr)
+ *
+ * 2002/09/18 - Jay Vosburgh <fubar at us dot ibm dot com>
+ *     - Fixed up bond_check_dev_link() (and callers): removed some magic
+ *	 numbers, banished local MII_ defines, wrapped ioctl calls to
+ *	 prevent EFAULT errors
  */
 
 #include <linux/config.h>
@@ -229,15 +234,6 @@
 #define BOND_LINK_MON_INTERV	0
 #endif
 
-#undef  MII_LINK_UP
-#define MII_LINK_UP	0x04
-
-#undef  MII_ENDOF_NWAY
-#define MII_ENDOF_NWAY	0x20
-
-#undef  MII_LINK_READY
-#define MII_LINK_READY	(MII_LINK_UP)
-
 #ifndef BOND_LINK_ARP_INTERV
 #define BOND_LINK_ARP_INTERV	0
 #endif
@@ -387,13 +383,25 @@
 	return slave;
 }
 
+/*
+ * Less bad way to call ioctl from within the kernel; this needs to be
+ * done some other way to get the call out of interrupt context.
+ * Needs "ioctl" variable to be supplied by calling context.
+ */
+#define IOCTL(dev, arg, cmd) ({		\
+	int ret;			\
+	mm_segment_t fs = get_fs();	\
+	set_fs(get_ds());		\
+	ret = ioctl(dev, arg, cmd);	\
+	set_fs(fs);			\
+	ret; })
+
 /* 
- * if <dev> supports MII link status reporting, check its link
- * and report it as a bit field in a short int :
- *   - 0x04 means link is up,
- *   - 0x20 means end of autonegociation
- * If the device doesn't support MII, then we only report 0x24,
- * meaning that the link is up and running since we can't check it.
+ * if <dev> supports MII link status reporting, check its link status.
+ *
+ * Return either BMSR_LSTATUS, meaning that the link is up (or we
+ * can't tell and just pretend it is), or 0, meaning that the link is
+ * down.
  */
 static u16 bond_check_dev_link(struct net_device *dev)
 {
@@ -402,7 +410,8 @@
 	struct mii_ioctl_data *mii;
 	struct ethtool_value etool;
 
-	if ((ioctl = dev->do_ioctl) != NULL)  { /* ioctl to access MII */
+	ioctl = dev->do_ioctl;
+	if (ioctl) {
 		/* TODO: set pointer to correct ioctl on a per team member */
 		/*       bases to make this more efficient. that is, once  */
 		/*       we determine the correct ioctl, we will always    */
@@ -416,9 +425,9 @@
 		/* effect...                                               */
 	        etool.cmd = ETHTOOL_GLINK;
 	        ifr.ifr_data = (char*)&etool;
-		if (ioctl(dev, &ifr, SIOCETHTOOL) == 0) {
+		if (IOCTL(dev, &ifr, SIOCETHTOOL) == 0) {
 			if (etool.data == 1) {
-				return(MII_LINK_READY);
+				return BMSR_LSTATUS;
 			} 
 			else { 
 				return(0);
@@ -432,21 +441,17 @@
 
 		/* Yes, the mii is overlaid on the ifreq.ifr_ifru */
 		mii = (struct mii_ioctl_data *)&ifr.ifr_data;
-		if (ioctl(dev, &ifr, SIOCGMIIPHY) != 0) {
-			return MII_LINK_READY;	 /* can't tell */
+		if (IOCTL(dev, &ifr, SIOCGMIIPHY) != 0) {
+			return BMSR_LSTATUS;	 /* can't tell */
 		}
 
-		mii->reg_num = 1;
-		if (ioctl(dev, &ifr, SIOCGMIIREG) == 0) {
-			/*
-			 * mii->val_out contains MII reg 1, BMSR
-			 * 0x0004 means link established
-			 */
-			return mii->val_out;
+		mii->reg_num = MII_BMSR;
+		if (IOCTL(dev, &ifr, SIOCGMIIREG) == 0) {
+			return mii->val_out & BMSR_LSTATUS;
 		}
 
 	}
-	return MII_LINK_READY;  /* spoof link up ( we can't check it) */
+	return BMSR_LSTATUS;  /* spoof link up ( we can't check it) */
 }
 
 static u16 bond_check_mii_link(bonding_t *bond)
@@ -460,7 +465,7 @@
 	read_unlock(&bond->ptrlock);
 	read_unlock_irqrestore(&bond->lock, flags);
 
-	return (has_active_interface ? MII_LINK_READY : 0);
+	return (has_active_interface ? BMSR_LSTATUS : 0);
 }
 
 static int bond_open(struct net_device *dev)
@@ -798,8 +803,8 @@
 	new_slave->link_failure_count = 0;
 
 	/* check for initial state */
-	if ((miimon <= 0) || ((bond_check_dev_link(slave_dev) & MII_LINK_READY)
-		 == MII_LINK_READY)) {
+	if ((miimon <= 0) ||
+	    (bond_check_dev_link(slave_dev) == BMSR_LSTATUS)) {
 #ifdef BONDING_DEBUG
 		printk(KERN_CRIT "Initial state of slave_dev is BOND_LINK_UP\n");
 #endif
@@ -1221,7 +1226,7 @@
 
 		switch (slave->link) {
 		case BOND_LINK_UP:	/* the link was up */
-			if ((link_state & MII_LINK_UP) == MII_LINK_UP) {
+			if (link_state == BMSR_LSTATUS) {
 				/* link stays up, tell that this one
 				   is immediately available */
 				if (IS_UP(dev) && (mindelay > -2)) {
@@ -1257,7 +1262,7 @@
 			   ensure proper action to be taken
 			*/
 		case BOND_LINK_FAIL:	/* the link has just gone down */
-			if ((link_state & MII_LINK_UP) == 0) {
+			if (link_state != BMSR_LSTATUS) {
 				/* link stays down */
 				if (slave->delay <= 0) {
 					/* link down for too long time */
@@ -1286,7 +1291,7 @@
 				} else {
 					slave->delay--;
 				}
-			} else if ((link_state & MII_LINK_READY) == MII_LINK_READY) {
+			} else {
 				/* link up again */
 				slave->link  = BOND_LINK_UP;
 				printk(KERN_INFO
@@ -1305,7 +1310,7 @@
 			}
 			break;
 		case BOND_LINK_DOWN:	/* the link was down */
-			if ((link_state & MII_LINK_READY) != MII_LINK_READY) {
+			if (link_state != BMSR_LSTATUS) {
 				/* the link stays down, nothing more to do */
 				break;
 			} else {	/* link going up */
@@ -1327,7 +1332,7 @@
 			   case there's something to do.
 			*/
 		case BOND_LINK_BACK:	/* the link has just come back */
-			if ((link_state & MII_LINK_UP) == 0) {
+			if (link_state != BMSR_LSTATUS) {
 				/* link down again */
 				slave->link  = BOND_LINK_DOWN;
 				printk(KERN_INFO
@@ -1336,8 +1341,7 @@
 					master->name,
 					(updelay - slave->delay) * miimon,
 					dev->name);
-			}
-			else if ((link_state & MII_LINK_READY) == MII_LINK_READY) {
+			} else {
 				/* link stays up */
 				if (slave->delay == 0) {
 					/* now the link has been up for long time enough */
@@ -2111,7 +2115,7 @@
 
 		len += sprintf(buf + len, "MII Status: ");
 		len += sprintf(buf + len, 
-				link == MII_LINK_READY ? "up\n" : "down\n");
+				link == BMSR_LSTATUS ? "up\n" : "down\n");
 		len += sprintf(buf + len, "MII Polling Interval (ms): %d\n", 
 				miimon);
 		len += sprintf(buf + len, "Up Delay (ms): %d\n", updelay);

Re: PATCH Re: [Bonding-devel] Re: Bonding driver unreliable under high CPUload

[Jeff Garzik]

Jay Vosburgh wrote:
> 
>       Ok, here is a patch for bonding against 2.4.20-pre7.  This gets rid
> of the MII_whatever macros, wraps ioctl() to make it less bad, and replaces
> a couple of magic numbers with friendly labels.
> 
>       Comments?


The patch looks ok but doesn't address the core problem that the link 
state is checked in interrupt context.

	Jeff

Re: PATCH Re: [Bonding-devel] Re: Bonding driver unreliable under high CPUload

[Jay Vosburgh]

      No, it doesn't.  This is a mere band aid until somebody can get time
to do it right.  It's "less bad."

      -J


Jeff Garzik <jgarzik@mandrakesoft.com> on 09/18/2002 05:48:24 PM

To:    Jay Vosburgh/Beaverton/IBM@IBMUS
cc:    Andrew Morton <akpm@digeo.com>, "Cureington, Tony"
       <tony.cureington@hp.com>, Pascal Brisset
       <pascal.brisset-ml@wanadoo.fr>, bonding-devel@lists.sourceforge.net,
       netdev@oss.sgi.com
Subject:    Re: PATCH Re: [Bonding-devel] Re: Bonding driver unreliable
       under high CPUload



Jay Vosburgh wrote:
>
>       Ok, here is a patch for bonding against 2.4.20-pre7.  This gets rid
> of the MII_whatever macros, wraps ioctl() to make it less bad, and
replaces
> a couple of magic numbers with friendly labels.
>
>       Comments?


The patch looks ok but doesn't address the core problem that the link
state is checked in interrupt context.

 Jeff

Re: PATCH Re: [Bonding-devel] Re: Bonding driver unreliable under high CPUload

[Jeff Garzik]

Jay Vosburgh wrote:
> 
>       No, it doesn't.  This is a mere band aid until somebody can get time
> to do it right.  It's "less bad."


Oh, well, in that case, patch approved :)

I'll apply it, unless you prefer to send it through DaveM or somebody else.

	Jeff