[patch 4/10] s390: network driver.

[patch 4/10] s390: network driver.

[Martin Schwidefsky]

[patch 4/10] s390: network driver.

From: Thomas Spatzier <tspat@de.ibm.com>
From: Peter Tiedemann <ptiedem@de.ibm.com>

network driver changes:
 - qeth: return -EINVAL if an skb is too large.
 - qeth: don't call netif_stop_queue after cable pull. Drop the
   packets instead.
 - qeth: fix race between SET_IP and SET_MC kernel thread by removing
   SET_MC thread and let the SET_IP thread do multicast requests as well.
 - qeth: make it compile without CONFIG_VLAN.
 - ctc: avoid compiler warnings.
 - lcs: write package sequence number to skb->cb.

Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

diffstat:
 drivers/s390/net/ctcmain.c   |   22 ++-
 drivers/s390/net/lcs.c       |    6 -
 drivers/s390/net/lcs.h       |    3 
 drivers/s390/net/qeth.h      |    8 -
 drivers/s390/net/qeth_main.c |  253 +++++++++++++++++++++----------------------
 5 files changed, 154 insertions(+), 138 deletions(-)

diff -urN linux-2.6/drivers/s390/net/ctcmain.c linux-2.6-patched/drivers/s390/net/ctcmain.c
--- linux-2.6/drivers/s390/net/ctcmain.c	2004-11-11 15:06:39.000000000 +0100
+++ linux-2.6-patched/drivers/s390/net/ctcmain.c	2004-11-11 15:06:56.000000000 +0100
@@ -1,5 +1,5 @@
 /*
- * $Id: ctcmain.c,v 1.63 2004/07/28 12:27:54 ptiedem Exp $
+ * $Id: ctcmain.c,v 1.65 2004/10/27 09:12:48 mschwide Exp $
  *
  * CTC / ESCON network driver
  *
@@ -36,7 +36,7 @@
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  *
- * RELEASE-TAG: CTC/ESCON network driver $Revision: 1.63 $
+ * RELEASE-TAG: CTC/ESCON network driver $Revision: 1.65 $
  *
  */
 \f
@@ -320,7 +320,7 @@
 print_banner(void)
 {
 	static int printed = 0;
-	char vbuf[] = "$Revision: 1.63 $";
+	char vbuf[] = "$Revision: 1.65 $";
 	char *version = vbuf;
 
 	if (printed)
@@ -1224,7 +1224,9 @@
 	fsm_deltimer(&ch->timer);
 	fsm_addtimer(&ch->timer, CTC_TIMEOUT_5SEC, CH_EVENT_TIMER, ch);
 	fsm_newstate(fi, CH_STATE_SETUPWAIT);
-	if (event == CH_EVENT_TIMER)
+	saveflags = 0;	/* avoids compiler warning with
+			   spin_unlock_irqrestore */
+	if (event == CH_EVENT_TIMER)	// only for timer not yet locked
 		spin_lock_irqsave(get_ccwdev_lock(ch->cdev), saveflags);
 	rc = ccw_device_start(ch->cdev, &ch->ccw[6], (unsigned long) ch, 0xff, 0);
 	if (event == CH_EVENT_TIMER)
@@ -1335,7 +1337,9 @@
 	DBF_TEXT(trace, 3, __FUNCTION__);
 	fsm_deltimer(&ch->timer);
 	fsm_addtimer(&ch->timer, CTC_TIMEOUT_5SEC, CH_EVENT_TIMER, ch);
-	if (event == CH_EVENT_STOP)
+	saveflags = 0;	/* avoids comp warning with
+			   spin_unlock_irqrestore */
+	if (event == CH_EVENT_STOP)	// only for STOP not yet locked
 		spin_lock_irqsave(get_ccwdev_lock(ch->cdev), saveflags);
 	oldstate = fsm_getstate(fi);
 	fsm_newstate(fi, CH_STATE_TERM);
@@ -1508,7 +1512,9 @@
 	fsm_addtimer(&ch->timer, CTC_TIMEOUT_5SEC, CH_EVENT_TIMER, ch);
 	oldstate = fsm_getstate(fi);
 	fsm_newstate(fi, CH_STATE_STARTWAIT);
-	if (event == CH_EVENT_TIMER)
+	saveflags = 0;	/* avoids compiler warning with
+			   spin_unlock_irqrestore */
+	if (event == CH_EVENT_TIMER)	// only for timer not yet locked
 		spin_lock_irqsave(get_ccwdev_lock(ch->cdev), saveflags);
 	rc = ccw_device_halt(ch->cdev, (unsigned long) ch);
 	if (event == CH_EVENT_TIMER)
@@ -1674,7 +1680,9 @@
 				return;
 			}
 			fsm_addtimer(&ch->timer, 1000, CH_EVENT_TIMER, ch);
-			if (event == CH_EVENT_TIMER)
+			saveflags = 0;	/* avoids compiler warning with
+					   spin_unlock_irqrestore */
+			if (event == CH_EVENT_TIMER) // only for TIMER not yet locked
 				spin_lock_irqsave(get_ccwdev_lock(ch->cdev),
 						  saveflags);
 			rc = ccw_device_start(ch->cdev, &ch->ccw[3],
diff -urN linux-2.6/drivers/s390/net/lcs.c linux-2.6-patched/drivers/s390/net/lcs.c
--- linux-2.6/drivers/s390/net/lcs.c	2004-11-11 15:06:39.000000000 +0100
+++ linux-2.6-patched/drivers/s390/net/lcs.c	2004-11-11 15:06:56.000000000 +0100
@@ -11,7 +11,7 @@
  *			  Frank Pavlic (pavlic@de.ibm.com) and
  *		 	  Martin Schwidefsky <schwidefsky@de.ibm.com>
  *
- *    $Revision: 1.94 $	 $Date: 2004/10/19 09:30:54 $
+ *    $Revision: 1.96 $	 $Date: 2004/11/11 13:42:33 $
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -59,7 +59,7 @@
 /**
  * initialization string for output
  */
-#define VERSION_LCS_C  "$Revision: 1.94 $"
+#define VERSION_LCS_C  "$Revision: 1.96 $"
 
 static char version[] __initdata = "LCS driver ("VERSION_LCS_C "/" VERSION_LCS_H ")";
 static char debug_buffer[255];
@@ -191,6 +191,7 @@
 		return NULL;
 	memset(card, 0, sizeof(struct lcs_card));
 	card->lan_type = LCS_FRAME_TYPE_AUTO;
+	card->pkt_seq = 0;
 	card->lancmd_timeout = LCS_LANCMD_TIMEOUT_DEFAULT;
 	/* Allocate io buffers for the read channel. */
 	rc = lcs_alloc_channel(&card->read);
@@ -1874,6 +1875,7 @@
 	skb->protocol =	card->lan_type_trans(skb, card->dev);
 	card->stats.rx_bytes += skb_len;
 	card->stats.rx_packets++;
+	*((__u32 *)skb->cb) = ++card->pkt_seq;
 	netif_rx(skb);
 }
 
diff -urN linux-2.6/drivers/s390/net/lcs.h linux-2.6-patched/drivers/s390/net/lcs.h
--- linux-2.6/drivers/s390/net/lcs.h	2004-10-18 23:54:40.000000000 +0200
+++ linux-2.6-patched/drivers/s390/net/lcs.h	2004-11-11 15:06:56.000000000 +0100
@@ -6,7 +6,7 @@
 #include <linux/workqueue.h>
 #include <asm/ccwdev.h>
 
-#define VERSION_LCS_H "$Revision: 1.18 $"
+#define VERSION_LCS_H "$Revision: 1.19 $"
 
 #define LCS_DBF_TEXT(level, name, text) \
 	do { \
@@ -309,6 +309,7 @@
 	__u16 ip_assists_supported;
 	__u16 ip_assists_enabled;
 	__s8 lan_type;
+	__u32 pkt_seq;
 	__u16 sequence_no;
 	__s16 portno;
 	/* Some info copied from probeinfo */
diff -urN linux-2.6/drivers/s390/net/qeth.h linux-2.6-patched/drivers/s390/net/qeth.h
--- linux-2.6/drivers/s390/net/qeth.h	2004-11-11 15:06:39.000000000 +0100
+++ linux-2.6-patched/drivers/s390/net/qeth.h	2004-11-11 15:06:56.000000000 +0100
@@ -24,7 +24,7 @@
 
 #include "qeth_mpc.h"
 
-#define VERSION_QETH_H 		"$Revision: 1.116 $"
+#define VERSION_QETH_H 		"$Revision: 1.123 $"
 
 #ifdef CONFIG_QETH_IPV6
 #define QETH_VERSION_IPV6 	":IPv6"
@@ -557,6 +557,7 @@
 	QETH_IP_TYPE_NORMAL,
 	QETH_IP_TYPE_VIPA,
 	QETH_IP_TYPE_RXIP,
+	QETH_IP_TYPE_DEL_ALL_MC,
 };
 
 enum qeth_cmd_buffer_state {
@@ -713,8 +714,7 @@
  */
 enum qeth_threads {
 	QETH_SET_IP_THREAD  = 1,
-	QETH_SET_MC_THREAD  = 2,
-	QETH_RECOVER_THREAD = 4,
+	QETH_RECOVER_THREAD = 2,
 };
 
 struct qeth_card {
@@ -748,7 +748,7 @@
 	volatile unsigned long thread_running_mask;
 	spinlock_t ip_lock;
 	struct list_head ip_list;
-	struct list_head ip_tbd_list;
+	struct list_head *ip_tbd_list;
 	struct qeth_ipato ipato;
 	struct list_head cmd_waiter_list;
 	/* QDIO buffer handling */
diff -urN linux-2.6/drivers/s390/net/qeth_main.c linux-2.6-patched/drivers/s390/net/qeth_main.c
--- linux-2.6/drivers/s390/net/qeth_main.c	2004-11-11 15:06:39.000000000 +0100
+++ linux-2.6-patched/drivers/s390/net/qeth_main.c	2004-11-11 15:06:56.000000000 +0100
@@ -1,6 +1,6 @@
 /*
  *
- * linux/drivers/s390/net/qeth_main.c ($Revision: 1.155 $)
+ * linux/drivers/s390/net/qeth_main.c ($Revision: 1.168 $)
  *
  * Linux on zSeries OSA Express and HiperSockets support
  *
@@ -12,7 +12,7 @@
  *			  Frank Pavlic (pavlic@de.ibm.com) and
  *		 	  Thomas Spatzier <tspat@de.ibm.com>
  *
- *    $Revision: 1.155 $	 $Date: 2004/10/21 13:27:46 $
+ *    $Revision: 1.168 $	 $Date: 2004/11/08 15:55:12 $
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -78,7 +78,7 @@
 #include "qeth_mpc.h"
 #include "qeth_fs.h"
 
-#define VERSION_QETH_C "$Revision: 1.155 $"
+#define VERSION_QETH_C "$Revision: 1.168 $"
 static const char *version = "qeth S/390 OSA-Express driver";
 
 /**
@@ -160,6 +160,9 @@
 qeth_get_addr_buffer(enum qeth_prot_versions);
 
 static void
+qeth_set_multicast_list(struct net_device *);
+
+static void
 qeth_notify_processes(void)
 {
 	/*notify all  registered processes */
@@ -249,6 +252,7 @@
 		free_netdev(card->dev);
 	qeth_clear_ip_list(card, 0, 0);
 	qeth_clear_ipato_list(card);
+	kfree(card->ip_tbd_list);
 	qeth_free_qdio_buffers(card);
 	kfree(card);
 }
@@ -660,7 +664,10 @@
 	struct qeth_ipaddr *tmp, *t;
 	int found = 0;
 
-	list_for_each_entry_safe(tmp, t, &card->ip_tbd_list, entry) {
+	list_for_each_entry_safe(tmp, t, card->ip_tbd_list, entry) {
+		if ((addr->type == QETH_IP_TYPE_DEL_ALL_MC) &&
+		    (tmp->type == QETH_IP_TYPE_DEL_ALL_MC))
+			return 0;
 		if ((tmp->proto        == QETH_PROT_IPV4)     &&
 		    (addr->proto       == QETH_PROT_IPV4)     &&
 		    (tmp->type         == addr->type)         &&
@@ -692,14 +699,18 @@
 		}
 		return 0;
 	} else {
-		if (addr->users == 0)
-			addr->users += add? 1:-1;
-		if (add && (addr->type == QETH_IP_TYPE_NORMAL) &&
-		    qeth_is_addr_covered_by_ipato(card, addr)){
-			QETH_DBF_TEXT(trace, 2, "tkovaddr");
-			addr->set_flags |= QETH_IPA_SETIP_TAKEOVER_FLAG;
+		if (addr->type == QETH_IP_TYPE_DEL_ALL_MC)
+			list_add(&addr->entry, card->ip_tbd_list);
+		else {
+			if (addr->users == 0)
+				addr->users += add? 1:-1;
+			if (add && (addr->type == QETH_IP_TYPE_NORMAL) &&
+			    qeth_is_addr_covered_by_ipato(card, addr)){
+				QETH_DBF_TEXT(trace, 2, "tkovaddr");
+				addr->set_flags |= QETH_IPA_SETIP_TAKEOVER_FLAG;
+			}
+			list_add_tail(&addr->entry, card->ip_tbd_list);
 		}
-		list_add_tail(&addr->entry, &card->ip_tbd_list);
 		return 1;
 	}
 }
@@ -717,8 +728,8 @@
 	if (addr->proto == QETH_PROT_IPV4)
 		QETH_DBF_HEX(trace,4,&addr->u.a4.addr,4);
 	else {
-		QETH_DBF_HEX(trace,4,&addr->u.a6.addr,4);
-		QETH_DBF_HEX(trace,4,((char *)&addr->u.a6.addr)+4,4);
+		QETH_DBF_HEX(trace,4,&addr->u.a6.addr,8);
+		QETH_DBF_HEX(trace,4,((char *)&addr->u.a6.addr)+8,8);
 	}
 	spin_lock_irqsave(&card->ip_lock, flags);
 	rc = __qeth_insert_ip_todo(card, addr, 0);
@@ -736,8 +747,8 @@
 	if (addr->proto == QETH_PROT_IPV4)
 		QETH_DBF_HEX(trace,4,&addr->u.a4.addr,4);
 	else {
-		QETH_DBF_HEX(trace,4,&addr->u.a6.addr,4);
-		QETH_DBF_HEX(trace,4,((char *)&addr->u.a6.addr)+4,4);
+		QETH_DBF_HEX(trace,4,&addr->u.a6.addr,8);
+		QETH_DBF_HEX(trace,4,((char *)&addr->u.a6.addr)+8,8);
 	}
 	spin_lock_irqsave(&card->ip_lock, flags);
 	rc = __qeth_insert_ip_todo(card, addr, 1);
@@ -745,19 +756,21 @@
 	return rc;
 }
 
-static void
-qeth_reinsert_todos(struct qeth_card *card, struct list_head *todos)
+static inline void
+__qeth_delete_all_mc(struct qeth_card *card, unsigned long *flags)
 {
-	struct qeth_ipaddr *todo, *tmp;
+	struct qeth_ipaddr *addr, *tmp;
+	int rc;
 
-	list_for_each_entry_safe(todo, tmp, todos, entry){
-		list_del_init(&todo->entry);
-		if (todo->users < 0) {
-			if (!qeth_delete_ip(card, todo))
-				kfree(todo);
-		} else {
-			if (!qeth_add_ip(card, todo))
-				kfree(todo);
+	list_for_each_entry_safe(addr, tmp, &card->ip_list, entry) {
+		if (addr->is_multicast) {
+			spin_unlock_irqrestore(&card->ip_lock, *flags);
+			rc = qeth_deregister_addr_entry(card, addr);
+			spin_lock_irqsave(&card->ip_lock, *flags);
+			if (!rc) {
+				list_del(&addr->entry);
+				kfree(addr);
+			}
 		}
 	}
 }
@@ -765,7 +778,7 @@
 static void
 qeth_set_ip_addr_list(struct qeth_card *card)
 {
-	struct list_head failed_todos;
+	struct list_head *tbd_list;
 	struct qeth_ipaddr *todo, *addr;
 	unsigned long flags;
 	int rc;
@@ -773,13 +786,25 @@
 	QETH_DBF_TEXT(trace, 2, "sdiplist");
 	QETH_DBF_HEX(trace, 2, &card, sizeof(void *));
 
-	INIT_LIST_HEAD(&failed_todos);
-
 	spin_lock_irqsave(&card->ip_lock, flags);
-	while (!list_empty(&card->ip_tbd_list)) {
-		todo = list_entry(card->ip_tbd_list.next,
-				  struct qeth_ipaddr, entry);
-		list_del_init(&todo->entry);
+	tbd_list = card->ip_tbd_list;
+	card->ip_tbd_list = kmalloc(sizeof(struct list_head), GFP_ATOMIC);
+	if (!card->ip_tbd_list) {
+		QETH_DBF_TEXT(trace, 0, "silnomem");
+		card->ip_tbd_list = tbd_list;
+		spin_unlock_irqrestore(&card->ip_lock, flags);
+		return;
+	} else
+		INIT_LIST_HEAD(card->ip_tbd_list);
+
+	while (!list_empty(tbd_list)){
+		todo = list_entry(tbd_list->next, struct qeth_ipaddr, entry);
+		list_del(&todo->entry);
+		if (todo->type == QETH_IP_TYPE_DEL_ALL_MC){
+			__qeth_delete_all_mc(card, &flags);
+			kfree(todo);
+			continue;
+		}
 		rc = __qeth_ref_ip_on_card(card, todo, &addr);
 		if (rc == 0) {
 			/* nothing to be done; only adjusted refcount */
@@ -792,24 +817,22 @@
 			if (!rc)
 				list_add_tail(&todo->entry, &card->ip_list);
 			else
-				list_add_tail(&todo->entry, &failed_todos);
+				kfree(todo);
 		} else if (rc == -1) {
 			/* on-card entry to be removed */
 			list_del_init(&addr->entry);
 			spin_unlock_irqrestore(&card->ip_lock, flags);
 			rc = qeth_deregister_addr_entry(card, addr);
 			spin_lock_irqsave(&card->ip_lock, flags);
-			if (!rc) {
+			if (!rc)
 				kfree(addr);
-				kfree(todo);
-			} else {
+			else
 				list_add_tail(&addr->entry, &card->ip_list);
-				list_add_tail(&todo->entry, &failed_todos);
-			}
+			kfree(todo);
 		}
 	}
 	spin_unlock_irqrestore(&card->ip_lock, flags);
-	qeth_reinsert_todos(card, &failed_todos);
+	kfree(tbd_list);
 }
 
 static void qeth_delete_mc_addresses(struct qeth_card *);
@@ -887,28 +910,7 @@
 }
 
 static int
-qeth_register_mc_addresses(void *ptr)
-{
-	struct qeth_card *card;
-
-	card = (struct qeth_card *) ptr;
-	daemonize("qeth_reg_mcaddrs");
-	QETH_DBF_TEXT(trace,4,"regmcth1");
-	if (!qeth_do_run_thread(card, QETH_SET_MC_THREAD))
-		return 0;
-	QETH_DBF_TEXT(trace,4,"regmcth2");
-	qeth_delete_mc_addresses(card);
-	qeth_add_multicast_ipv4(card);
-#ifdef CONFIG_QETH_IPV6
-	qeth_add_multicast_ipv6(card);
-#endif
-	qeth_set_ip_addr_list(card);
-	qeth_clear_thread_running_bit(card, QETH_SET_MC_THREAD);
-	return 0;
-}
-
-static int
-qeth_register_ip_address(void *ptr)
+qeth_register_ip_addresses(void *ptr)
 {
 	struct qeth_card *card;
 
@@ -988,9 +990,7 @@
 		return;
 
 	if (qeth_do_start_thread(card, QETH_SET_IP_THREAD))
-		kernel_thread(qeth_register_ip_address, (void *) card, SIGCHLD);
-	if (qeth_do_start_thread(card, QETH_SET_MC_THREAD))
-		kernel_thread(qeth_register_mc_addresses, (void *)card,SIGCHLD);
+		kernel_thread(qeth_register_ip_addresses, (void *)card,SIGCHLD);
 	if (qeth_do_start_thread(card, QETH_RECOVER_THREAD))
 		kernel_thread(qeth_recover, (void *) card, SIGCHLD);
 }
@@ -1041,7 +1041,12 @@
 	INIT_WORK(&card->kernel_thread_starter,
 		  (void *)qeth_start_kernel_thread,card);
 	INIT_LIST_HEAD(&card->ip_list);
-	INIT_LIST_HEAD(&card->ip_tbd_list);
+	card->ip_tbd_list = kmalloc(sizeof(struct list_head), GFP_KERNEL);
+	if (!card->ip_tbd_list) {
+		QETH_DBF_TEXT(setup, 0, "iptbdnom");
+		return -ENOMEM;
+	}
+	INIT_LIST_HEAD(card->ip_tbd_list);
 	INIT_LIST_HEAD(&card->cmd_waiter_list);
 	init_waitqueue_head(&card->wait_q);
 	/* intial options */
@@ -1575,9 +1580,8 @@
 	QETH_DBF_TEXT(trace, 2, "rstipadd");
 
 	qeth_clear_ip_list(card, 0, 1);
-	if ( (qeth_set_thread_start_bit(card, QETH_SET_IP_THREAD) == 0) ||
-	     (qeth_set_thread_start_bit(card, QETH_SET_MC_THREAD) == 0) )
-		schedule_work(&card->kernel_thread_starter);
+	/* this function will also schedule the SET_IP_THREAD */
+	qeth_set_multicast_list(card->dev);
 }
 
 static struct qeth_ipa_cmd *
@@ -1600,10 +1604,7 @@
 					   card->info.if_name,
 					   card->info.chpid);
 				card->lan_online = 0;
-				if (netif_carrier_ok(card->dev)) {
-					netif_carrier_off(card->dev);
-					netif_stop_queue(card->dev);
-				}
+				netif_carrier_off(card->dev);
 				return NULL;
 			case IPA_CMD_STARTLAN:
 				PRINT_INFO("Link reestablished on %s "
@@ -1612,10 +1613,7 @@
 					   card->info.if_name,
 					   card->info.chpid);
 				card->lan_online = 1;
-				if (!netif_carrier_ok(card->dev)) {
-					netif_carrier_on(card->dev);
-					netif_wake_queue(card->dev);
-				}
+				netif_carrier_on(card->dev);
 				qeth_reset_ip_addresses(card);
 				return NULL;
 			case IPA_CMD_REGISTER_LOCAL_ADDR:
@@ -2807,7 +2805,8 @@
 	}
 	atomic_sub(count, &queue->used_buffers);
 	/* check if we need to do something on this outbound queue */
-	qeth_check_outbound_queue(queue);
+	if (card->info.type != QETH_CARD_TYPE_IQD)
+		qeth_check_outbound_queue(queue);
 
 	netif_wake_queue(card->dev);
 #ifdef CONFIG_QETH_PERF_STATS
@@ -3381,13 +3380,16 @@
 	if (skb==NULL) {
 		card->stats.tx_dropped++;
 		card->stats.tx_errors++;
-		return -EIO;
+		/* return OK; otherwise ksoftirqd goes to 100% */
+		return NETDEV_TX_OK;
 	}
-	if ((card->state != CARD_STATE_UP) || !netif_carrier_ok(dev)) {
+	if ((card->state != CARD_STATE_UP) || !card->lan_online) {
 		card->stats.tx_dropped++;
 		card->stats.tx_errors++;
 		card->stats.tx_carrier_errors++;
-		return -EIO;
+		dev_kfree_skb_any(skb);
+		/* return OK; otherwise ksoftirqd goes to 100% */
+		return NETDEV_TX_OK;
 	}
 #ifdef CONFIG_QETH_PERF_STATS
 	card->perf_stats.outbound_cnt++;
@@ -3398,8 +3400,18 @@
 	 * got our own synchronization on queues we can keep the stack's
 	 * queue running.
 	 */
-	if ((rc = qeth_send_packet(card, skb)))
-		netif_stop_queue(dev);
+	if ((rc = qeth_send_packet(card, skb))){
+		if (rc == -EBUSY) {
+			netif_stop_queue(dev);
+			rc = NETDEV_TX_BUSY;
+		} else {
+			card->stats.tx_errors++;
+			card->stats.tx_dropped++;
+			dev_kfree_skb_any(skb);
+			/* set to OK; otherwise ksoftirqd goes to 100% */
+			rc = NETDEV_TX_OK;
+		}
+	}
 
 #ifdef CONFIG_QETH_PERF_STATS
 	card->perf_stats.outbound_time += qeth_get_micros() -
@@ -3503,7 +3515,6 @@
 	if (!card->lan_online){
 		if (netif_carrier_ok(dev))
 			netif_carrier_off(dev);
-		netif_stop_queue(dev);
 	}
 	return 0;
 }
@@ -4988,12 +4999,28 @@
 	spin_unlock_irqrestore(&card->vlanlock, flags);
 	if (card->options.layer2)
 		qeth_layer2_send_setdelvlan(card, vid, IPA_CMD_DELVLAN);
- 	if ( (qeth_set_thread_start_bit(card, QETH_SET_IP_THREAD) == 0) ||
-	     (qeth_set_thread_start_bit(card, QETH_SET_MC_THREAD) == 0) )
-		schedule_work(&card->kernel_thread_starter);
+	qeth_set_multicast_list(card->dev);
 }
 #endif
 
+/**
+ * set multicast address on card
+ */
+static void
+qeth_set_multicast_list(struct net_device *dev)
+{
+	struct qeth_card *card = (struct qeth_card *) dev->priv;
+
+	QETH_DBF_TEXT(trace,3,"setmulti");
+	qeth_delete_mc_addresses(card);
+	qeth_add_multicast_ipv4(card);
+#ifdef CONFIG_QETH_IPV6
+	qeth_add_multicast_ipv6(card);
+#endif
+ 	if (qeth_set_thread_start_bit(card, QETH_SET_IP_THREAD) == 0)
+		schedule_work(&card->kernel_thread_starter);
+}
+
 static int
 qeth_neigh_setup(struct net_device *dev, struct neigh_parms *np)
 {
@@ -5049,24 +5076,19 @@
 static void
 qeth_delete_mc_addresses(struct qeth_card *card)
 {
-	struct qeth_ipaddr *ipm, *iptodo;
+	struct qeth_ipaddr *iptodo;
 	unsigned long flags;
 
 	QETH_DBF_TEXT(trace,4,"delmc");
-	spin_lock_irqsave(&card->ip_lock, flags);
-	list_for_each_entry(ipm, &card->ip_list, entry){
-		if (!ipm->is_multicast)
-			continue;
-		iptodo = qeth_get_addr_buffer(ipm->proto);
-		if (!iptodo) {
-			QETH_DBF_TEXT(trace, 2, "dmcnomem");
-			continue;
-		}
-		memcpy(iptodo, ipm, sizeof(struct qeth_ipaddr));
-		iptodo->users = iptodo->users * -1;
-		if (!__qeth_insert_ip_todo(card, iptodo, 0))
-			kfree(iptodo);
+	iptodo = qeth_get_addr_buffer(QETH_PROT_IPV4);
+	if (!iptodo) {
+		QETH_DBF_TEXT(trace, 2, "dmcnomem");
+		return;
 	}
+	iptodo->type = QETH_IP_TYPE_DEL_ALL_MC;
+	spin_lock_irqsave(&card->ip_lock, flags);
+	if (!__qeth_insert_ip_todo(card, iptodo, 0))
+		kfree(iptodo);
 	spin_unlock_irqrestore(&card->ip_lock, flags);
 }
 
@@ -5277,20 +5299,6 @@
 
 	return rc;
 }
-/**
- * set multicast address on card
- */
-static void
-qeth_set_multicast_list(struct net_device *dev)
-{
-	struct qeth_card *card;
-
-	QETH_DBF_TEXT(trace,3,"setmulti");
-	card = (struct qeth_card *) dev->priv;
-
-	if (qeth_set_thread_start_bit(card, QETH_SET_MC_THREAD) == 0)
-		schedule_work(&card->kernel_thread_starter);
-}
 
 static void
 qeth_fill_ipacmd_header(struct qeth_card *card, struct qeth_ipa_cmd *cmd,
@@ -6635,7 +6643,7 @@
 	QETH_DBF_TEXT(trace,4,"clearip");
 	spin_lock_irqsave(&card->ip_lock, flags);
 	/* clear todo list */
-	list_for_each_entry_safe(addr, tmp, &card->ip_tbd_list, entry){
+	list_for_each_entry_safe(addr, tmp, card->ip_tbd_list, entry){
 		list_del(&addr->entry);
 		kfree(addr);
 	}
@@ -6653,7 +6661,7 @@
 			kfree(addr);
 			continue;
 		}
-		list_add_tail(&addr->entry, &card->ip_tbd_list);
+		list_add_tail(&addr->entry, card->ip_tbd_list);
 	}
 	spin_unlock_irqrestore(&card->ip_lock, flags);
 }
@@ -6893,8 +6901,7 @@
 	rtnl_lock();
 	dev_open(card->dev);
 	rtnl_unlock();
- 	if (qeth_set_thread_start_bit(card, QETH_SET_MC_THREAD) == 0)
-		schedule_work(&card->kernel_thread_starter);
+	qeth_set_multicast_list(card->dev);
 }
 
 
@@ -7008,9 +7015,7 @@
 /*maybe it was set offline without ifconfig down
  * we can also use this state for recovery purposes*/
 	if (card->options.layer2)
-		qeth_set_allowed_threads(card,
-					 QETH_RECOVER_THREAD |
-					 QETH_SET_MC_THREAD,0);
+		qeth_set_allowed_threads(card, QETH_RECOVER_THREAD, 0);
 	else
 		qeth_set_allowed_threads(card, 0xffffffff, 0);
 	if (recover_flag == CARD_STATE_RECOVER)
@@ -7339,7 +7344,7 @@
 		return -ENOMEM;
 	spin_lock_irqsave(&card->ip_lock, flags);
 	if (__qeth_address_exists_in_list(&card->ip_list, ipaddr, 0) ||
-	    __qeth_address_exists_in_list(&card->ip_tbd_list, ipaddr, 0))
+	    __qeth_address_exists_in_list(card->ip_tbd_list, ipaddr, 0))
 		rc = -EEXIST;
 	spin_unlock_irqrestore(&card->ip_lock, flags);
 	if (rc){
@@ -7412,7 +7417,7 @@
 		return -ENOMEM;
 	spin_lock_irqsave(&card->ip_lock, flags);
 	if (__qeth_address_exists_in_list(&card->ip_list, ipaddr, 0) ||
-	    __qeth_address_exists_in_list(&card->ip_tbd_list, ipaddr, 0))
+	    __qeth_address_exists_in_list(card->ip_tbd_list, ipaddr, 0))
 		rc = -EEXIST;
 	spin_unlock_irqrestore(&card->ip_lock, flags);
 	if (rc){

Re: [patch 4/10] s390: network driver.

[Jeff Garzik]

Martin Schwidefsky wrote:
> [patch 4/10] s390: network driver.
> 
> From: Thomas Spatzier <tspat@de.ibm.com>
> From: Peter Tiedemann <ptiedem@de.ibm.com>
> 
> network driver changes:
>  - qeth: return -EINVAL if an skb is too large.
>  - qeth: don't call netif_stop_queue after cable pull. Drop the
>    packets instead.


You should be using netif_carrier_{on,off} properly, and not drop the 
packets.  When (if) link comes back, you requeue the packets to hardware 
(or hypervisor or whatever).  Your dev->stop() should stop operation and 
clean up anything left in your send/receive {rings | buffers}.

	Jeff

Re: [patch 4/10] s390: network driver.

[Thomas Spatzier]

> You should be using netif_carrier_{on,off} properly, and not drop the
> packets.  When (if) link comes back, you requeue the packets to hardware
> (or hypervisor or whatever).  Your dev->stop() should stop operation and
> clean up anything left in your send/receive {rings | buffers}.
>

When we do not drop packets, but call netif_stop_queue the write queues
of all sockets associated to the net device are blocked as soon as they
get full. This causes problems with programs such as the zebra routing
daemon. So we have to keep the netif queue running in order to not block
any programs.
We also had a look at some other drivers and the common behaviour seems to
be that packets are lost if the network cable is pulled out.

Regards,
Thomas.

Re: [patch 4/10] s390: network driver.

[Jeff Garzik]

Thomas Spatzier wrote:
> 
> 
> 
>>You should be using netif_carrier_{on,off} properly, and not drop the
>>packets.  When (if) link comes back, you requeue the packets to hardware
>>(or hypervisor or whatever).  Your dev->stop() should stop operation and
>>clean up anything left in your send/receive {rings | buffers}.
>>
> 
> 
> When we do not drop packets, but call netif_stop_queue the write queues
> of all sockets associated to the net device are blocked as soon as they
> get full. This causes problems with programs such as the zebra routing
> daemon. So we have to keep the netif queue running in order to not block
> any programs.

This is very, very wrong.  You are essentially creating in in-driver 
/dev/null simulator.  This does nothing but chew CPU cycles by having 
the driver drop packets, rather than allowing the system to work as it 
should.

Queues are DESIGNED to fill up under various conditions.

Would not the zebra routing software have the same problems with cable 
pull under an e1000 or tg3 gigabit NIC?


> We also had a look at some other drivers and the common behaviour seems to
> be that packets are lost if the network cable is pulled out.

Which drivers, specifically, are these?

The most popular drivers -- e1000, tg3, etc. -- do not do this, for very 
good reasons.

	Jeff

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Sat, 13 Nov 2004, Jeff Garzik wrote:

> Queues are DESIGNED to fill up under various conditions.

What happens when they are full? Blocking isnt good, at least not 
from GNU Zebra / Quagga's POV.

> Would not the zebra routing software have the same problems with 
> cable pull under an e1000 or tg3 gigabit NIC?

If they block further writes, yes.

> The most popular drivers -- e1000, tg3, etc. -- do not do this, for 
> very good reasons.

The problem is that GNU Zebra / Quagga uses a single raw socket for 
certain protocols, eg OSPF. Blocking the socket because one NIC has a 
cable pulled is undesireable, and there's no point queueing the 
packets, by the time the link comes back, if ever, its highly 
unlikely there is any use in sending the packets (sending OSPF 
hello's from X minutes ago on a link that just came back is useless).

The kernel really shouldnt get too much in the way of an application 
that already is fully aware of reliability issues[1] - at least that 
is the application's expectation in this case.

We could change it to use a socket/interface on Linux, but it seems a 
bit unnecessary to me, at least for raw socket/included-header 
sockets.

> 	Jeff

1. an application must be if its uses raw sockets surely? Even for 
non-raw/header-included sockets, eg BGP tcp sockets, a user like GNU 
Zebra / Quagga would much prefer packets to be dropped.

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
My interest is in the future because I am going to spend the rest of my
life there.

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Mon, 15 Nov 2004, Paul Jakma wrote:

> non-raw/header-included sockets, eg BGP tcp sockets, a user like 
> GNU Zebra / Quagga would much prefer packets to be dropped.

Ur... not for TCP.. obviously.

Anyway, is there any advice on how applications that use a single 
socket for raw/udp should deal with this new behaviour? All of the 
link-orientated routing protocol daemons in Quagga/GNU Zebra are 
going to break on Linux with this new behaviour.

Should such applications be changed to open a seperate socket per 
interface? Or could we have a SO_DROP_DONT_QUEUE sockopt to allow a 
privileged application to retain the previous behaviour, or some way 
to flush the queue for a socket?

Using a socket per interface wont address problem of sending quite 
stale packets when a link comes back after a long time down, AUI. 
(not a huge problem - but not nice).

Jeff???

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
Be incomprehensible.  If they can't understand, they can't disagree.

Re: [patch 4/10] s390: network driver.

[Thomas Spatzier]

> Using a socket per interface wont address problem of sending
> quite stale packets when a link > comes back after a long time
> down, AUI. (not a huge problem - but not nice).
>
> Jeff???

Has there been any outcome on the discussion about whether or not
a device driver should drop packets when the cable is disconnected?
It seems that from the zebra point of view, as Paul wrote,
it would be better to not block sockets by queueing up packets
when there is no cable connection.

I do also think that it does not make sense to keep packets in the
queue and then send those packets when the cable is plugged in
again after a possibly long time.
There are protocols like TCP that handle packet loss anyway.

Regards,
Thomas.

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Mon, 29 Nov 2004, Thomas Spatzier wrote:

> Has there been any outcome on the discussion about whether or not a 
> device driver should drop packets when the cable is disconnected?

There hasnt.

> It seems that from the zebra point of view, as Paul wrote, it would 
> be better to not block sockets by queueing up packets when there is 
> no cable connection.

I'd prefer either to get ENOBUFS or to have kernel drop the packet - 
we're privileged apps writing to raw sockets and/or with IP_HDRINCL, 
the kernel should assume we know what we're doing (cause if we dont, 
there's far /worse/ we could do than send packets to an 
effective /dev/null).

> I do also think that it does not make sense to keep packets in the 
> queue and then send those packets when the cable is plugged in 
> again after a possibly long time.

Well, if the kernel is going to queue these packets without notifying 
us, we absolutely *must* have some way to flush those queues. Sending 
stale packets many minutes after the application generated them could 
have serious consequences for routing (eg, think sending RIP, IPv4 
IRDP or v6 RAs which are no longer valid - client receives them and 
installs routes which are long invalid and loses connectivity to some 
part of the network).

So even if we moved to socket/interface, we still need some way to 
tell kernel to flush a socket when we receive link-down over netlink 
later.

Not trying to queue on sockets where app has no expectation of 
reliability in first place would be even better ;)

> There are protocols like TCP that handle packet loss anyway.

Yes.

I'd be very interested to hear advice from the kernel gurus (eg "god, 
dont be so stupid, do xyz in your application instead"). We can 
accomodate whatever kernel wants as long as its workable.

> Regards,
> Thomas.

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
You will pay for your sins.  If you have already paid, please disregard
this message.

Re: [patch 4/10] s390: network driver.

[Thomas Spatzier]

Paul Jakma <paul@clubi.ie> wrote on 29.11.2004 17:30:23:
> Well, if the kernel is going to queue these packets without notifying
> us, we absolutely *must* have some way to flush those queues. Sending
> stale packets many minutes after the application generated them could
> have serious consequences for routing (eg, think sending RIP, IPv4
> IRDP or v6 RAs which are no longer valid - client receives them and
> installs routes which are long invalid and loses connectivity to some
> part of the network).
>

Yes, for the examples you mentioned the app should better be notified.
However, AFAICS, there are no such notification mechanisms on a
per-packet basis implemented in the kernel.
And I doubt that they are going to be implemented.

> I'd be very interested to hear advice from the kernel gurus (eg "god,
> dont be so stupid, do xyz in your application instead"). We can
> accomodate whatever kernel wants as long as its workable.

Good suggestion, if anyone has an interesting and feasible solution
I will be happy to integrate it. So far, however, it don't see one and I
would point people being worried about lost packets to TCP.


Regards,
Thomas.

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Mon, 29 Nov 2004, Thomas Spatzier wrote:

> Yes, for the examples you mentioned the app should better be 
> notified. However, AFAICS, there are no such notification 
> mechanisms on a per-packet basis implemented in the kernel. And I 
> doubt that they are going to be implemented.

We do get notified. We get a netlink interface update with 
unset IFF_RUNNING from the interface flags.

However, it can take time before zebra gets to read it, and further 
time before zebra sends its own interface update to protocol daemons, 
and further time before they might get to read and update their own 
interface state. By which time those daemons may have sent packets 
down those interfaces (which packets may become invalid before link 
becomes back, but which still will be sent and hence which 
potentially could disrupt routing).

Ideally, we should be notified synchronously (EBUFS?) or if thats not 
possible, packet should be dropped (see my below presumption though).

The other solution is some means for a daemon to be able flush queues 
for a specific interface. (but thats a bit ugly).

> Good suggestion, if anyone has an interesting and feasible solution 
> I will be happy to integrate it. So far, however, it don't see one 
> and I would point people being worried about lost packets to TCP.

Surely TCP already was able to take care of retransmits? I'm not 
familiar with Linux internals, but how did TCP cope with the previous 
driver behaviour? (I get the vague impression that we're talking 
about a driver specific buffer here, rather than the applications 
socket buffer - is that correct?).

Jeff, David, enlighten us please! :)

> Regards,
> Thomas.

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
I can't understand why people are frightened of new ideas.  I'm frightened
of the old ones.
 		-- John Cage

Re: [patch 4/10] s390: network driver.

[Thomas Spatzier]

Paul Jakma <paul@clubi.ie> wrote on 29.11.2004 21:27:57:
> We do get notified. We get a netlink interface update with
> unset IFF_RUNNING from the interface flags.
>
> However, it can take time before zebra gets to read it, and further
> time before zebra sends its own interface update to protocol daemons,
> and further time before they might get to read and update their own
> interface state. By which time those daemons may have sent packets
> down those interfaces (which packets may become invalid before link
> becomes back, but which still will be sent and hence which
> potentially could disrupt routing).

Ok, then some logic could be implemented in userland to take
appropriate actions. It must be ensured that zebra handles the
netlink notification fast enough.

>
> Ideally, we should be notified synchronously (EBUFS?) or if thats not
> possible, packet should be dropped (see my below presumption though).

In the manpages for send/sendto/sendmsg it says that there is a -ENOBUFS
return value, if a sockets write queue is full. It also says:
"Normally, this does not occur in Linux. Packets are just silently dropped
when a device queue overflows."
So, if packets are 'silently dropped' anyway, the fact that we drop them in
our driver (and increment the error count in the net_device_stats
accordingly)
should not be a problem.

> Surely TCP already was able to take care of retransmits? I'm not
> familiar with Linux internals, but how did TCP cope with the previous
> driver behaviour?

I think that both behaviours are similar for TCP. TCP waits for ACKs for
each packet. If they do not arrive, a retransmit is done. Sooner or later
the connection will be reset, if no responses from the other side arrive.
So the result for both driver behaviours should be the same.

Regards,
Thomas

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Tue, 30 Nov 2004, Thomas Spatzier wrote:

> Ok, then some logic could be implemented in userland to take 
> appropriate actions. It must be ensured that zebra handles the 
> netlink notification fast enough.

AIUI, netlink is not synchronous, it most definitely makes no 
reliability guarantees (and at the moment, zebra isnt terribly 
efficient at reading netlink, large numbers of interfaces will cause 
overruns in zebra - fixing this is on the TODO list). So we can never 
get rid of the window where a daemon could send a packet out a 
link-down interface - we can make that window smaller but not 
eliminate it.

Hence we need either a way to flush packets associated with an 
(interface,socket) (or just the socket) or we need the kernel to not 
accept such packets (and drop packets it has accepted).

> In the manpages for send/sendto/sendmsg it says that there is a -ENOBUFS
> return value, if a sockets write queue is full.

Yes, ENOBUFS, sorry.

> It also says:

> "Normally, this does not occur in Linux. Packets are just silently dropped
> when a device queue overflows."

This has always been (AFAIK) the behaviour yes. We started getting 
reports of the new queuing behaviour with, iirc, a version of Intel's 
e100 driver for 2.4.2x, which was later changed back to the old 
behaviour. However now that the queue behaviour is apparently the 
mandated behaviour we really need to work out what to do about the 
sending-long-stale packets problem.

> So, if packets are 'silently dropped' anyway, the fact that we drop 
> them in our driver (and increment the error count in the 
> net_device_stats accordingly) should not be a problem.

It shouldnt no.

The likes of OSPF already specify their own reliability mechanisms.

> I think that both behaviours are similar for TCP. TCP waits for 
> ACKs for each packet. If they do not arrive, a retransmit is done. 
> Sooner or later the connection will be reset, if no responses from 
> the other side arrive. So the result for both driver behaviours 
> should be the same.

But if TCP worked even when drivers dropped packets, then that 
implies TCP has its own queue? That we're talking about a seperate 
driver packet queue rather than the socket buffer (which is, 
presumably, where TCP retains packets until ACKed - i have no idea).

Anyway, we do, I think, need some way to deal with the 
sending-stale-packet-on-link-back problem. Either a way to flush this 
driver queue or else a guarantee that writes to sockets whose 
protocol makes no reliability guarantee will either return ENOBUFS or 
drop the packet.

Otherwise we will start getting reports of "Quagga on Linux sent an 
ancient {RIP,IRDP,RA} packet when we fixed a switch problem, and it 
caused an outage for a section of our network due to bad routes", I 
think.

Some comment or advice would be useful. (Am I kill-filed by all of 
netdev? feels like it).

> Regards,
> Thomas

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
No directory.

Re: [patch 4/10] s390: network driver.

[jamal]

On Sun, 2004-12-05 at 01:25, Paul Jakma wrote:
[..]
> Anyway, we do, I think, need some way to deal with the 
> sending-stale-packet-on-link-back problem. Either a way to flush this 
> driver queue or else a guarantee that writes to sockets whose 
> protocol makes no reliability guarantee will either return ENOBUFS or 
> drop the packet.
> 
> Otherwise we will start getting reports of "Quagga on Linux sent an 
> ancient {RIP,IRDP,RA} packet when we fixed a switch problem, and it 
> caused an outage for a section of our network due to bad routes", I 
> think.
> 
> Some comment or advice would be useful. (Am I kill-filed by all of 
> netdev? feels like it).

Dont post networking related patches on other lists. I havent seen said
patch, but it seems someone is complaining about some behavior changing?

In regards to link down and packets being queued.
Agreed this is a little problematic for some apps/transports. TCP is
definetely not one of them. TCP in Linux actually is told if the drop is
local. This way it can make better judgement (and not unnecesarily
adjust windows for example).
SCTP AFAIK is the only transport that provides its apps opportunity to
obsolete messages already sent. I am not sure how well implemented or
whtether it is implemented at all. Someone working on SCTP could
comment.

In the case the netdevice is administratively downed both the qdisc and
DMA ring packets are flushed. Newer packets will never be queued and
you should quickly be able to find from your app that the device is
down.
In the case of netdevice being operationally down - I am hoping this is
what the discussion is, having jumped on it - both queues stay intact.
What you can do is certainly from user space admin down/up the device
when you receive a netlink carrier off notification.
I am struggling to see whether dropping the packet inside the tx path
once it is operationaly down is so blasphemous ... need to get caffeine
first.

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[Hasso Tepper]

jamal wrote:
> Dont post networking related patches on other lists. I havent seen said
> patch, but it seems someone is complaining about some behavior changing?

For some strange reason I didn't find the beginning of thread either from 
archive, the first post seems to be  
http://oss.sgi.com/projects/netdev/archive/2004-11/msg01015.html.

I'm trying to summarize. The approach - one raw socket to send/receive 
messages no matter how many interfaces are used - worked for Quagga/Zebra 
routing software daemons till now. If some of these interfaces was 
operationally down, socket wasn't blocked even if the queue was full. In 
fact "man sendmsg" has still text:

ENOBUFS
	The output queue for a network interface was full.  This  gener-
	ally  indicates  that the interface has stopped sending, but may
	be caused by transient congestion.   (Normally,  this  does  not
	occur  in Linux. Packets are just silently dropped when a device
	queue overflows.)

Seems that it's no longer true. Seems that kernel is now trying as hard as 
possible not to loose any data - data is queued and if the queue will be 
full, all related sockets will be blocked to notify application. So, one 
socket approach don't work any more for Quagga/Zebra. No problem, we can 
take the "one socket per interface" approach. And we already have link 
detection implemented to notify daemons.

But there will be still potential problem - sending the "interface down" 
message from kernel to the zebra daemon and then to the routing protocol 
daemon takes time. And during this time daemon can send packets which will 
sit in queue and may cause many problems if sent to the network later (if 
link comes up again). Think about statelss routing protocols like rip(ng), 
ipv6 router advertisements etc. They may carry the info that's no longer 
true causing routing loops etc.

And to clarify a little bit: no - the Quagga/Zebra didn't work with previous 
approach perfectly. I fact with link detection and socket per interface it 
will be better than ever no matter what's the kernel behaviour. We just 
want to make sure that solution will be bulletproof.

So, problem - how can we make sure that no potentially dangerous aged 
(routing) info will be in the network?


-- 
Hasso Tepper
Elion Enterprises Ltd.
WAN administrator

Re: [patch 4/10] s390: network driver.

[Herbert Xu]

Hasso Tepper <hasso@estpak.ee> wrote:
> 
> Seems that it's no longer true. Seems that kernel is now trying as hard as 
> possible not to loose any data - data is queued and if the queue will be 
> full, all related sockets will be blocked to notify application. So, one 
> socket approach don't work any more for Quagga/Zebra. No problem, we can 
> take the "one socket per interface" approach. And we already have link 
> detection implemented to notify daemons.

I don't see why this should be happening.  Can you please provide a
minimal program that reproduces this blocking problem? For example,
something that sends a packet to a downed interface and then sends
one to an interface that's up?
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [patch 4/10] s390: network driver.

[jamal]

On Mon, 2004-12-06 at 20:13, Herbert Xu wrote:
> Hasso Tepper <hasso@estpak.ee> wrote:
> > 
> > Seems that it's no longer true. Seems that kernel is now trying as hard as 
> > possible not to loose any data - data is queued and if the queue will be 
> > full, all related sockets will be blocked to notify application. So, one 
> > socket approach don't work any more for Quagga/Zebra. No problem, we can 
> > take the "one socket per interface" approach. And we already have link 
> > detection implemented to notify daemons.
> 
> I don't see why this should be happening.  Can you please provide a
> minimal program that reproduces this blocking problem? For example,
> something that sends a packet to a downed interface and then sends
> one to an interface that's up?

This may be something to do with socket level changes maybe?
Does this happen with sockets that are not raw?

Having said that: I think getting rid of obsolete messages is important.
One of the suggested schemes of operation sounds to be the least brutal.

Jgarzik, I thought about it a little and it seems to me that allowing
the device driver to drop packets on txmit when netcarrier is off is not
that bad. This is almost like pretending packets were dropped on the
wire. Thoughts?

cheers,
jamal 

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Tue, 7 Dec 2004, Herbert Xu wrote:

> I don't see why this should be happening.

Thomas' original patch was to address this problem. I wonder could he 
recap the kernel side of this problem?

> Can you please provide a minimal program that reproduces this 
> blocking problem? For example, something that sends a packet to a 
> downed interface and then sends one to an interface that's up?

I will try get this for you soon.

regards
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
Who was that masked man?

Re: [patch 4/10] s390: network driver.

[Thomas Spatzier]

Paul Jakma <paul@clubi.ie> wrote on 10.12.2004 16:37:15:
> Thomas' original patch was to address this problem. I wonder could he
> recap the kernel side of this problem?

Here is why we submitted the original patch: We got reports from
several customers that their dynamic routing daemons got hung when
one network interface lost its physical connection. Some debugging
showed that the write queues of sockets went full and got blocked.
This was because we issued a netif_stop_queue when we detect a
cable pull or something.
As a solution, we removed the netif_stop_queue calls and just dropped
the packets + we increment the respective error counts in the
net_device_stats and call netif_carrier_off.
This solved the customer problems and seems to be right thing for
zebra etc.

Regards,
Thomas.

Re: [patch 4/10] s390: network driver.

[jamal]

On Tue, 2004-12-14 at 02:40, Thomas Spatzier wrote:
> 
> 
> Paul Jakma <paul@clubi.ie> wrote on 10.12.2004 16:37:15:
> > Thomas' original patch was to address this problem. I wonder could he
> > recap the kernel side of this problem?
> 
> Here is why we submitted the original patch: We got reports from
> several customers that their dynamic routing daemons got hung when
> one network interface lost its physical connection. Some debugging
> showed that the write queues of sockets went full and got blocked.
> This was because we issued a netif_stop_queue when we detect a
> cable pull or something.

I did some more thinking in the background and i wish to change my
opinion. What you see is Very Odd. I think there may be a bug upstream
at the socket layer or even before that - but doesnt sound like a device
level bug. Wasnt someone supposed to send a small proggie to Herbert?
When you netif_stop_queue you should never receive packets anymore
at the device level. If you receive any its a bug and you should drop
them and bitch violently. In other words i think what you have at the
moment is bandaid not the solution.

> As a solution, we removed the netif_stop_queue calls and just dropped
> the packets + we increment the respective error counts in the
> net_device_stats and call netif_carrier_off.
> This solved the customer problems and seems to be right thing for
> zebra etc.

We need to Fix this issue. Either your driver is doing something wrong
or something is broken upstackstream.
Can you describe how your driver uses the netif_start/stop/wake
interfaces?
Whoever promised to send that program to Herbert - please do.

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[Thomas Spatzier]

jamal <hadi@cyberus.ca> wrote on 15.12.2004 14:50:27:

> When you netif_stop_queue you should never receive packets anymore
> at the device level. If you receive any its a bug and you should drop
> them and bitch violently. In other words i think what you have at the
> moment is bandaid not the solution.

When we do a netif_stop_queue, we do not get any more packets.
So this behaviour is ok. The problem is that the socket write
queues fill up then and get blocked as soon as they are full.

> Can you describe how your driver uses the netif_start/stop/wake
> interfaces?

Before the patch we are talking about:
When we detect a cable pull (or something like this) we call
netif_stop_queue and set switch off the IFF_RUNNING flag.
Then when we detect that the cable is plugged in again, we
call netif_wake_queue and switch the IFF_RUNNING flag on.

And with the patch:
On cable pull we switch off IFF_RUNNING and call
netif_carrier_off. We still get packets but drop them.
When the cable is plugged in we switch on IFF_RUNNING and
call netif_carrier_on.

Regard,
Thomas.

Re: [patch 4/10] s390: network driver.

[jamal]

On Wed, 2004-12-15 at 10:03, Thomas Spatzier wrote:

> jamal <hadi@cyberus.ca> wrote on 15.12.2004 14:50:27:
> 
> > When you netif_stop_queue you should never receive packets anymore
> > at the device level. If you receive any its a bug and you should drop
> > them and bitch violently. In other words i think what you have at the
> > moment is bandaid not the solution.
> 
> When we do a netif_stop_queue, we do not get any more packets.
> So this behaviour is ok. The problem is that the socket write
> queues fill up then and get blocked as soon as they are full.
> 

This is the strange part. Anyone still willing to provide a sample
program that hangs?

> > Can you describe how your driver uses the netif_start/stop/wake
> > interfaces?
> 
> Before the patch we are talking about:
> When we detect a cable pull (or something like this) we call
> netif_stop_queue and set switch off the IFF_RUNNING flag.
> Then when we detect that the cable is plugged in again, we
> call netif_wake_queue and switch the IFF_RUNNING flag on.
> 

Not too bad except user space doesnt get async notification.

> And with the patch:
> On cable pull we switch off IFF_RUNNING and call
> netif_carrier_off. We still get packets but drop them.
> When the cable is plugged in we switch on IFF_RUNNING and
> call netif_carrier_on.

Ok, thats something you need to change.
Why you are setting that IFF_RUNNING flag?
Look at e1000 they do it right there. 

On link up:
	netif_carrier_on(netdev);
	netif_wake_queue(netdev);
On Link Down:
	netif_carrier_off(netdev); // wonder if these need swapping
	netif_stop_queue(netdev);

Still, I think we need to resolve the original problem.
And I have a feeling we wont be seeing any program which 
reproduces it ;->

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

jamal wrote:
> On Wed, 2004-12-15 at 10:03, Thomas Spatzier wrote:
> 
>>jamal <hadi@cyberus.ca> wrote on 15.12.2004 14:50:27:
>>
>>
>>>When you netif_stop_queue you should never receive packets anymore
>>>at the device level. If you receive any its a bug and you should drop
>>>them and bitch violently. In other words i think what you have at the
>>>moment is bandaid not the solution.
>>
>>When we do a netif_stop_queue, we do not get any more packets.
>>So this behaviour is ok. The problem is that the socket write
>>queues fill up then and get blocked as soon as they are full.
>>
> 
> This is the strange part. Anyone still willing to provide a sample
> program that hangs?

I haven't tried this myself, but surely it can happen when the
socket send-buffer is smaller than what can be queued up for
transmission: i.e. in the qdisc queue and device DMA ring.
And even more so, when sending to multiple devices from one socket.

> Look at e1000 they do it right there. 
> 
> On link up:
> 	netif_carrier_on(netdev);
> 	netif_wake_queue(netdev);
> On Link Down:
> 	netif_carrier_off(netdev); // wonder if these need swapping
> 	netif_stop_queue(netdev);
> 

Well, this is the same as what started this whole thread.
I believe that stopping the queue on link-down events is simply
bad behavior of the driver.

-Tommy

Re: [patch 4/10] s390: network driver.

[jamal]

On Sun, 2004-12-19 at 17:29, Tommy Christensen wrote:

> I haven't tried this myself, but surely it can happen when the
> socket send-buffer is smaller than what can be queued up for
> transmission: i.e. in the qdisc queue and device DMA ring.

Shouldnt this have to do with socket options? If you wish to block while
waiting for send, then you should be allowed.
For a routing protocol that actually is notified that the link went
down, it should probably flush those socket buffer at that point.

> And even more so, when sending to multiple devices from one socket.

Yes, this looks more within reason. I dont know what the answer to this
is. But would be helpful for someone to create a setup that reproduces
this.

> > Look at e1000 they do it right there. 
> > 
> > On link up:
> > 	netif_carrier_on(netdev);
> > 	netif_wake_queue(netdev);
> > On Link Down:
> > 	netif_carrier_off(netdev); // wonder if these need swapping
> > 	netif_stop_queue(netdev);
> > 
> 
> Well, this is the same as what started this whole thread.
> I believe that stopping the queue on link-down events is simply
> bad behavior of the driver.

>From what Thomas was saying, this is not what he was doing. Read his
email.

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

jamal wrote:
> On Sun, 2004-12-19 at 17:29, Tommy Christensen wrote:
> 
>>I haven't tried this myself, but surely it can happen when the
>>socket send-buffer is smaller than what can be queued up for
>>transmission: i.e. in the qdisc queue and device DMA ring.
> 
> Shouldnt this have to do with socket options? If you wish to block while
> waiting for send, then you should be allowed.
> For a routing protocol that actually is notified that the link went
> down, it should probably flush those socket buffer at that point.

OK. So is this the recommendation for these pour souls?

  - Use a socket for each device.
  - Set the socket buffer (SO_SNDBUF) large enough. E.g. 1 MB ?
    Or use non-blocking sockets - just in case.
  - If you care about not sending stale packets, it is the
    responsibility of the application to flush the socket on
    link-down events (by down'ing the interface?).

>>Well, this is the same as what started this whole thread.
>>I believe that stopping the queue on link-down events is simply
>>bad behavior of the driver.
> 
>>From what Thomas was saying, this is not what he was doing. Read his
> email.

It was at least to the same effect. The key issue is whether the
packets are kept in the queue (qdisc) until the link is back up,
or they are drained (and dropped) by the driver.

-Tommy

Re: [patch 4/10] s390: network driver.

[Jeff Garzik]

Tommy Christensen wrote:
> It was at least to the same effect. The key issue is whether the
> packets are kept in the queue (qdisc) until the link is back up,
> or they are drained (and dropped) by the driver.


They should absolutely not be dropped by the driver.  No ifs, ands, or buts.

This sort of problem is NOT solved by modifying hundreds of drivers.

	Jeff

Re: [patch 4/10] s390: network driver.

[jamal]

On Sun, 2004-12-19 at 18:46, Tommy Christensen wrote:

> OK. So is this the recommendation for these pour souls?
> 
>   - Use a socket for each device.

Sounds sensible (each device with IP enabled is more like it)

>   - Set the socket buffer (SO_SNDBUF) large enough. E.g. 1 MB ?
>     Or use non-blocking sockets - just in case.

I think we may need a socket "flush socket buffer" signal

>   - If you care about not sending stale packets, it is the
>     responsibility of the application to flush the socket on
>     link-down events (by down'ing the interface?).

sigh. I am begining to think this is too complex an approach.
It requires there be a way to automagically clean up the buffers
when things like this happen.

I beginuing to think thats the simplest way to achieve this: i.e not to
stop the queue but rather to let the packets continue showing up and
drop them at the driver when the link is down . The netlink async
carrier signal to the app is to be used to reroute instead of being a
signal to flush buffers. In other words the other Thomas got it right
(with the exception of setting the IFF_RUNNIGN flags)

Jeff? 

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[Jeff Garzik]

jamal wrote:
> I beginuing to think thats the simplest way to achieve this: i.e not to
> stop the queue but rather to let the packets continue showing up and
> drop them at the driver when the link is down . The netlink async
> carrier signal to the app is to be used to reroute instead of being a
> signal to flush buffers. In other words the other Thomas got it right
> (with the exception of setting the IFF_RUNNIGN flags)
> 
> Jeff? 


I haven't heard anything to convince me that the same change should be 
deployed across NNN drivers.  The drivers already signal the net core 
that the link is down; to me, that implies there should be code in _one_ 
place that handles this condition, not NNN places.

Further,
* if this policy ("drop them in the driver") ever changes, we must again 
touch NNN drivers

* dropping them in the driver but not stopping the queue means that the 
system is allowed to continue to stream data into the driver, only for 
the driver to free it.  That will scale -- right up to (worst case) 100% 
CPU, with userland sending packets as fast as it can, and the driver 
dropping packets as fast as it can.  The only places the net stack 
currently checks carrier is dev_get_flags() and dev_watchdog().

* If you need a hook to flush the in-hardware buffers, add such a hook. 
  Don't hack it in like this.  Yeah, adding a hook touches NNN drivers 
but at least the hook is far more self-contained, it's semantics will be 
more clear, and it will increase the likelihood that the driver changes 
do not affect the hot path nor current netif_{start,stop}_queue() logic.

	Jeff

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

Jeff Garzik wrote:
> I haven't heard anything to convince me that the same change should be 
> deployed across NNN drivers.  The drivers already signal the net core 
> that the link is down; to me, that implies there should be code in _one_ 
> place that handles this condition, not NNN places.

AFAICS only a handful of (newer) drivers call netif_stop_queue() directly.
Others may do this indirectly if the MAC stops taking packets from the
DMA ringbuffer. At least some MAC's/drivers certainly don't.

I've always thought of netif_stop_queue() as the replacement of the old
tbusy flag, signaling a transient condition where the HW is unable to
keep up with the flow of packets. And it seems to be used for just this
in most cases.
Perhaps somebody confused netif_stop_queue with dev_deactivate() ??

OK, another view on this: isn't is problematic to have skb's stuck in
the network stack "indefinitely" ?
They hold references to a dst_entry and a sock (and probably more).
So how about this for the FAQ:
  Q: Why can't I unload the af_packet module?
  A: Ohh, you'll have to plug in the darn cable to eth0 first!
*Please* tell me, I've got this all wrong.


-Tommy

Re: [patch 4/10] s390: network driver.

[Jeff Garzik]

Tommy Christensen wrote:
> Jeff Garzik wrote:
> 
>> I haven't heard anything to convince me that the same change should be 
>> deployed across NNN drivers.  The drivers already signal the net core 
>> that the link is down; to me, that implies there should be code in 
>> _one_ place that handles this condition, not NNN places.
> 
> 
> AFAICS only a handful of (newer) drivers call netif_stop_queue() directly.
> Others may do this indirectly if the MAC stops taking packets from the
> DMA ringbuffer. At least some MAC's/drivers certainly don't.

Incorrect.  Use of netif_stop_queue() is -required- to signal that the 
hardware cannot accept any more skbs from the system.

Far more than a "handful" and required for all but a few very strange 
drivers.


> OK, another view on this: isn't is problematic to have skb's stuck in
> the network stack "indefinitely" ?
> They hold references to a dst_entry and a sock (and probably more).
> So how about this for the FAQ:
>  Q: Why can't I unload the af_packet module?
>  A: Ohh, you'll have to plug in the darn cable to eth0 first!
> *Please* tell me, I've got this all wrong.

You've got this all wrong.

	Jeff

Re: [patch 4/10] s390: network driver.

[Thomas Spatzier]

Jeff Garzik <jgarzik@pobox.com> wrote on 20.12.2004 19:54:53:
> I haven't heard anything to convince me that the same change should be
> deployed across NNN drivers.  The drivers already signal the net core
> that the link is down; to me, that implies there should be code in _one_
> place that handles this condition, not NNN places.
>

This sounds plausible and I'm with Jeff here.
For me as the driver author it's the smallest change.
I will put it like this:
on cable gone:
      netif_stop_queue();
      netif_carrier_off();
on cable reconnected:
      netif_carrier_on();
      netif_wake_queue();

Is that ok, i.e. what all drivers do or should do?

For the problems that applications might have (i.e. sockets being
blocked etc.) another solution should be found.
And as Jeff pointed out, this should be a central solution and
not be implemented in drivers.

Regards,
Thomas.

Re: [patch 4/10] s390: network driver.

[Jeff Garzik]

On Wed, Dec 22, 2004 at 11:56:06AM +0100, Thomas Spatzier wrote:
> on cable reconnected:
>       netif_carrier_on();
>       netif_wake_queue();

Side note to all driver authors, make sure you only ever start or
wake the queue if there is truly space available for another skb.

	Jeff

Re: [patch 4/10] s390: network driver.

[jamal]

On Wed, 2004-12-22 at 05:56, Thomas Spatzier wrote:

> 
> Is that ok, i.e. what all drivers do or should do?
> 
> For the problems that applications might have (i.e. sockets being
> blocked etc.) another solution should be found.
> And as Jeff pointed out, this should be a central solution and
> not be implemented in drivers.
> 

I think this needs to be resolved too. 
It is possible to have a centralized action instead of requiring drivers
to make changes if we know the state of the driver is in netcarrier_off.
What that would require is 
on cable gone, you just say:
      netif_carrier_off();
and the top layer code will junk the packets before they hit the driver.
This way the socket code can continue sending whatever it wants but if
theres no link, then its fair to drop those packets?

If this acceptable i can generate a quick patch.

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[Thomas Spatzier]

jamal <hadi@cyberus.ca> wrote on 22.12.2004 14:48:28:
> I think this needs to be resolved too.
> It is possible to have a centralized action instead of requiring drivers
> to make changes if we know the state of the driver is in netcarrier_off.
> What that would require is
> on cable gone, you just say:
>       netif_carrier_off();
> and the top layer code will junk the packets before they hit the driver.
> This way the socket code can continue sending whatever it wants but if
> theres no link, then its fair to drop those packets?
>
> If this acceptable i can generate a quick patch.

Does Jamal's solution sound good for all? Then I would change my driver
to do the following:
just call netif_carrier_off() (not netif_stop_queue)

Then the upper layers will do the propper thing, so I should not
get any more packets. If I still get some, I will drop them.
Did I get this correctly?

BTW: A happy new year to all ;-)

Regards,
Thomas.

Re: [patch 4/10] s390: network driver.

[jamal]

The change is simple if theres consensus to go this path.

cheers,
jamal

On Mon, 2005-01-03 at 04:10, Thomas Spatzier wrote:
> 
> 
> jamal <hadi@cyberus.ca> wrote on 22.12.2004 14:48:28:
> > I think this needs to be resolved too.
> > It is possible to have a centralized action instead of requiring drivers
> > to make changes if we know the state of the driver is in netcarrier_off.
> > What that would require is
> > on cable gone, you just say:
> >       netif_carrier_off();
> > and the top layer code will junk the packets before they hit the driver.
> > This way the socket code can continue sending whatever it wants but if
> > theres no link, then its fair to drop those packets?
> >
> > If this acceptable i can generate a quick patch.
> 
> Does Jamal's solution sound good for all? Then I would change my driver
> to do the following:
> just call netif_carrier_off() (not netif_stop_queue)
> 
> Then the upper layers will do the propper thing, so I should not
> get any more packets. If I still get some, I will drop them.
> Did I get this correctly?
> 
> BTW: A happy new year to all ;-)
> 
> Regards,
> Thomas.
> 
> 

Re: [patch 4/10] s390: network driver.

[Jeff Garzik]

jamal wrote:
> The change is simple if theres consensus to go this path.

Can you resend your patch?

My main objection was that any change should be made in the core, not in 
individual net drivers.

Another objection was that it seemed that some of the proposed solutions 
for clearing the queue on link down were imposing app-specific policy on 
the overall kernel.

Aren't there cases where people would -not- want the queue to be cleared?

	Jeff

Re: [patch 4/10] s390: network driver.

[jamal]

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

On Tue, 2005-01-04 at 18:28, Jeff Garzik wrote:
> jamal wrote:
> > The change is simple if theres consensus to go this path.
> 
> Can you resend your patch?

I didnt send any patch - but heres one that looks right - havent tried
compiling it.

> My main objection was that any change should be made in the core, not in 
> individual net drivers.

Attached patch resolves that concern

> Another objection was that it seemed that some of the proposed solutions 
> for clearing the queue on link down were imposing app-specific policy on 
> the overall kernel.
>
> Aren't there cases where people would -not- want the queue to be cleared?

Thats a good question and your point of imposing policy in the kernel is
valid. I know that i would probably want packets to appear they are
going out when the wire is pulled from underneath me.
Theres possibly people who would want it differently - so for we havent
heard from them. Maybe poll far and wide - or push it in and wait for
them to whine.

cheers,
jamal

[-- Attachment #2: nc.p --]
[-- Type: text/plain, Size: 512 bytes --]

--- 2610-bk1/net/sched/sch_generic.c	2005/01/05 01:21:04	1.1
+++ 2610-bk1/net/sched/sch_generic.c	2005/01/05 01:36:26
@@ -152,7 +152,7 @@
 					spin_lock(&dev->queue_lock);
 					goto collision; 
 				}
-			}
+			} 
 
 			/* NETDEV_TX_BUSY - we need to requeue */
 			/* Release the driver */
@@ -162,6 +162,11 @@
 			} 
 			spin_lock(&dev->queue_lock);
 			q = dev->qdisc;
+			if (!netif_carrier_ok(dev)) {
+				kfree_skb(skb);
+				q->qstats.drops++;
+				return -1;
+			}
 		}
 
 		/* Device kicked us out :(

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Wed, 4 Jan 2005, jamal wrote:

> Theres possibly people who would want it differently - so for we 
> havent heard from them. Maybe poll far and wide - or push it in and 
> wait for them to whine.

We're whining.

This policy in the Linux kernel makes using Linux dangerous for 
certain routing applications (RIP, routing advertisements).

Name a non-TCP application that *does* want this newish kernel policy 
and tell me how it breaks without this *new* policy. ;)

> cheers,
> jamal

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
My only love sprung from my only hate!
Too early seen unknown, and known too late!
 		-- William Shakespeare, "Romeo and Juliet"

Re: [patch 4/10] s390: network driver.

[jamal]

On Wed, 2005-01-05 at 01:30, Paul Jakma wrote:
> On Wed, 4 Jan 2005, jamal wrote:
> 
> > Theres possibly people who would want it differently - so for we 
> > havent heard from them. Maybe poll far and wide - or push it in and 
> > wait for them to whine.
> 
> We're whining.
> 
> This policy in the Linux kernel makes using Linux dangerous for 
> certain routing applications (RIP, routing advertisements).
> 
> Name a non-TCP application that *does* want this newish kernel policy 
> and tell me how it breaks without this *new* policy. ;)

Ok, Iam confused - I thought you guys _wanted this_ ;->
The issue is about message obsolency more than it is about reliability.
Without this the scenario you played you played for us before was:

1)=>send a few LSAs from user space, pull cable before they go out
(this will happen if you are sending sufficiently large amounts of
packets i.e network is busy),
2)=>user space gets notified via netlink, device shuts down acces to
DMA, packets queued anyways and you have no ability to say "ooops,sorry
take that packet back"
a)You could do a move to another device at this point.
or
b) dumb app will continue sending

3)=>plug cable back in 2 minutes later, obsolete LSAs sent followed by
any new ones that may follow
....

With the patch, packets in #2 will be dropped. As a matter of fact
within those two minutes, if stopped, it is probable the device watchdog
timer will kick in and flush the DMA but not the scheduler queues above
it (which is where upto a 1000 stale packets could be sitting).

What is it that you dont like now? 

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Wed, 5 Jan 2005, jamal wrote:

> Ok, Iam confused - I thought you guys _wanted this_ ;->

I'm confused too now. We dont want "queue packets" - that's the 
'newish' policy I was referring to. (newish in quotes, as I'm not 
sure from when this behaviour dates).

> The issue is about message obsolency more than it is about reliability.

Right, exactly.

So we do want what you think we want ;) (i think).

> Without this the scenario you played you played for us before was:
>
> 1)=>send a few LSAs from user space, pull cable before they go out
> (this will happen if you are sending sufficiently large amounts of
> packets i.e network is busy),
> 2)=>user space gets notified via netlink, device shuts down acces to
> DMA, packets queued anyways and you have no ability to say "ooops,sorry
> take that packet back"

Right.

Two things:

- we noticed this behaviour because of OSPF

Users reported ospfd would cease to send packets on all interfaces, 
(with certain drivers) because /one/ interface was link-down.

We can workaround this easily by opening a socket per interface - at 
present we simply punt OSPF packets down a single raw socket and rely 
on IP_HDRINCL to have kernel route the packet out correct interface 
(IP_MULTICAST_IF for multicast destined packets).

- The queueing does not affect OSPF terribly, it would affect other
   protocols though

OSPF implements its own 'synchronisation' facilities between 
neighbours and can easily 'detect' obsolecent packets. So the 
obsolence issue does not affect it, routing-information in stale 
packets will not propogate, so they cant do much damage really. (just 
unneccessary to queue and send such packets).

However, other commonly used protocols are not as robust. Mostly 
those where a protocol is used to distribute routing information to 
passive listeners, eg:

- RIP
- IPv4 ICMP based router-discovery (IRDP)
- IPv6 Router-advertisements

In these cases, the queuing behaviour is potentially dangerous and 
could disrupt connectivity by propogating no-longer-valid routing 
information.

> a)You could do a move to another device at this point.
> or
> b) dumb app will continue sending
>
> 3)=>plug cable back in 2 minutes later, obsolete LSAs sent followed by
> any new ones that may follow

Right. Except OSPF is robust enough against stale packets. Other 
protocols are not.

> With the patch, packets in #2 will be dropped.

Perfect.

> As a matter of fact within those two minutes, if stopped, it is 
> probable the device watchdog timer will kick in and flush the DMA 
> but not the scheduler queues above it (which is where upto a 1000 
> stale packets could be sitting).

Right, and our argument it doesnt make sense to send those packets. 
I've never heard of any UDP and/or raw application that expected a 
kernel to queue packets if they could not be sent for lack of link or 
other problem, and any which did are surely broken by definition? ;)

> What is it that you dont like now?

Sorry, wires crossed re "new behaviour". The "new new" behaviour in 
the patch as you describe would be perfect.

PS: Another issue, could we have kernel space IP fragmentation for 
IP_HDRINCL sockets please? We currently have to implement 
fragmentation ourselves, which seems silly given that kernel already 
has this functionality.

> cheers,
> jamal

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
The early bird who catches the worm works for someone who comes in late
and owns the worm farm.
 		-- Travis McGee

Re: [patch 4/10] s390: network driver.

[jamal]

On Wed, 2005-01-05 at 09:29, Paul Jakma wrote:
> On Wed, 5 Jan 2005, jamal wrote:

[..]
> Sorry, wires crossed re "new behaviour". The "new new" behaviour in 
> the patch as you describe would be perfect.
> 
> PS: Another issue, could we have kernel space IP fragmentation for 
> IP_HDRINCL sockets please? We currently have to implement 
> fragmentation ourselves, which seems silly given that kernel already 
> has this functionality.

I will let some other fireman grab this bait ;-> 
If not i will revisit it after a unknown/random timeout.

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

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

jamal wrote:
> On Tue, 2005-01-04 at 18:28, Jeff Garzik wrote:
> 
>>jamal wrote:
>>
>>>The change is simple if theres consensus to go this path.
>>
>>Can you resend your patch?
> 
> 
> I didnt send any patch - but heres one that looks right - havent tried
> compiling it.

Thank you for diving into this, Jamal.

For the patch to have much effect, we need to check the carrier before
calling hard_start_xmit(). Like in the modified patch below.

>>My main objection was that any change should be made in the core, not in 
>>individual net drivers.
> 
> 
> Attached patch resolves that concern

Except for the drivers that call netif_stop_queue() on link-down. These
calls (and the corresponding netif_wake_queue) would have to be removed.


-Tommy

[-- Attachment #2: sch_generic.c.patch --]
[-- Type: text/plain, Size: 615 bytes --]

--- linux-2.6.10-bk7/net/sched/sch_generic.c	Wed Jan  5 16:27:13 2005
+++ linux-2.6.10-work/net/sched/sch_generic.c	Wed Jan  5 16:28:53 2005
@@ -134,7 +134,7 @@
 			/* And release queue */
 			spin_unlock(&dev->queue_lock);
 
-			if (!netif_queue_stopped(dev)) {
+			if (!netif_queue_stopped(dev) && netif_carrier_ok(dev)) {
 				int ret;
 				if (netdev_nit)
 					dev_queue_xmit_nit(skb, dev);
@@ -162,6 +162,11 @@
 			} 
 			spin_lock(&dev->queue_lock);
 			q = dev->qdisc;
+			if (!netif_carrier_ok(dev)) {
+				kfree_skb(skb);
+				q->qstats.drops++;
+				return -1;
+			}
 		}
 
 		/* Device kicked us out :(

Re: [patch 4/10] s390: network driver.

[jamal]

On Wed, 2005-01-05 at 10:35, Tommy Christensen wrote:
> jamal wrote:

> 
> Except for the drivers that call netif_stop_queue() on link-down. These
> calls (and the corresponding netif_wake_queue) would have to be removed.

If we assume all drivers do:
netif_stop then carrier_off then you dont need that extra check.
Thats the working assumption i had - maybe a comment is deserving or
we could say we dont think that all drivers are going to follow that 
sequence.

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

On Thu, 2005-01-06 at 14:58, jamal wrote:
> On Wed, 2005-01-05 at 10:35, Tommy Christensen wrote:
> > jamal wrote:
> 
> > 
> > Except for the drivers that call netif_stop_queue() on link-down. These
> > calls (and the corresponding netif_wake_queue) would have to be removed.
> 
> If we assume all drivers do:
> netif_stop then carrier_off then you dont need that extra check.
> Thats the working assumption i had - maybe a comment is deserving or
> we could say we dont think that all drivers are going to follow that 
> sequence.

But qdisc_restart() isn't called any more after the queue is
stopped. So how do we get to drain the packets?

Another approach could be to reset the qdisc (kind of what
dev_deactivate does) if the driver stays in queue_stopped
and carrier_off for some period of time.

-Tommy

Re: [patch 4/10] s390: network driver.

[jamal]

On Thu, 2005-01-06 at 10:06, Tommy Christensen wrote:

> But qdisc_restart() isn't called any more after the queue is
> stopped. So how do we get to drain the packets?

We return -1; so qdisc restart will be called until theres no packets
left on the queue. Did i miss something in the state machine there?

> Another approach could be to reset the qdisc (kind of what
> dev_deactivate does) if the driver stays in queue_stopped
> and carrier_off for some period of time.
> 

reseting qdisc could certainly be part of the device watchdog.
Note that once the packets are drained, the next thing that will happen
if device is stopped for a dev->timeout period is watchdog kicking in
and flushing the DMA. So you could reset your qdisc there though i am
not sure it will be needed with that patch.

cheers,
jamal 

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

On Fri, 2005-01-07 at 14:32, jamal wrote:
> On Thu, 2005-01-06 at 10:06, Tommy Christensen wrote:
> 
> > But qdisc_restart() isn't called any more after the queue is
> > stopped. So how do we get to drain the packets?
> 
> We return -1; so qdisc restart will be called until theres no packets
> left on the queue. Did i miss something in the state machine there?

Yes, qdisc_run does this
  while (!netif_queue_stopped(dev) && qdisc_restart(dev) < 0)
    /* NOTHING */;

> > Another approach could be to reset the qdisc (kind of what
> > dev_deactivate does) if the driver stays in queue_stopped
> > and carrier_off for some period of time.
> > 
> 
> reseting qdisc could certainly be part of the device watchdog.
> Note that once the packets are drained, the next thing that will happen
> if device is stopped for a dev->timeout period is watchdog kicking in
> and flushing the DMA. So you could reset your qdisc there though i am
> not sure it will be needed with that patch.

Unfortunately the watchdog won't do this when carrier is off, which
is fair enough since the chip isn't hung as such.

So, we would need another method to flush out stale packets from
the DMA ring. Some drivers already do this themselves, and some won't
need it because they keep on running even when link is down.

-Tommy

Re: [patch 4/10] s390: network driver.

[jamal]

On Fri, 2005-01-07 at 10:26, Tommy Christensen wrote:
> On Fri, 2005-01-07 at 14:32, jamal wrote:
> > On Thu, 2005-01-06 at 10:06, Tommy Christensen wrote:

[..]

> > reseting qdisc could certainly be part of the device watchdog.
> > Note that once the packets are drained, the next thing that will happen
> > if device is stopped for a dev->timeout period is watchdog kicking in
> > and flushing the DMA. So you could reset your qdisc there though i am
> > not sure it will be needed with that patch.
> 
> Unfortunately the watchdog won't do this when carrier is off, which
> is fair enough since the chip isn't hung as such.

We may have to kick the dog in that path as well then.

> So, we would need another method to flush out stale packets from
> the DMA ring. Some drivers already do this themselves, and some won't
> need it because they keep on running even when link is down.

kicking tx_timeout seems like the right thing to do until i looked at
the drivers. Sigh. I just stared at a few drivers and it is not pleasant
- and they mostly have the same theme (so you cant say they are
inconsistent).
Looking at e1000 for example is quiet interesting - it actually brings
down then up the device ;-> This will both flash the dma and reset qdisc
but also loose any static routes. It will also reschedule the device;-<
So i am not so sure that the dog kicking in a minute later is a good
thing at all. We may need a lot of janitorial work to cleanse the
drivers - something jgarzik is against. 

A qdisc_reset, as you suggest, will probably not cure the problem but
provide some cosmetics. Thoughts?

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[jamal]

Didnt see anymore discussion on this - have we given up on it?

cheers,
jamal

On Mon, 2005-01-10 at 08:18, jamal wrote:
> On Fri, 2005-01-07 at 10:26, Tommy Christensen wrote:
> > On Fri, 2005-01-07 at 14:32, jamal wrote:
> > > On Thu, 2005-01-06 at 10:06, Tommy Christensen wrote:
> 
> [..]
> 
> > > reseting qdisc could certainly be part of the device watchdog.
> > > Note that once the packets are drained, the next thing that will happen
> > > if device is stopped for a dev->timeout period is watchdog kicking in
> > > and flushing the DMA. So you could reset your qdisc there though i am
> > > not sure it will be needed with that patch.
> > 
> > Unfortunately the watchdog won't do this when carrier is off, which
> > is fair enough since the chip isn't hung as such.
> 
> We may have to kick the dog in that path as well then.
> 
> > So, we would need another method to flush out stale packets from
> > the DMA ring. Some drivers already do this themselves, and some won't
> > need it because they keep on running even when link is down.
> 
> kicking tx_timeout seems like the right thing to do until i looked at
> the drivers. Sigh. I just stared at a few drivers and it is not pleasant
> - and they mostly have the same theme (so you cant say they are
> inconsistent).
> Looking at e1000 for example is quiet interesting - it actually brings
> down then up the device ;-> This will both flash the dma and reset qdisc
> but also loose any static routes. It will also reschedule the device;-<
> So i am not so sure that the dog kicking in a minute later is a good
> thing at all. We may need a lot of janitorial work to cleanse the
> drivers - something jgarzik is against. 
> 
> A qdisc_reset, as you suggest, will probably not cure the problem but
> provide some cosmetics. Thoughts?
> 
> cheers,
> jamal
> 
> 
> 

Re: [patch 4/10] s390: network driver.

[Hasso Tepper]

jamal wrote:
> Didnt see anymore discussion on this - have we given up on it?

I'm not competent enough about kernel internals to decide how this should be 
done in kernel. But I would like to clarify situation for me as Quagga 
developer.

All this work is done to avoid sending stale packets to the network? Ie. 
there will be no change regarding socket blocking issue? Detecting carrier 
on/off and/or socket per interface is must be for us in user space to avoid 
socket blocking? I'm just trying to have full picture so I can put my 
priorities in place for next release.


Thanks for your support,

-- 
Hasso Tepper
Elion Enterprises Ltd.
WAN administrator

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

Hasso Tepper wrote:
> All this work is done to avoid sending stale packets to the network? Ie. 
> there will be no change regarding socket blocking issue? Detecting carrier 
> on/off and/or socket per interface is must be for us in user space to avoid 
> socket blocking? I'm just trying to have full picture so I can put my 
> priorities in place for next release.

A proper solution would take care of both these issues: "stale packets"
and "socket blocking". They are pretty much related.

Using a socket for each interface *ought* not be needed.
Carrier detection sounds like a good thing in its own right.

Hope this helps,
Tommy

Re: [patch 4/10] s390: network driver.

[Peter Buckingham]

Hi,

Tommy Christensen wrote:
> A proper solution would take care of both these issues: "stale packets"
> and "socket blocking". They are pretty much related.
> 
> Using a socket for each interface *ought* not be needed.
> Carrier detection sounds like a good thing in its own right.

I came across this same problem with multicast/unicast with an e1000. 
For a quick hack we just check to see whether the carrier is ok, if it's 
not we just drop the packet. this might do some nasty things with tcp 
and it may be better to check to see wither the socket buffer is full, 
but this works for me here... (this is against a 2.6.9ish kernel)

peter

Index: net/core/dev.c
===================================================================
--- net/core/dev.c
+++ net/core/dev.c
@@ -1379,6 +1379,11 @@
  #ifdef CONFIG_NET_CLS_ACT
  	skb->tc_verd = SET_TC_AT(skb->tc_verd,AT_EGRESS);
  #endif
+	if (!netif_carrier_ok(dev)) {
+		rc = NET_XMIT_DROP;
+		goto out_kfree_skb;
+	}
+
  	if (q->enqueue) {
  		/* Grab device queue */
  		spin_lock(&dev->queue_lock);

Re: [patch 4/10] s390: network driver.

[jamal]

Hi there,

The solution that Tommy and myself have been batting around is a lot
more cleaner than this (however still doesnt solve the issue 100%)

cheers,
jamal

On Mon, 2005-01-17 at 17:13, Peter Buckingham wrote:
> Hi,
> 
> Tommy Christensen wrote:
> > A proper solution would take care of both these issues: "stale packets"
> > and "socket blocking". They are pretty much related.
> > 
> > Using a socket for each interface *ought* not be needed.
> > Carrier detection sounds like a good thing in its own right.
> 
> I came across this same problem with multicast/unicast with an e1000. 
> For a quick hack we just check to see whether the carrier is ok, if it's 
> not we just drop the packet. this might do some nasty things with tcp 
> and it may be better to check to see wither the socket buffer is full, 
> but this works for me here... (this is against a 2.6.9ish kernel)
> 
> peter
> 
> Index: net/core/dev.c
> ===================================================================
> --- net/core/dev.c
> +++ net/core/dev.c
> @@ -1379,6 +1379,11 @@
>   #ifdef CONFIG_NET_CLS_ACT
>   	skb->tc_verd = SET_TC_AT(skb->tc_verd,AT_EGRESS);
>   #endif
> +	if (!netif_carrier_ok(dev)) {
> +		rc = NET_XMIT_DROP;
> +		goto out_kfree_skb;
> +	}
> +
>   	if (q->enqueue) {
>   		/* Grab device queue */
>   		spin_lock(&dev->queue_lock);
> 
> 

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

Peter Buckingham wrote:
> Hi,
> 
> Tommy Christensen wrote:
> 
>> A proper solution would take care of both these issues: "stale packets"
>> and "socket blocking". They are pretty much related.
>>
>> Using a socket for each interface *ought* not be needed.
>> Carrier detection sounds like a good thing in its own right.
> 
> 
> I came across this same problem with multicast/unicast with an e1000. 
> For a quick hack we just check to see whether the carrier is ok, if it's 
> not we just drop the packet. this might do some nasty things with tcp 
> and it may be better to check to see wither the socket buffer is full, 
> but this works for me here... (this is against a 2.6.9ish kernel)

Thanks for the input!

The downside of this solution is that it doesn't do anything with the
packets already queued for tx (in the qdisc queue and in the DMA ring).
OTOH it probably works just fine in real life (!).

Also, we may not want to handle all devices alike, as this patch does.

-Tommy

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

jamal wrote:
> Didnt see anymore discussion on this - have we given up on it?

It is not forgotten - just stalled ...
I'll try to look at it some more.

-Tommy

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

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

Tommy Christensen wrote:
> jamal wrote:
> 
>> Didnt see anymore discussion on this - have we given up on it?
> 
> 
> It is not forgotten - just stalled ...
> I'll try to look at it some more.

Sorry for the long delay.

Jamal, do you think something like this could work?

Flushing of the DMA ring could be added if the need arises.
e1000 does this already in the driver on link-down.

-Tommy


[-- Attachment #2: carrier.patch --]
[-- Type: text/plain, Size: 1671 bytes --]

diff -ur linux-2.6.11-rc2-bk8/net/core/link_watch.c linux-2.6.11-work/net/core/link_watch.c
--- linux-2.6.11-rc2-bk8/net/core/link_watch.c	2005-01-30 22:08:44.000000000 +0100
+++ linux-2.6.11-work/net/core/link_watch.c	2005-01-31 00:17:45.716039247 +0100
@@ -16,6 +16,7 @@
 #include <linux/netdevice.h>
 #include <linux/if.h>
 #include <net/sock.h>
+#include <net/pkt_sched.h>
 #include <linux/rtnetlink.h>
 #include <linux/jiffies.h>
 #include <linux/spinlock.h>
@@ -74,6 +75,9 @@
 		clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
 
 		if (dev->flags & IFF_UP) {
+			if (netif_carrier_ok(dev) &&
+			    dev->qdisc_sleeping != &noop_qdisc)
+				dev_activate(dev);
 			netdev_state_change(dev);
 		}
 
diff -ur linux-2.6.11-rc2-bk8/net/sched/sch_generic.c linux-2.6.11-work/net/sched/sch_generic.c
--- linux-2.6.11-rc2-bk8/net/sched/sch_generic.c	2005-01-30 22:11:50.458216314 +0100
+++ linux-2.6.11-work/net/sched/sch_generic.c	2005-01-31 00:06:47.609769835 +0100
@@ -185,6 +185,7 @@
 static void dev_watchdog(unsigned long arg)
 {
 	struct net_device *dev = (struct net_device *)arg;
+	int check_carrier = 0;
 
 	spin_lock(&dev->xmit_lock);
 	if (dev->qdisc != &noop_qdisc) {
@@ -198,10 +199,23 @@
 			}
 			if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
 				dev_hold(dev);
-		}
+		} else
+			check_carrier = 1;
 	}
 	spin_unlock(&dev->xmit_lock);
 
+	if (check_carrier) {
+		spin_lock(&dev->queue_lock);
+		if (!netif_carrier_ok(dev) && netif_queue_stopped(dev)) {
+			struct Qdisc *qdisc;
+
+			qdisc = dev->qdisc;
+			dev->qdisc = &noop_qdisc;
+			qdisc_reset(qdisc);
+		}
+		spin_unlock(&dev->queue_lock);
+	}
+
 	dev_put(dev);
 }
 

Re: [patch 4/10] s390: network driver.

[jamal]

Tommy,

I wasnt sure why you did:

dev->qdisc = &noop_qdisc;

You should probably save the old qdisc in qdisc_sleeping instead
and restore it on wakeup - otherwise you always end with default qdisc.
check_carrier should probably just call dev_activate which does all you
wanted, no?

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[jamal]

On Sun, 2005-01-30 at 19:09, jamal wrote:
> Tommy,
> 
> I wasnt sure why you did:
> 
> dev->qdisc = &noop_qdisc;
> 
> You should probably save the old qdisc in qdisc_sleeping instead
> and restore it on wakeup - otherwise you always end with default qdisc.
> check_carrier should probably just call dev_activate

meant dev_deactivate - watch them locks though ..

cheers,
jamal

>  which does all you
> wanted, no?
> 
> cheers,
> jamal

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

jamal wrote:
> Tommy,
> 
> I wasnt sure why you did:
> 
> dev->qdisc = &noop_qdisc;
> 
> You should probably save the old qdisc in qdisc_sleeping instead
> and restore it on wakeup - otherwise you always end with default qdisc.

I think normally we will have qdisc == qdisc_sleeping. At least this
is how I read the code in dev_graft_qdisc(). When can they differ?

> check_carrier should probably just call dev_activate which does all you
> wanted, no?

I copied the logic from dev_deactivate(), but I didn't want to include
the waiting parts, since this is not in process context. OK?

-Tommy

Re: [patch 4/10] s390: network driver.

[jamal]

On Sun, 2005-01-30 at 19:31, Tommy Christensen wrote:
> jamal wrote:
> > Tommy,
> > 
> > I wasnt sure why you did:
> > 
> > dev->qdisc = &noop_qdisc;
> > 
> > You should probably save the old qdisc in qdisc_sleeping instead
> > and restore it on wakeup - otherwise you always end with default qdisc.
> 
> I think normally we will have qdisc == qdisc_sleeping. At least this
> is how I read the code in dev_graft_qdisc(). When can they differ?

You are correct.

> > check_carrier should probably just call dev_activate which does all you
> > wanted, no?
> 
> I copied the logic from dev_deactivate(), but I didn't want to include
> the waiting parts, since this is not in process context. OK?

Same here.
It does look pretty sane.. Tested?

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[Tommy Christensen]

On Mon, 2005-01-31 at 04:26, jamal wrote:
> It does look pretty sane.. Tested?

Nope, I don't have access to any relevant HW. Hopefully someone
else can give it some beating. Please.

A simple test could be:
 o start tcpdump on host B
 o start pinging from host A to B
 o unplug cable on host A
 o wait a while and then plug the cable back in

This is what I get on an embedded PPC:
01:41:27.712729 IP 192.168.6.22 > 192.168.27.119: icmp 64: echo request
seq 7
01:41:28.712697 IP 192.168.6.22 > 192.168.27.119: icmp 64: echo request
seq 8
01:41:43.712240 IP 192.168.6.22 > 192.168.27.119: icmp 64: echo request
seq 23
01:41:44.712210 IP 192.168.6.22 > 192.168.27.119: icmp 64: echo request
seq 24

The interesting point is that the jump in sequence number coincides
with the jump in rx time. Theory is that not all drivers will behave
like this.

Hopefully someone can verify that with e.g. an e1000, and then check
whether the patch I send makes any difference.

-Tommy

Re: [patch 4/10] s390: network driver.

[Hasso Tepper]

Tommy Christensen wrote:
> On Mon, 2005-01-31 at 04:26, jamal wrote:
> > It does look pretty sane.. Tested?
>
> Nope, I don't have access to any relevant HW. Hopefully someone
> else can give it some beating. Please.

Sorry for long delay. I lost track in this discussion, was busy with other 
things, but found finally time to test this.

> Hopefully someone can verify that with e.g. an e1000, and then check
> whether the patch I send makes any difference.

Yes, confirmed that patch makes difference and solves Quagga ospfd problem 
(from what it all started :).

Thank you very much!


PS. Patch is tested with 2.4.29 as well, but it requires link watch stuff to 
work. Complete (link-watch + vlan carrier detection + this fix) is here - 
http://hasso.linux.ee/quagga/2.4.29-link-watch.patch

-- 
Hasso Tepper
Elion Enterprises Ltd.
WAN administrator

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Tue, 4 Jan 2005, Jeff Garzik wrote:

> Aren't there cases where people would -not- want the queue to be 
> cleared?

Why *do* you want the queue to be cleared? (avoiding NIC /dev/null is 
only reason right?)

TCP (AIUI) has its owns means to resend.

Most (all?) other transports have *never* had an expectation of such 
reliability. Applications *know* they have to provide their own 
reliability. Queueing and later sending (by then) stale packets is 
*bad*

Think:

- heartbeat applications
- Router or route advertisements (IPv6 RA, IPv4 ICMP IRDP, RIP)
- Streaming media (ok, not much damage here, but still there's simply
   no point queueing while link is down..)

Why queue packets on behalf of applications which have no expectation 
of kernel doing this (any application expecting such is certainlty 
broken) and which very likely implement their own reliability or 
packet-loss recovery strategies? Particularly when queueing is quite 
possibly *detrimental* to what the application is trying to achieve 
(timely sending of packets).

If an application wants reliable sending of packets, it will be using 
TCP..

> 	Jeff

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
When the speaker and he to whom he is speaks do not understand, that is
metaphysics.
 		-- Voltaire

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Mon, 20 Dec 2004, Tommy Christensen wrote:

>> For a routing protocol that actually is notified that the link 
>> went down, it should probably flush those socket buffer at that 
>> point.

Or why not return an error, as soon as possible on the socket, eg 
ENOBUFS, and discard anything in the queue before that. Make it 
configurable via a sockopt if you think it'd harm ordinary apps 
(though, anything that cant deal with ENOBUFS is broken already, 
really..) or make it apply only to nonblock sockets.

>   responsibility of the application to flush the socket on
>   link-down events (by down'ing the interface?).

That seems more complex than needs be, for userspace at least.

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
"Paul Lynde to block..."
-- a contestant on "Hollywood Squares"

Re: [patch 4/10] s390: network driver.

[Jeff Garzik]

Paul Jakma wrote:
>>   responsibility of the application to flush the socket on
>>   link-down events (by down'ing the interface?).
> 
> 
> That seems more complex than needs be, for userspace at least.

It is the responsibility of the kernel to push complexity to userland.

Some applications may NOT desire that the socket be flushed.  That's an 
app policy decision.

If this is the core issue, then I am even more inclined to think that 
the kernel is not what needs to be modified here.

	Jeff

Re: [patch 4/10] s390: network driver.

[Herbert Xu]

Tommy Christensen <tommy.christensen@tpack.net> wrote:
> 
> OK. So is this the recommendation for these pour souls?
> 
>  - Use a socket for each device.

Please show us your program first.  Then we can figure out
whether this is necessary or not.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [patch 4/10] s390: network driver.

[Jeff Garzik]

jamal wrote:
> On Wed, 2004-12-15 at 10:03, Thomas Spatzier wrote:
> 
> 
>>jamal <hadi@cyberus.ca> wrote on 15.12.2004 14:50:27:
>>
>>
>>>When you netif_stop_queue you should never receive packets anymore
>>>at the device level. If you receive any its a bug and you should drop
>>>them and bitch violently. In other words i think what you have at the
>>>moment is bandaid not the solution.
>>
>>When we do a netif_stop_queue, we do not get any more packets.
>>So this behaviour is ok. The problem is that the socket write
>>queues fill up then and get blocked as soon as they are full.
>>
> 
> 
> This is the strange part. Anyone still willing to provide a sample
> program that hangs?
> 
> 
>>>Can you describe how your driver uses the netif_start/stop/wake
>>>interfaces?
>>
>>Before the patch we are talking about:
>>When we detect a cable pull (or something like this) we call
>>netif_stop_queue and set switch off the IFF_RUNNING flag.
>>Then when we detect that the cable is plugged in again, we
>>call netif_wake_queue and switch the IFF_RUNNING flag on.
>>
> 
> 
> Not too bad except user space doesnt get async notification.
> 
> 
>>And with the patch:
>>On cable pull we switch off IFF_RUNNING and call
>>netif_carrier_off. We still get packets but drop them.
>>When the cable is plugged in we switch on IFF_RUNNING and
>>call netif_carrier_on.
> 
> 
> Ok, thats something you need to change.
> Why you are setting that IFF_RUNNING flag?
> Look at e1000 they do it right there. 
> 
> On link up:
> 	netif_carrier_on(netdev);
> 	netif_wake_queue(netdev);
> On Link Down:
> 	netif_carrier_off(netdev); // wonder if these need swapping
> 	netif_stop_queue(netdev);
> 
> Still, I think we need to resolve the original problem.
> And I have a feeling we wont be seeing any program which 
> reproduces it ;->


I've been watching this thread, and am still waiting to see a good, 
isolated test case.

My initial reaction was based on an observation that
(a) the proposed s390 change creates a CPU cycle soaker, a /dev/null for 
skbs.
(b) it really sounds like the userland program is doing something broken

Even if (b) is not true, the change is unacceptable due to (a) regardless.

	Jeff

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Sun, 19 Dec 2004, jamal wrote:

> This is the strange part. Anyone still willing to provide a sample
> program that hangs?

I can provide instructions, or you can wait a wee bit - I didnt have 
any e1000 hardware to test with (e1000 being one of the drivers which 
has this behavious, AFAIK/TTBOMK) - but a computer with an e1000 
arrived Friday. So, give me a bit and I'll try come up with a test 
case. A simple UDP send should be enough.

> Still, I think we need to resolve the original problem.
> And I have a feeling we wont be seeing any program which
> reproduces it ;->

Just send a UDP packet - AIUI when link goes down the application can 
(or at least a window exists) where the application can send a packet 
down the file descriptor without error, but it will be queued rather 
than sent and the fd eventually blocks/returns EAGAIN.

That window is the problem.

I'll test tomorrow.

> cheers,
> jamal

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
You want to know why I kept getting promoted?  Because my mouth knows more
than my brain.
 	-- W.G.

Re: [patch 4/10] s390: network driver.

[jamal]

On Sun, 2004-12-19 at 18:54, Paul Jakma wrote:
> On Sun, 19 Dec 2004, jamal wrote:
> 
> > This is the strange part. Anyone still willing to provide a sample
> > program that hangs?
> 
> I can provide instructions, or you can wait a wee bit - I didnt have 
> any e1000 hardware to test with (e1000 being one of the drivers which 
> has this behavious, AFAIK/TTBOMK) - but a computer with an e1000 
> arrived Friday. So, give me a bit and I'll try come up with a test 
> case. A simple UDP send should be enough.

When you are ready let me know, we could repeat what i said in my last
email.

cheers,
jamal

Re: [patch 4/10] s390: network driver.

[jamal]

On Mon, 2004-12-06 at 09:42, Hasso Tepper wrote:

> I'm trying to summarize. The approach - one raw socket to send/receive 
> messages no matter how many interfaces are used - worked for Quagga/Zebra 
> routing software daemons till now. If some of these interfaces was 
> operationally down, socket wasn't blocked even if the queue was full.
>
>  In 
> fact "man sendmsg" has still text:
> 
> ENOBUFS
> 	The output queue for a network interface was full.  This  gener-
> 	ally  indicates  that the interface has stopped sending, but may
> 	be caused by transient congestion.   (Normally,  this  does  not
> 	occur  in Linux. Packets are just silently dropped when a device
> 	queue overflows.)
> 
> Seems that it's no longer true. Seems that kernel is now trying as hard as 
> possible not to loose any data - data is queued and if the queue will be 
> full, all related sockets will be blocked to notify application.

We need to investigate why this happens. It doesnt sound like good
behavior neither legit.
Does this happen to only raw sockets? Herbert asked for a sample small
program that reproduces it.

>  So, one 
> socket approach don't work any more for Quagga/Zebra. No problem, we can 
> take the "one socket per interface" approach. And we already have link 
> detection implemented to notify daemons.
> 

I dont think it would be necessary with latest changes to notifications
in 2.6.x

> But there will be still potential problem - sending the "interface down" 
> message from kernel to the zebra daemon and then to the routing protocol 
> daemon takes time. And during this time daemon can send packets which will 
> sit in queue and may cause many problems if sent to the network later (if 
> link comes up again). Think about statelss routing protocols like rip(ng), 
> ipv6 router advertisements etc. They may carry the info that's no longer 
> true causing routing loops etc.
> 
> And to clarify a little bit: no - the Quagga/Zebra didn't work with previous 
> approach perfectly. I fact with link detection and socket per interface it 
> will be better than ever no matter what's the kernel behaviour. We just 
> want to make sure that solution will be bulletproof.
> 
> So, problem - how can we make sure that no potentially dangerous aged 
> (routing) info will be in the network?

I think the idea of having driver junk packets when link is down does
sound plausible as a solution.

cheers,
jamal 

Re: [patch 4/10] s390: network driver.

[Paul Jakma]

On Mon, 6 Dec 2004, jamal wrote:

> Dont post networking related patches on other lists. I havent seen said
> patch, but it seems someone is complaining about some behavior changing?

I missed the beginning of the thread too, but saw Jeff's reply to 
Thomas on netdev. It appears the original patch was to make the s390 
network driver discard packets on link-down.

Jeff had replied to say this was bad, that queues are meant to fill 
and that this was what other drivers (e1000, tg3) did.

> In regards to link down and packets being queued. Agreed this is a 
> little problematic for some apps/transports.

Tis yes. Particularly for apps using raw and UDP+IP_HDRINCL sockets.

This problem came to light when we got reports of ospfd blocking 
because link was down, late in 2.4 with a certain version of the 
(iirc) e100 driver. ospfd uses one single socket for all interfaces, 
and relies on IP_HDRINCL to have the packet routed out right 
interface. However this approach doesnt play well if the socket can 
be blocked completely because of /one/ interface having its link 
down. The behaviour we expected (and got up until now) is to receive 
either ENOBUFS or else, if the kernel accepts the packet write, for 
it to drop it if it can not be sent.

We can work around that by moving to a socket/interface. However it 
still leaves the problem of packets being queued indefinitely while 
the link is down and being sent when link comes back. This is *not* 
good for RIP, IPv4 IRDP and IPv6 RA.

> In the case the netdevice is administratively downed both the qdisc 
> and DMA ring packets are flushed.

What about any packets remaining in the socket buffer? (if that makes 
sense - i dont know enough about internals sadly). Are those queued?

> Newer packets will never be queued

That no longer appears to be the case though. The socket blocks, and 
/some/ packets are queued (presumably those which still were in the 
socket buffer? i dont know exactly..).

> and you should quickly be able to find from your app that 
> the device is down.

We can yes, via rtnetlink - but impossible to guarantee we'll know 
the link is down before we try write a packet.

> In the case of netdevice being operationally down

?

As in 'ip link set dev ... down'?

> - I am hoping this is what the discussion is, having jumped on it -

No, its for link-down, AIUI.

> both queues stay intact. What you can do is certainly from user 
> space admin down/up the device when you receive a netlink carrier 
> off notification.

That seems possible, but quite a hack. Something to work at a socket 
level would possibly be nicer. (Socket being the primary handle our 
application has).

> I am struggling to see whether dropping the packet inside the tx 
> path once it is operationaly down is so blasphemous ... need to get 
> caffeine first.

As long as reliable transports have some other transport specific 
queue, shouldnt be a problem. For UDP and raw no reliability or 
guarantees are expected by applications (least shouldnt be), and 
queueing some packets on link-down interferes with application-layer 
expectations.

> cheers,
> jamal

regards,
-- 
Paul Jakma	paul@clubi.ie	paul@jakma.org	Key ID: 64A2FF6A
Fortune:
The UPS doesn't have a battery backup.