Netdev List
 help / color / mirror / Atom feed
* [PATCH 2.6.18 1/3] tg3: Remove unnecessary tx_lock
From: Michael Chan @ 2006-06-05 19:47 UTC (permalink / raw)
  To: davem; +Cc: herbert, jgarzik, netdev

Remove tx_lock where it is unnecessary. tg3 runs lockless and so it
requires interrupts to be disabled and sync'ed, netif_queue and NAPI
poll to be stopped before the device can be reconfigured. After
stopping everything, it is no longer necessary to get the tx_lock.

Signed-off-by: Michael Chan <mchan@broadcom.com>


diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index f2382aa..4cda2af 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -2984,9 +2984,7 @@ static void tg3_tx_recover(struct tg3 *t
 	       "and include system chipset information.\n", tp->dev->name);
 
 	spin_lock(&tp->lock);
-	spin_lock(&tp->tx_lock);
 	tp->tg3_flags |= TG3_FLAG_TX_RECOVERY_PENDING;
-	spin_unlock(&tp->tx_lock);
 	spin_unlock(&tp->lock);
 }
 
@@ -3429,12 +3427,10 @@ static inline void tg3_full_lock(struct 
 	if (irq_sync)
 		tg3_irq_quiesce(tp);
 	spin_lock_bh(&tp->lock);
-	spin_lock(&tp->tx_lock);
 }
 
 static inline void tg3_full_unlock(struct tg3 *tp)
 {
-	spin_unlock(&tp->tx_lock);
 	spin_unlock_bh(&tp->lock);
 }
 



^ permalink raw reply related

* [PATCH 2.6.18 2/3] tg3: Convert to non-LLTX
From: Michael Chan @ 2006-06-05 19:47 UTC (permalink / raw)
  To: davem; +Cc: herbert, jgarzik, netdev

Herbert Xu pointed out that it is unsafe to call netif_tx_disable()
from LLTX drivers because it uses dev->xmit_lock to synchronize
whereas LLTX drivers use private locks.

Convert tg3 to non-LLTX to fix this issue. tg3 is a lockless driver
where hard_start_xmit and tx completion handling can run concurrently
under normal conditions. A tx_lock is only needed to prevent
netif_stop_queue and netif_wake_queue race condtions when the queue
is full.

So whether we use LLTX or non-LLTX, it makes practically no
difference.

Signed-off-by: Michael Chan <mchan@broadcom.com>


diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 4cda2af..f085890 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -3759,14 +3759,11 @@ static int tg3_start_xmit(struct sk_buff
 
 	len = skb_headlen(skb);
 
-	/* No BH disabling for tx_lock here.  We are running in BH disabled
-	 * context and TX reclaim runs via tp->poll inside of a software
+	/* We are running in BH disabled context with netif_tx_lock
+	 * and TX reclaim runs via tp->poll inside of a software
 	 * interrupt.  Furthermore, IRQ processing runs lockless so we have
 	 * no IRQ context deadlocks to worry about either.  Rejoice!
 	 */
-	if (!spin_trylock(&tp->tx_lock))
-		return NETDEV_TX_LOCKED;
-
 	if (unlikely(TX_BUFFS_AVAIL(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {
 		if (!netif_queue_stopped(dev)) {
 			netif_stop_queue(dev);
@@ -3775,7 +3772,6 @@ static int tg3_start_xmit(struct sk_buff
 			printk(KERN_ERR PFX "%s: BUG! Tx Ring full when "
 			       "queue awake!\n", dev->name);
 		}
-		spin_unlock(&tp->tx_lock);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -3858,15 +3854,16 @@ static int tg3_start_xmit(struct sk_buff
 	tw32_tx_mbox((MAILBOX_SNDHOST_PROD_IDX_0 + TG3_64BIT_REG_LOW), entry);
 
 	tp->tx_prod = entry;
-	if (TX_BUFFS_AVAIL(tp) <= (MAX_SKB_FRAGS + 1)) {
+	if (unlikely(TX_BUFFS_AVAIL(tp) <= (MAX_SKB_FRAGS + 1))) {
+		spin_lock(&tp->tx_lock);
 		netif_stop_queue(dev);
 		if (TX_BUFFS_AVAIL(tp) > TG3_TX_WAKEUP_THRESH)
 			netif_wake_queue(tp->dev);
+		spin_unlock(&tp->tx_lock);
 	}
 
 out_unlock:
     	mmiowb();
-	spin_unlock(&tp->tx_lock);
 
 	dev->trans_start = jiffies;
 
@@ -3885,14 +3882,11 @@ static int tg3_start_xmit_dma_bug(struct
 
 	len = skb_headlen(skb);
 
-	/* No BH disabling for tx_lock here.  We are running in BH disabled
-	 * context and TX reclaim runs via tp->poll inside of a software
+	/* We are running in BH disabled context with netif_tx_lock
+	 * and TX reclaim runs via tp->poll inside of a software
 	 * interrupt.  Furthermore, IRQ processing runs lockless so we have
 	 * no IRQ context deadlocks to worry about either.  Rejoice!
 	 */
-	if (!spin_trylock(&tp->tx_lock))
-		return NETDEV_TX_LOCKED; 
-
 	if (unlikely(TX_BUFFS_AVAIL(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {
 		if (!netif_queue_stopped(dev)) {
 			netif_stop_queue(dev);
@@ -3901,7 +3895,6 @@ static int tg3_start_xmit_dma_bug(struct
 			printk(KERN_ERR PFX "%s: BUG! Tx Ring full when "
 			       "queue awake!\n", dev->name);
 		}
-		spin_unlock(&tp->tx_lock);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -4039,15 +4032,16 @@ static int tg3_start_xmit_dma_bug(struct
 	tw32_tx_mbox((MAILBOX_SNDHOST_PROD_IDX_0 + TG3_64BIT_REG_LOW), entry);
 
 	tp->tx_prod = entry;
-	if (TX_BUFFS_AVAIL(tp) <= (MAX_SKB_FRAGS + 1)) {
+	if (unlikely(TX_BUFFS_AVAIL(tp) <= (MAX_SKB_FRAGS + 1))) {
+		spin_lock(&tp->tx_lock);
 		netif_stop_queue(dev);
 		if (TX_BUFFS_AVAIL(tp) > TG3_TX_WAKEUP_THRESH)
 			netif_wake_queue(tp->dev);
+		spin_unlock(&tp->tx_lock);
 	}
 
 out_unlock:
     	mmiowb();
-	spin_unlock(&tp->tx_lock);
 
 	dev->trans_start = jiffies;
 
@@ -11332,7 +11326,6 @@ static int __devinit tg3_init_one(struct
 	SET_MODULE_OWNER(dev);
 	SET_NETDEV_DEV(dev, &pdev->dev);
 
-	dev->features |= NETIF_F_LLTX;
 #if TG3_VLAN_TAG_USED
 	dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
 	dev->vlan_rx_register = tg3_vlan_rx_register;
diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
index 7f22559..6a067de 100644
--- a/drivers/net/tg3.h
+++ b/drivers/net/tg3.h
@@ -2074,12 +2074,22 @@ struct tg3 {
 
 	/* SMP locking strategy:
 	 *
-	 * lock: Held during all operations except TX packet
-	 *       processing.
+	 * lock: Held during reset, PHY access, timer, and when
+	 *       updating tg3_flags and tg3_flags2.
 	 *
-	 * tx_lock: Held during tg3_start_xmit and tg3_tx
+	 * tx_lock: Held during tg3_start_xmit and tg3_tx only
+	 *          when calling netif_[start|stop]_queue.
+	 *          tg3_start_xmit is protected by netif_tx_lock.
 	 *
 	 * Both of these locks are to be held with BH safety.
+	 *
+	 * Because the IRQ handler, tg3_poll, and tg3_start_xmit
+	 * are running lockless, it is necessary to completely
+	 * quiesce the chip with tg3_netif_stop and tg3_full_lock
+	 * before reconfiguring the device.
+	 *
+	 * indirect_lock: Held when accessing registers indirectly
+	 *                with IRQ disabling.
 	 */
 	spinlock_t			lock;
 	spinlock_t			indirect_lock;



^ permalink raw reply related

* [PATCH 2.6.18 3/3] tg3: Add tg3_netif_stop() in vlan funtions
From: Michael Chan @ 2006-06-05 19:47 UTC (permalink / raw)
  To: davem; +Cc: herbert, jgarzik, netdev

Add tg3_netif_stop() when changing the vlgrp (vlan group) pointer. It
is necessary to quiesce the device before changing that pointer.

Update version to 3.60

Signed-off-by: Michael Chan <mchan@broadcom.com>


diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index f085890..d97cded 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -69,8 +69,8 @@
 
 #define DRV_MODULE_NAME		"tg3"
 #define PFX DRV_MODULE_NAME	": "
-#define DRV_MODULE_VERSION	"3.59"
-#define DRV_MODULE_RELDATE	"May 26, 2006"
+#define DRV_MODULE_VERSION	"3.60"
+#define DRV_MODULE_RELDATE	"June 5, 2006"
 
 #define TG3_DEF_MAC_MODE	0
 #define TG3_DEF_RX_MODE		0
@@ -8735,6 +8735,9 @@ static void tg3_vlan_rx_register(struct 
 {
 	struct tg3 *tp = netdev_priv(dev);
 
+	if (netif_running(dev))
+		tg3_netif_stop(tp);
+
 	tg3_full_lock(tp, 0);
 
 	tp->vlgrp = grp;
@@ -8743,16 +8746,25 @@ static void tg3_vlan_rx_register(struct 
 	__tg3_set_rx_mode(dev);
 
 	tg3_full_unlock(tp);
+
+	if (netif_running(dev))
+		tg3_netif_start(tp);
 }
 
 static void tg3_vlan_rx_kill_vid(struct net_device *dev, unsigned short vid)
 {
 	struct tg3 *tp = netdev_priv(dev);
 
+	if (netif_running(dev))
+		tg3_netif_stop(tp);
+
 	tg3_full_lock(tp, 0);
 	if (tp->vlgrp)
 		tp->vlgrp->vlan_devices[vid] = NULL;
 	tg3_full_unlock(tp);
+
+	if (netif_running(dev))
+		tg3_netif_start(tp);
 }
 #endif
 



^ permalink raw reply related

* Please pull 'upstream-fixes' branch of wireless-2.6
From: John W. Linville @ 2006-06-05 21:53 UTC (permalink / raw)
  To: jeff; +Cc: netdev

This pull is intended for 2.6.17 if at all possible.

Thanks,

John

---

The following changes since commit 672c6108a51bf559d19595d9f8193dfd81f0f752:
  Linus Torvalds:
        Merge master.kernel.org:/.../jejb/scsi-rc-fixes-2.6

are found in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-2.6.git upstream-fixes

Michael Buesch:
      bcm43xx: add DMA rx poll workaround to DMA4

 drivers/net/wireless/bcm43xx/bcm43xx_dma.c |   31 ++++++++++++++++++++--------
 1 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/bcm43xx/bcm43xx_dma.c b/drivers/net/wireless/bcm43xx/bcm43xx_dma.c
index bbecba0..d0318e5 100644
--- a/drivers/net/wireless/bcm43xx/bcm43xx_dma.c
+++ b/drivers/net/wireless/bcm43xx/bcm43xx_dma.c
@@ -624,25 +624,28 @@ err_destroy_tx0:
 static u16 generate_cookie(struct bcm43xx_dmaring *ring,
 			   int slot)
 {
-	u16 cookie = 0x0000;
+	u16 cookie = 0xF000;
 
 	/* Use the upper 4 bits of the cookie as
 	 * DMA controller ID and store the slot number
-	 * in the lower 12 bits
+	 * in the lower 12 bits.
+	 * Note that the cookie must never be 0, as this
+	 * is a special value used in RX path.
 	 */
 	switch (ring->mmio_base) {
 	default:
 		assert(0);
 	case BCM43xx_MMIO_DMA1_BASE:
+		cookie = 0xA000;
 		break;
 	case BCM43xx_MMIO_DMA2_BASE:
-		cookie = 0x1000;
+		cookie = 0xB000;
 		break;
 	case BCM43xx_MMIO_DMA3_BASE:
-		cookie = 0x2000;
+		cookie = 0xC000;
 		break;
 	case BCM43xx_MMIO_DMA4_BASE:
-		cookie = 0x3000;
+		cookie = 0xD000;
 		break;
 	}
 	assert(((u16)slot & 0xF000) == 0x0000);
@@ -660,16 +663,16 @@ struct bcm43xx_dmaring * parse_cookie(st
 	struct bcm43xx_dmaring *ring = NULL;
 
 	switch (cookie & 0xF000) {
-	case 0x0000:
+	case 0xA000:
 		ring = dma->tx_ring0;
 		break;
-	case 0x1000:
+	case 0xB000:
 		ring = dma->tx_ring1;
 		break;
-	case 0x2000:
+	case 0xC000:
 		ring = dma->tx_ring2;
 		break;
-	case 0x3000:
+	case 0xD000:
 		ring = dma->tx_ring3;
 		break;
 	default:
@@ -839,8 +842,18 @@ static void dma_rx(struct bcm43xx_dmarin
 		/* We received an xmit status. */
 		struct bcm43xx_hwxmitstatus *hw = (struct bcm43xx_hwxmitstatus *)skb->data;
 		struct bcm43xx_xmitstatus stat;
+		int i = 0;
 
 		stat.cookie = le16_to_cpu(hw->cookie);
+		while (stat.cookie == 0) {
+			if (unlikely(++i >= 10000)) {
+				assert(0);
+				break;
+			}
+			udelay(2);
+			barrier();
+			stat.cookie = le16_to_cpu(hw->cookie);
+		}
 		stat.flags = hw->flags;
 		stat.cnt1 = hw->cnt1;
 		stat.cnt2 = hw->cnt2;
-- 
John W. Linville
linville@tuxdriver.com

^ permalink raw reply related

* Please pull 'upstream' branch of wireless-2.6
From: John W. Linville @ 2006-06-05 21:55 UTC (permalink / raw)
  To: jeff; +Cc: netdev
In-Reply-To: <20060605215353.GA7381@tuxdriver.com>

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

This pull is intended for 2.6.18.

Thanks,

John

---

The following changes since commit f6882a0688ea83db5fc2f3491ac9fcdce0834cc7:
  John W. Linville:
        Merge branch 'upstream-fixes' into upstream

are found in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-2.6.git upstream

Daniel Drake:
      softmac: complete shared key authentication
      softmac: Fix handling of authentication failure

Jason Lunz:
      bcm43xx: quiet down log spam from set_security

Joseph Jezak:
      softmac: unified capabilities computation

Pavel Machek:
      usb wifi: zd1201 cleanups
      wireless: move zd1201 where it belongs

Toralf Förster:
      ieee80211softmac_io.c: fix warning "defined but not used"

 drivers/net/wireless/Kconfig                   |   17 ++
 drivers/net/wireless/Makefile                  |    2 
 drivers/net/wireless/bcm43xx/bcm43xx_main.c    |   13 +-
 drivers/net/wireless/zd1201.c                  |   67 ++++------
 drivers/net/wireless/zd1201.h                  |    0 
 drivers/usb/net/Kconfig                        |   17 --
 drivers/usb/net/Makefile                       |    1 
 include/net/ieee80211.h                        |    3 
 include/net/ieee80211softmac.h                 |    2 
 net/ieee80211/ieee80211_tx.c                   |   25 +++-
 net/ieee80211/softmac/Kconfig                  |    1 
 net/ieee80211/softmac/ieee80211softmac_assoc.c |   22 +++
 net/ieee80211/softmac/ieee80211softmac_auth.c  |   12 +-
 net/ieee80211/softmac/ieee80211softmac_event.c |    5 +
 net/ieee80211/softmac/ieee80211softmac_io.c    |  169 +++++++++++-------------
 15 files changed, 180 insertions(+), 176 deletions(-)
 rename drivers/{usb/net/zd1201.c => net/wireless/zd1201.c} (98%)
 rename drivers/{usb/net/zd1201.h => net/wireless/zd1201.h} (100%)

Patch included as attachment "upstream.patch.bz2"
-- 
John W. Linville
linville@tuxdriver.com

[-- Attachment #2: upstream.patch.bz2 --]
[-- Type: application/x-bzip2, Size: 23158 bytes --]

^ permalink raw reply

* Re: [PATCH 1/1] LSM-IPsec SELinux Authorize
From: James Morris @ 2006-06-05 22:32 UTC (permalink / raw)
  To: Catherine Zhang
  Cc: netdev, davem, chrisw, herbert, sds, tjaeger, latten, sergeh,
	gcwilson, czhang.us
In-Reply-To: <20060605195607.GA28030@jiayuguan.watson.ibm.com>

On Mon, 5 Jun 2006, Catherine Zhang wrote:


Looks ok to me, except for one minor nit:

> +	if (ctx) {
> +		rc = avc_has_perm(tsec->sid, ctx->ctx_sid,
> +		SECCLASS_ASSOCIATION,
> +		ASSOCIATION__SETCONTEXT, NULL);
> +	}

you don't need the braces here (similar again further in the patch).


- James
-- 
James Morris
<jmorris@namei.org>

^ permalink raw reply

* Re: [PATCH 2.6.18 2/3] tg3: Convert to non-LLTX
From: Stephen Hemminger @ 2006-06-05 22:58 UTC (permalink / raw)
  To: Michael Chan; +Cc: davem, herbert, jgarzik, netdev
In-Reply-To: <1149536852.13155.7.camel@rh4>

On Mon, 05 Jun 2006 12:47:32 -0700
"Michael Chan" <mchan@broadcom.com> wrote:

> Herbert Xu pointed out that it is unsafe to call netif_tx_disable()
> from LLTX drivers because it uses dev->xmit_lock to synchronize
> whereas LLTX drivers use private locks.
> 
> Convert tg3 to non-LLTX to fix this issue. tg3 is a lockless driver
> where hard_start_xmit and tx completion handling can run concurrently
> under normal conditions. A tx_lock is only needed to prevent
> netif_stop_queue and netif_wake_queue race condtions when the queue
> is full.
> 
> So whether we use LLTX or non-LLTX, it makes practically no
> difference.
> 
> Signed-off-by: Michael Chan <mchan@broadcom.com>

Since you are going more lockless, you probably need memory barriers.

I don't feel although that comfortable with the lockless driver model as is.
Perhaps there is something simpler that could be done with a ring model and
some atomic primitives like cmpxchg()?

^ permalink raw reply

* Re: [PATCH] TCP: removal of unused label
From: Stephen Hemminger @ 2006-06-05 23:02 UTC (permalink / raw)
  To: Francois Romieu; +Cc: David S. Miller, netdev
In-Reply-To: <20060605195549.GA14942@electric-eye.fr.zoreil.com>

On Mon, 5 Jun 2006 21:55:49 +0200
Francois Romieu <romieu@fr.zoreil.com> wrote:

> 
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
> 
> diff --git a/net/ipv4/tcp_compound.c b/net/ipv4/tcp_compound.c
> index bc54f7e..a3c36c0 100644
> --- a/net/ipv4/tcp_compound.c
> +++ b/net/ipv4/tcp_compound.c
> @@ -401,6 +401,7 @@ static void tcp_compound_cong_avoid(stru
>  static void tcp_compound_get_info(struct sock *sk, u32 ext, struct sk_buff *skb)
>  {
>  	const struct compound *ca = inet_csk_ca(sk);
> +
>  	if (ext & (1 << (INET_DIAG_VEGASINFO - 1))) {
>  		struct tcpvegas_info *info;
>  
> @@ -411,7 +412,6 @@ static void tcp_compound_get_info(struct
>  		info->tcpv_rttcnt = ca->cntRTT;
>  		info->tcpv_rtt = ca->baseRTT;
>  		info->tcpv_minrtt = ca->minRTT;
> -	rtattr_failure:;
>  	}
>  }
>  


rtattr_failure is used inside __RTA_PUT() macro. It's gross, but there are
those who love it..

^ permalink raw reply

* Re: [PATCH] TCP: removal of unused label
From: David Miller @ 2006-06-05 23:03 UTC (permalink / raw)
  To: shemminger; +Cc: romieu, netdev
In-Reply-To: <20060605160240.4951f7ad@dxpl.pdx.osdl.net>

From: Stephen Hemminger <shemminger@osdl.org>
Date: Mon, 5 Jun 2006 16:02:40 -0700

> rtattr_failure is used inside __RTA_PUT() macro. It's gross, but there are
> those who love it..

Yes, it was clear that this patch wasn't build tested.

And GCC usually warns very loudly about truly unused
labels.

^ permalink raw reply

* [PATCH 0/2] e1000: fixes for netpoll+NAPI, ARM
From: Kok, Auke @ 2006-06-05 23:09 UTC (permalink / raw)
  To: Garzik, Jeff; +Cc: netdev, Brandeburg, Jesse, Kok, Auke, Kok, Auke


Hi,

This patch series implements two e1000 fixes:

1: fix netpoll with NAPI
2: fix ARM prefetch failure by removing risky prefetches


These changes are available through git:

git://lost.foo-projects.org/~ahkok/git/netdev-2.6 upstream-fixes

these patches are against
 netdev-2.6#upstream-fixes c7812d8ccf1d11f29f752f53727ef6b55bc35150

---

Jeff:

The patch earlier sent regarding:
    e1000: fix irq sharing when running ethtool test
is also still in this branch.

They are intended for jgarzik/netdev-2.6#upstream-fixes.


Cheers,

Auke


---
 drivers/net/e1000/e1000_main.c |   17 ++++++++++-------
  1 files changed, 10 insertions(+), 7 deletions(-)


--
Auke Kok <auke-jan.h.kok@intel.com>

^ permalink raw reply

* [PATCH 1/2] e1000: fix netpoll with NAPI
From: Kok, Auke @ 2006-06-05 23:11 UTC (permalink / raw)
  To: Garzik, Jeff; +Cc: netdev, Brandeburg, Jesse, Kok, Auke, Kok, Auke
In-Reply-To: <20060605230917.12584.55755.stgit@gitlost.site>


Netpoll was broken due to the earlier addition of multiqueue.

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
---

 drivers/net/e1000/e1000_main.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index ed15fca..7103a0e 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -4629,10 +4629,17 @@ static void
 e1000_netpoll(struct net_device *netdev)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
+#ifdef CONFIG_E1000_NAPI
+	int budget = 0;
+
+	disable_irq(adapter->pdev->irq);
+	e1000_clean_tx_irq(adapter, adapter->tx_ring);
+	adapter->clean_rx(adapter, adapter->rx_ring, &budget, netdev->weight);
+#else
+
 	disable_irq(adapter->pdev->irq);
 	e1000_intr(adapter->pdev->irq, netdev, NULL);
 	e1000_clean_tx_irq(adapter, adapter->tx_ring);
-#ifndef CONFIG_E1000_NAPI
 	adapter->clean_rx(adapter, adapter->rx_ring);
 #endif
 	enable_irq(adapter->pdev->irq);



--
Auke Kok <auke-jan.h.kok@intel.com>

^ permalink raw reply related

* [PATCH 2/2] e1000: remove risky prefetch on next_skb->data
From: Kok, Auke @ 2006-06-05 23:11 UTC (permalink / raw)
  To: Garzik, Jeff; +Cc: netdev, Brandeburg, Jesse, Kok, Auke, Kok, Auke
In-Reply-To: <20060605230917.12584.55755.stgit@gitlost.site>


It was brought to our attention that the prefetches break e1000 traffic
on xscale/arm architectures.  Remove them for now.  We'll let them
stay in mm for a while, or find a better solution to enable.

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
---

 drivers/net/e1000/e1000_main.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 7103a0e..aef616f 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -3519,7 +3519,7 @@ e1000_clean_rx_irq(struct e1000_adapter 
 	buffer_info = &rx_ring->buffer_info[i];
 
 	while (rx_desc->status & E1000_RXD_STAT_DD) {
-		struct sk_buff *skb, *next_skb;
+		struct sk_buff *skb;
 		u8 status;
 #ifdef CONFIG_E1000_NAPI
 		if (*work_done >= work_to_do)
@@ -3537,8 +3537,6 @@ e1000_clean_rx_irq(struct e1000_adapter 
 		prefetch(next_rxd);
 
 		next_buffer = &rx_ring->buffer_info[i];
-		next_skb = next_buffer->skb;
-		prefetch(next_skb->data - NET_IP_ALIGN);
 
 		cleaned = TRUE;
 		cleaned_count++;
@@ -3668,7 +3666,7 @@ e1000_clean_rx_irq_ps(struct e1000_adapt
 	struct e1000_buffer *buffer_info, *next_buffer;
 	struct e1000_ps_page *ps_page;
 	struct e1000_ps_page_dma *ps_page_dma;
-	struct sk_buff *skb, *next_skb;
+	struct sk_buff *skb;
 	unsigned int i, j;
 	uint32_t length, staterr;
 	int cleaned_count = 0;
@@ -3697,8 +3695,6 @@ e1000_clean_rx_irq_ps(struct e1000_adapt
 		prefetch(next_rxd);
 
 		next_buffer = &rx_ring->buffer_info[i];
-		next_skb = next_buffer->skb;
-		prefetch(next_skb->data - NET_IP_ALIGN);
 
 		cleaned = TRUE;
 		cleaned_count++;



--
Auke Kok <auke-jan.h.kok@intel.com>

^ permalink raw reply related

* Re: [PATCH 2.6.18 2/3] tg3: Convert to non-LLTX
From: David Miller @ 2006-06-05 23:06 UTC (permalink / raw)
  To: shemminger; +Cc: mchan, herbert, jgarzik, netdev
In-Reply-To: <20060605155834.031d37a2@localhost.localdomain>

From: Stephen Hemminger <shemminger@osdl.org>
Date: Mon, 5 Jun 2006 15:58:34 -0700

> Perhaps there is something simpler that could be done with a ring
> model and some atomic primitives like cmpxchg()?

cmpxchg() is something not available natively on many platforms, you
can't even emulate it %100 properly on systems that just have some
kind of test-and-set spinlock type primitive.

You can synchronize the cmpxchg() itself using a spinlock on such
platforms, but you cannot synchronize the accesses done by other
processors which do not use cmpxchg().

So using cmpxchg() would block out such platforms from being able
to use the tg3 driver any longer, which really isn't an option.

^ permalink raw reply

* Re: [PATCH 2.6.18 2/3] tg3: Convert to non-LLTX
From: Michael Chan @ 2006-06-05 21:29 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: davem, herbert, jgarzik, netdev
In-Reply-To: <20060605155834.031d37a2@localhost.localdomain>

On Mon, 2006-06-05 at 15:58 -0700, Stephen Hemminger wrote:

> 
> Since you are going more lockless, you probably need memory barriers.

No, we're not going more lockless. We're simply replacing the private
tx_lock with dev->xmit_lock by dropping the LLTX feature flag. The
amount of locking is exactly the same as before.



^ permalink raw reply

* Re: [PATCH 2/2] e1000: remove risky prefetch on next_skb->data
From: Rick Jones @ 2006-06-05 23:21 UTC (permalink / raw)
  To: Kok, Auke; +Cc: Garzik, Jeff, netdev, Brandeburg, Jesse, Kok, Auke
In-Reply-To: <20060605231127.12584.14321.stgit@gitlost.site>

Kok, Auke wrote:
> It was brought to our attention that the prefetches break e1000 traffic
> on xscale/arm architectures.  Remove them for now.  We'll let them
> stay in mm for a while, or find a better solution to enable.

Out of curiousity, what breaks?

rick jones


^ permalink raw reply

* Re: [patch 06/17] neighbour.c, pneigh_get_next() skips published entry
From: David Miller @ 2006-06-05 23:30 UTC (permalink / raw)
  To: akpm; +Cc: netdev, Jari.Takkala
In-Reply-To: <200606020328.k523S8hh028820@shell0.pdx.osdl.net>

From: akpm@osdl.org
Date: Thu, 01 Jun 2006 20:32:26 -0700

> From: "Jari Takkala" <Jari.Takkala@Q9.com>
> 
> Fix a problem where output from /proc/net/arp skips a record when the full
> output does not fit into the users read() buffer.
> 
> To reproduce: publish a large number of ARP entries (more than 10 required
> on my system).  Run 'dd if=/proc/net/arp of=arp-1024.out bs=1024'.  View
> the output, one entry will be missing.
> 
> Signed-off-by: Jari Takkala <jari.takkala@q9.com>
> Signed-off-by: Andrew Morton <akpm@osdl.org>

This patch doesn't make any sense, I've been over it a few
times.

The seqfile layer should take care of that user buffering
issue transparently as long as we implement the interface
callbacks properly.

Even if something needs to be fixed in the pneigh dumper,
special casing *pos==1 doesn't look right.  Also, if pneigh
has this problem, how come the neigh seqfile iterators don't
have the same problem or do they?

^ permalink raw reply

* Re: MIB "ipInHdrErrors" error
From: David Miller @ 2006-06-05 23:38 UTC (permalink / raw)
  To: weid; +Cc: netdev
In-Reply-To: <1148546261.3038.14.camel@RHEL3GM>

From: Wei Dong <weid@nanjing-fnst.com>
Date: Thu, 25 May 2006 16:37:42 +0800

>     When I test linux kernel 2.6.9-34, and find that kernel statistics
> about ipInHdrErrors which exsits in file /proc/net/snmp doesn't increase
> correctly.  The criteria conform to RFC2011:
> 
> ipInHdrErrors OBJECT-TYPE
>     SYNTAX      Counter32
>     MAX-ACCESS  read-only
>     STATUS      current
>     DESCRIPTION
>             "The number of input datagrams discarded due to errors in
>             their IP headers, including bad checksums, version number
>             mismatch, other format errors, time-to-live exceeded, errors
>             discovered in processing their IP options, etc."
> 
> When kernel receives an IP packet containing error protocol in IP
> header, kernel doesn't increase this counter "ipInHdrErrors".

This event is not listed in the description you quoted.

> Also, when kernel receives an IP packet and need to forward, but
> TTL=1 or TTL=0, kernel just sends an ICMP packet to inform the
> sender TTL count exceeded, and doesn't increase this counter.

This part of your change seems correct, please just resubmit
this part.

^ permalink raw reply

* Re: [PATCH v3] bridge: fix locking and memory leak in br_add_bridge
From: David Miller @ 2006-06-05 23:38 UTC (permalink / raw)
  To: shemminger; +Cc: jbenc, netdev
In-Reply-To: <20060527094528.0c330d9f@localhost.localdomain>

From: Stephen Hemminger <shemminger@osdl.org>
Date: Sat, 27 May 2006 09:45:28 -0700

> There are several bugs in error handling in br_add_bridge:
> - when dev_alloc_name fails, allocated net_device is not freed
> - unregister_netdev is called when rtnl lock is held
> - free_netdev is called before netdev_run_todo has a chance to be run after
>   unregistering net_device
> 
> Signed-off-by: Jiri Benc <jbenc@suse.cz>
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

Applied, thanks a lot.

^ permalink raw reply

* [PATCH 0/2] PHYLIB: Fix forcing mode reduction
From: Nathaniel Case @ 2006-06-05 23:45 UTC (permalink / raw)
  To: Andy Fleming; +Cc: netdev, Jeff Garzik, galak

On Mon, 2006-06-05 at 17:08 -0500, Andy Fleming wrote:
> Looks good.  Feel free to send these patches to  
> netdev@vger.kernel.org (you may need to subscribe), and copy Jeff  
> Garzik <jeff@garzik.org>.

This fixes a problem seen when a port without a cable connected would
repeatedly print out "Trying 1000/HALF".  While in the PHY_FORCING
state, the call to phy_read_status() was resetting the value of
phydev->speed and phydev->duplex, preventing it from incrementally
trying the speed/duplex variations.

Since we really just want the link status updated for the PHY_FORCING
state, calling genphy_update_link() instead of phy_read_status() fixes
this issue.

Patch tested on a MPC8540 platform with a BCM5421 PHY.

Signed-off-by: Nate Case <ncase@xes-inc.com>
Signed-off-by: Andy Fleming <afleming@freescale.com>

---

--- a/drivers/net/phy/phy.c	2006-06-04 16:01:59.000000000 -0500
+++ b/drivers/net/phy/phy.c	2006-06-05 10:55:31.000000000 -0500
@@ -767,7 +783,7 @@
 			}
 			break;
 		case PHY_FORCING:
-			err = phy_read_status(phydev);
+			err = genphy_update_link(phydev);
 
 			if (err)
 				break;
--- a/drivers/net/phy/phy_device.c	2006-06-04 16:02:08.000000000 -0500
+++ b/drivers/net/phy/phy_device.c	2006-06-04 19:12:26.000000000 -0500
@@ -417,6 +417,7 @@
 
 	return 0;
 }
+EXPORT_SYMBOL(genphy_update_link);
 
 /* genphy_read_status
  *



^ permalink raw reply

* [PATCH 1/2] PHYLIB: Add get_link ethtool helper function
From: Nathaniel Case @ 2006-06-05 23:48 UTC (permalink / raw)
  To: Andy Fleming; +Cc: netdev, Jeff Garzik, galak

This adds a phy_ethtool_get_link() function along the same lines as
phy_ethtool_gset().  This provides drivers utilizing PHYLIB an
alternative to using ethtool_op_get_link().  This is more desirable
since the "Link detected" field in ethtool would actually reflect the
state of the PHY register.

Patch depends on previous patch (0/2).

Signed-off-by: Nate Case <ncase@xes-inc.com>
Signed-off-by: Andy Fleming <afleming@freescale.com>

---

--- a/drivers/net/phy/phy.c	2006-06-04 16:01:59.000000000 -0500
+++ b/drivers/net/phy/phy.c	2006-06-05 10:55:31.000000000 -0500
@@ -301,6 +301,22 @@
 	return 0;
 }
 
+/* 
+ * phy_ethtool_get_link:
+ * A generic ethtool get_link function which is more accurate than
+ * the network interface carrier status since it queries the
+ * PHY directly.
+ */
+u32 phy_ethtool_get_link(struct phy_device *phydev)
+{
+	int err;
+
+	err = genphy_update_link(phydev);
+	if (err)
+		return 0;
+
+	return (u32) phydev->link;
+}
 
 /* Note that this function is currently incompatible with the
  * PHYCONTROL layer.  It changes registers without regard to
--- a/include/linux/phy.h	2006-06-05 11:33:59.000000000 -0500
+++ b/include/linux/phy.h	2006-06-04 19:31:51.000000000 -0500
@@ -374,6 +374,7 @@
 void phy_stop_machine(struct phy_device *phydev);
 int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd);
 int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd);
+u32 phy_ethtool_get_link(struct phy_device *phydev);
 int phy_mii_ioctl(struct phy_device *phydev,
 		struct mii_ioctl_data *mii_data, int cmd);
 int phy_start_interrupts(struct phy_device *phydev);




^ permalink raw reply

* [PATCH 2/2] PHYLIB/gianfar: Use phy_ethtool_get_link() for get_link op
From: Nathaniel Case @ 2006-06-05 23:48 UTC (permalink / raw)
  To: Andy Fleming; +Cc: netdev, Jeff Garzik, galak

This patch makes the gianfar ethtool code use phy_ethtool_get_link() instead of
ethtool_op_get_link().

Patch depends on previous one (1/2).

Signed-off-by: Nate Case <ncase@xes-inc.com>

---

--- a/drivers/net/gianfar_ethtool.c	2006-06-05 11:27:19.000000000 -0500
+++ b/drivers/net/gianfar_ethtool.c	2006-06-04 19:31:01.000000000 -0500
@@ -228,6 +228,18 @@
 		buf[i] = gfar_read(&theregs[i]);
 }
 
+static u32 gfar_get_link(struct net_device *dev)
+{
+	struct gfar_private *priv = netdev_priv(dev);
+	struct phy_device *phydev = priv->phydev;
+
+	if (NULL == phydev)
+		return -ENODEV;
+
+	return phy_ethtool_get_link(phydev);
+}
+
+
 /* Convert microseconds to ethernet clock ticks, which changes
  * depending on what speed the controller is running at */
 static unsigned int gfar_usecs2ticks(struct gfar_private *priv, unsigned int usecs)
@@ -574,7 +578,7 @@
 	.get_drvinfo = gfar_gdrvinfo,
 	.get_regs_len = gfar_reglen,
 	.get_regs = gfar_get_regs,
-	.get_link = ethtool_op_get_link,
+	.get_link = gfar_get_link,
 	.get_coalesce = gfar_gcoalesce,
 	.set_coalesce = gfar_scoalesce,
 	.get_ringparam = gfar_gringparam,



^ permalink raw reply

* Re: [PATCH] TCP changes for 2.6.18
From: David Miller @ 2006-06-05 23:58 UTC (permalink / raw)
  To: shemminger; +Cc: netdev
In-Reply-To: <20060605110331.0f058738@dxpl.pdx.osdl.net>

From: Stephen Hemminger <shemminger@osdl.org>
Date: Mon, 5 Jun 2006 11:03:31 -0700

> Dave, please consider adding these for the 2.6.18.

I'll pick these into my tree by extracting out the patches.
Your tree wasn't based upon the net-2.6.18 tree, so if I just
pull it into mine I'll get all the upstream changes since I
cut the net-2.6.18 which at the current time I don't want :)

BTW, it seems we now have at least 3 instances "VEGAS + stuff"
and thus the core vegas logic is duplicated that many times.
Would be nice to have some core vegas infrastructure in the
generic congestion avoidance layer at some point.

^ permalink raw reply

* Re: [PATCH 2/2] e1000: remove risky prefetch on next_skb->data
From: Brandeburg, Jesse @ 2006-06-06  0:12 UTC (permalink / raw)
  To: Rick Jones
  Cc: Kok, Auke-jan H, Garzik, Jeff, netdev, Brandeburg, Jesse,
	Kok, Auke
In-Reply-To: <4484BC62.3090707@hp.com>

On Mon, 5 Jun 2006, Rick Jones wrote:

> 
> Kok, Auke wrote:
> > It was brought to our attention that the prefetches break e1000 traffic
> > on xscale/arm architectures.  Remove them for now.  We'll let them
> > stay in mm for a while, or find a better solution to enable.
> 
> Out of curiousity, what breaks?

Hi Rick, according to our reporter, receives break.  The prefetch (not 
always, but sometimes) lets the processor get junk from the prefetched 
area.  Apparently this version of arm doesn't quite do as strict 
enforcement of bus snoops as x86, ia64, (and even pSeries!) does.

This manifested with a large drop in receive peformance using TCP, 
probably because it was retransmitting frequently.

Jesse

^ permalink raw reply

* Re: [PATCH 2/2] e1000: remove risky prefetch on next_skb->data
From: Rick Jones @ 2006-06-06  0:16 UTC (permalink / raw)
  To: Brandeburg, Jesse; +Cc: Kok, Auke-jan H, Garzik, Jeff, netdev, Kok, Auke
In-Reply-To: <Pine.WNT.4.63.0606051707520.1168@jbrandeb-desk.amr.corp.intel.com>

Brandeburg, Jesse wrote:
> On Mon, 5 Jun 2006, Rick Jones wrote:
>>Kok, Auke wrote:
>>
>>>It was brought to our attention that the prefetches break e1000 traffic
>>>on xscale/arm architectures.  Remove them for now.  We'll let them
>>>stay in mm for a while, or find a better solution to enable.
>>
>>Out of curiousity, what breaks?
> 
> 
> Hi Rick, according to our reporter, receives break.  The prefetch (not 
> always, but sometimes) lets the processor get junk from the prefetched 
> area.  Apparently this version of arm doesn't quite do as strict 
> enforcement of bus snoops as x86, ia64, (and even pSeries!) does.

Bear with me, I'm a software guy :)  I interpret that to mean that the 
processor is basically broken?  If so, wouldn't it be the case that 
prefetch() needs to become a noop on that processor?

> This manifested with a large drop in receive peformance using TCP, 
> probably because it was retransmitting frequently.

I forget - what were the gains on the other CPUs?

rick

> 
> Jesse


^ permalink raw reply

* Re: [PATCH 2/2] e1000: remove risky prefetch on next_skb->data
From: Andi Kleen @ 2006-06-06  0:22 UTC (permalink / raw)
  To: Rick Jones
  Cc: Brandeburg, Jesse, Kok, Auke-jan H, Garzik, Jeff, netdev,
	Kok, Auke
In-Reply-To: <4484C95B.6010009@hp.com>


> Bear with me, I'm a software guy :)  I interpret that to mean that the 
> processor is basically broken?  If so, wouldn't it be the case that 
> prefetch() needs to become a noop on that processor?

I would agree with Rick - if prefetch() is broken on Xscale it should be disabled
in the architecture, not in individual drivers. Otherwise all other code
that use it might be broken too.

Maybe it's only broken on MMIO and not normal memory - in that
case introducing a mmio_prefetch() (and defining it as an nop on Xscale) would be also
an option.

-Andi

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox