* [PATCH]atl1c:update version from 1.0.0.0 to 1.0.1.0
From: jie.yang @ 2009-08-04 8:07 UTC (permalink / raw)
To: davem; +Cc: Angus.Huang, netdev, linux-kernel, jie yang
1. update atl1c_suspend routine, just set speed to 10M Full when
in suspend for power saving.
2. change atl1c_set_aspm routine for power saving.
3. add atl1c_pcie_patch routine just for refactoring.
4. update version to 1.0.1.0-NAPI.
Signed-off-by: jie yang <jie.yang@atheros.com>
---
diff --git a/drivers/net/atl1c/atl1c.h b/drivers/net/atl1c/atl1c.h
index 2a1120a..d838962 100644
--- a/drivers/net/atl1c/atl1c.h
+++ b/drivers/net/atl1c/atl1c.h
@@ -91,6 +91,7 @@
#define AT_ASPM_L0S_TIMER 6
#define AT_ASPM_L1_TIMER 12
+#define AT_LCKDET_TIMER 12
#define ATL1C_PCIE_L0S_L1_DISABLE 0x01
#define ATL1C_PCIE_PHY_RESET 0x02
diff --git a/drivers/net/atl1c/atl1c_hw.h b/drivers/net/atl1c/atl1c_hw.h
index c2c738d..58b58b4 100644
--- a/drivers/net/atl1c/atl1c_hw.h
+++ b/drivers/net/atl1c/atl1c_hw.h
@@ -149,13 +149,15 @@ int atl1c_restart_autoneg(struct atl1c_hw *hw);
#define PM_CTRL_ASPM_L0S_EN 0x00001000
#define PM_CTRL_CLK_SWH_L1 0x00002000
#define PM_CTRL_CLK_PWM_VER1_1 0x00004000
-#define PM_CTRL_PCIE_RECV 0x00008000
+#define PM_CTRL_RCVR_WT_TIMER 0x00008000
#define PM_CTRL_L1_ENTRY_TIMER_MASK 0xF
#define PM_CTRL_L1_ENTRY_TIMER_SHIFT 16
#define PM_CTRL_PM_REQ_TIMER_MASK 0xF
#define PM_CTRL_PM_REQ_TIMER_SHIFT 20
-#define PM_CTRL_LCKDET_TIMER_MASK 0x3F
+#define PM_CTRL_LCKDET_TIMER_MASK 0xF
#define PM_CTRL_LCKDET_TIMER_SHIFT 24
+#define PM_CTRL_EN_BUFS_RX_L0S 0x10000000
+#define PM_CTRL_SA_DLY_EN 0x20000000
#define PM_CTRL_MAC_ASPM_CHK 0x40000000
#define PM_CTRL_HOTRST 0x80000000
@@ -165,6 +167,7 @@ int atl1c_restart_autoneg(struct atl1c_hw *hw);
#define MASTER_CTRL_TEST_MODE_MASK 0x3
#define MASTER_CTRL_TEST_MODE_SHIFT 2
#define MASTER_CTRL_BERT_START 0x10
+#define MASTER_CTRL_SA_TIMER_EN 0x80
#define MASTER_CTRL_MTIMER_EN 0x100
#define MASTER_CTRL_MANUAL_INT 0x200
#define MASTER_CTRL_TX_ITIMER_EN 0x400
@@ -217,6 +220,12 @@ int atl1c_restart_autoneg(struct atl1c_hw *hw);
GPHY_CTRL_PWDOWN_HW |\
GPHY_CTRL_PHY_IDDQ)
+#define GPHY_CTRL_POWER_SAVING ( \
+ GPHY_CTRL_SEL_ANA_RST |\
+ GPHY_CTRL_HIB_EN |\
+ GPHY_CTRL_HIB_PULSE |\
+ GPHY_CTRL_PWDOWN_HW |\
+ GPHY_CTRL_PHY_IDDQ)
/* Block IDLE Status Register */
#define REG_IDLE_STATUS 0x1410
#define IDLE_STATUS_MASK 0x00FF
@@ -284,6 +293,14 @@ int atl1c_restart_autoneg(struct atl1c_hw *hw);
#define SERDES_LOCK_DETECT 0x1 /* SerDes lock detected. This signal
* comes from Analog SerDes */
#define SERDES_LOCK_DETECT_EN 0x2 /* 1: Enable SerDes Lock detect function */
+#define SERDES_LOCK_STS_SELFB_PLL_SHIFT 0xE
+#define SERDES_LOCK_STS_SELFB_PLL_MASK 0x3
+#define SERDES_OVCLK_18_25 0x0
+#define SERDES_OVCLK_12_18 0x1
+#define SERDES_OVCLK_0_4 0x2
+#define SERDES_OVCLK_4_12 0x3
+#define SERDES_MAC_CLK_SLOWDOWN 0x20000
+#define SERDES_PYH_CLK_SLOWDOWN 0x40000
/* MAC Control Register */
#define REG_MAC_CTRL 0x1480
@@ -688,6 +705,21 @@ int atl1c_restart_autoneg(struct atl1c_hw *hw);
#define REG_MAC_TX_STATUS_BIN 0x1760
#define REG_MAC_TX_STATUS_END 0x17c0
+#define REG_CLK_GATING_CTRL 0x1814
+#define CLK_GATING_DMAW_EN 0x0001
+#define CLK_GATING_DMAR_EN 0x0002
+#define CLK_GATING_TXQ_EN 0x0004
+#define CLK_GATING_RXQ_EN 0x0008
+#define CLK_GATING_TXMAC_EN 0x0010
+#define CLK_GATING_RXMAC_EN 0x0020
+
+#define CLK_GATING_EN_ALL (CLK_GATING_DMAW_EN |\
+ CLK_GATING_DMAR_EN |\
+ CLK_GATING_TXQ_EN |\
+ CLK_GATING_RXQ_EN |\
+ CLK_GATING_TXMAC_EN|\
+ CLK_GATING_RXMAC_EN)
+
/* DEBUG ADDR */
#define REG_DEBUG_DATA0 0x1900
#define REG_DEBUG_DATA1 0x1904
diff --git a/drivers/net/atl1c/atl1c_main.c b/drivers/net/atl1c/atl1c_main.c
index 1d601ce..a3ed02a 100644
--- a/drivers/net/atl1c/atl1c_main.c
+++ b/drivers/net/atl1c/atl1c_main.c
@@ -21,7 +21,7 @@
#include "atl1c.h"
-#define ATL1C_DRV_VERSION "1.0.0.1-NAPI"
+#define ATL1C_DRV_VERSION "1.0.1.0-NAPI"
char atl1c_driver_name[] = "atl1c";
char atl1c_driver_version[] = ATL1C_DRV_VERSION;
#define PCI_DEVICE_ID_ATTANSIC_L2C 0x1062
@@ -85,6 +85,15 @@ static const u16 atl1c_rrd_addr_lo_regs[AT_MAX_RECEIVE_QUEUE] =
REG_RRD3_HEAD_ADDR_LO
};
+static void atl1c_pcie_patch(struct atl1c_hw *hw)
+{
+ u32 data;
+
+ AT_READ_REG(hw, REG_PCIE_PHYMISC, &data);
+ data |= PCIE_PHYMISC_FORCE_RCV_DET;
+ AT_WRITE_REG(hw, REG_PCIE_PHYMISC, data);
+
+}
static const u32 atl1c_default_msg = NETIF_MSG_DRV | NETIF_MSG_PROBE |
NETIF_MSG_LINK | NETIF_MSG_TIMER | NETIF_MSG_IFDOWN | NETIF_MSG_IFUP;
@@ -117,6 +126,7 @@ static void atl1c_reset_pcie(struct atl1c_hw *hw, u32 flag)
data &= ~PCIE_UC_SERVRITY_FCP;
AT_WRITE_REG(hw, REG_PCIE_UC_SEVERITY, data);
+ atl1c_pcie_patch(hw);
if (flag & ATL1C_PCIE_L0S_L1_DISABLE)
atl1c_disable_l0s_l1(hw);
if (flag & ATL1C_PCIE_PHY_RESET)
@@ -626,9 +636,7 @@ static int atl1c_setup_mac_funcs(struct atl1c_hw *hw)
AT_READ_REG(hw, REG_PHY_STATUS, &phy_status_data);
AT_READ_REG(hw, REG_LINK_CTRL, &link_ctrl_data);
- hw->ctrl_flags = ATL1C_INTR_CLEAR_ON_READ |
- ATL1C_INTR_MODRT_ENABLE |
- ATL1C_RX_IPV6_CHKSUM |
+ hw->ctrl_flags = ATL1C_INTR_MODRT_ENABLE |
ATL1C_TXQ_MODE_ENHANCE;
if (link_ctrl_data & LINK_CTRL_L0S_EN)
hw->ctrl_flags |= ATL1C_ASPM_L0S_SUPPORT;
@@ -1063,7 +1071,7 @@ static void atl1c_configure_rx(struct atl1c_adapter *adapter)
rxq_ctrl_data |= (hw->rss_hash_bits & RSS_HASH_BITS_MASK) <<
RSS_HASH_BITS_SHIFT;
if (hw->ctrl_flags & ATL1C_ASPM_CTRL_MON)
- rxq_ctrl_data |= (ASPM_THRUPUT_LIMIT_100M &
+ rxq_ctrl_data |= (ASPM_THRUPUT_LIMIT_1M &
ASPM_THRUPUT_LIMIT_MASK) << ASPM_THRUPUT_LIMIT_SHIFT;
AT_WRITE_REG(hw, REG_RXQ_CTRL, rxq_ctrl_data);
@@ -1241,17 +1249,43 @@ static void atl1c_set_aspm(struct atl1c_hw *hw, bool linkup)
AT_READ_REG(hw, REG_PM_CTRL, &pm_ctrl_data);
pm_ctrl_data &= ~PM_CTRL_SERDES_PD_EX_L1;
+#if 0
+ pm_ctrl_data |= PM_CTRL_SERDES_BUDS_RX_L1_EN;
+ pm_ctrl_data |= PM_CTRL_SERDES_L1_EN;
+#endif
pm_ctrl_data &= ~(PM_CTRL_L1_ENTRY_TIMER_MASK <<
PM_CTRL_L1_ENTRY_TIMER_SHIFT);
+ pm_ctrl_data &= ~(PM_CTRL_LCKDET_TIMER_MASK <<
+ PM_CTRL_LCKDET_TIMER_SHIFT);
pm_ctrl_data |= PM_CTRL_MAC_ASPM_CHK;
+ pm_ctrl_data &= ~PM_CTRL_ASPM_L1_EN;
+ pm_ctrl_data |= PM_CTRL_RBER_EN;
+ pm_ctrl_data |= PM_CTRL_SDES_EN;
if (linkup) {
pm_ctrl_data |= PM_CTRL_SERDES_PLL_L1_EN;
pm_ctrl_data &= ~PM_CTRL_CLK_SWH_L1;
+ pm_ctrl_data |= AT_LCKDET_TIMER <<
+ PM_CTRL_LCKDET_TIMER_SHIFT;
pm_ctrl_data |= PM_CTRL_SERDES_BUDS_RX_L1_EN;
pm_ctrl_data |= PM_CTRL_SERDES_L1_EN;
+ pm_ctrl_data |= PM_CTRL_CLK_REQ_EN;
+ pm_ctrl_data &= ~PM_CTRL_ASPM_L0S_EN;
+#if 0 /* to disable L1/L0s when connected , required by AK */
+ if (hw->ctrl_flags & ATL1C_ASPM_L1_SUPPORT) {
+ pm_ctrl_data |= AT_ASPM_L1_TIMER <<
+ PM_CTRL_L1_ENTRY_TIMER_SHIFT;
+ pm_ctrl_data |= PM_CTRL_ASPM_L1_EN;
+ } else
+ pm_ctrl_data &= ~PM_CTRL_ASPM_L1_EN;
+
+ if (hw->ctrl_flags & ATL1C_ASPM_L0S_SUPPORT)
+ pm_ctrl_data |= PM_CTRL_ASPM_L0S_EN;
+ else
+ pm_ctrl_data &= ~PM_CTRL_ASPM_L0S_EN;
+#endif
} else {
pm_ctrl_data &= ~PM_CTRL_SERDES_BUDS_RX_L1_EN;
pm_ctrl_data &= ~PM_CTRL_SERDES_L1_EN;
@@ -1259,13 +1293,12 @@ static void atl1c_set_aspm(struct atl1c_hw *hw, bool linkup)
pm_ctrl_data &= ~PM_CTRL_SERDES_PLL_L1_EN;
pm_ctrl_data |= PM_CTRL_CLK_SWH_L1;
-
+ pm_ctrl_data |= 0xF << PM_CTRL_LCKDET_TIMER_SHIFT;
if (hw->ctrl_flags & ATL1C_ASPM_L1_SUPPORT)
pm_ctrl_data |= PM_CTRL_ASPM_L1_EN;
else
pm_ctrl_data &= ~PM_CTRL_ASPM_L1_EN;
}
-
AT_WRITE_REG(hw, REG_PM_CTRL, pm_ctrl_data);
}
@@ -2301,10 +2334,9 @@ static int atl1c_suspend(struct pci_dev *pdev, pm_message_t state)
struct net_device *netdev = pci_get_drvdata(pdev);
struct atl1c_adapter *adapter = netdev_priv(netdev);
struct atl1c_hw *hw = &adapter->hw;
- u32 ctrl;
u32 mac_ctrl_data;
u32 master_ctrl_data;
- u32 wol_ctrl_data;
+ u32 wol_ctrl_data = 0;
u16 mii_bmsr_data;
u16 save_autoneg_advertised;
u16 mii_intr_status_data;
@@ -2321,23 +2353,29 @@ static int atl1c_suspend(struct pci_dev *pdev, pm_message_t state)
retval = pci_save_state(pdev);
if (retval)
return retval;
- if (wufc) {
- AT_READ_REG(hw, REG_MASTER_CTRL, &master_ctrl_data);
- master_ctrl_data &= ~MASTER_CTRL_CLK_SEL_DIS;
/* get link status */
- atl1c_read_phy_reg(hw, MII_BMSR, (u16 *)&mii_bmsr_data);
- atl1c_read_phy_reg(hw, MII_BMSR, (u16 *)&mii_bmsr_data);
- save_autoneg_advertised = hw->autoneg_advertised;
- hw->autoneg_advertised = ADVERTISED_10baseT_Half;
- if (atl1c_restart_autoneg(hw) != 0)
- if (netif_msg_link(adapter))
- dev_warn(&pdev->dev, "phy autoneg failed\n");
- hw->phy_configured = false; /* re-init PHY when resume */
- hw->autoneg_advertised = save_autoneg_advertised;
+ atl1c_read_phy_reg(hw, MII_BMSR, (u16 *)&mii_bmsr_data);
+ atl1c_read_phy_reg(hw, MII_BMSR, (u16 *)&mii_bmsr_data);
+
+ save_autoneg_advertised = hw->autoneg_advertised;
+ hw->autoneg_advertised = ADVERTISED_10baseT_Full;
+ if (atl1c_restart_autoneg(hw) != 0)
+ dev_dbg(&pdev->dev, "phy autoneg failed\n");
+ hw->phy_configured = false; /* re-init PHY when resume */
+ hw->autoneg_advertised = save_autoneg_advertised;
+ AT_READ_REG(hw, REG_MASTER_CTRL, &master_ctrl_data);
+ AT_READ_REG(hw, REG_MAC_CTRL, &mac_ctrl_data);
+ mac_ctrl_data &= ~(MAC_CTRL_PRMLEN_MASK << MAC_CTRL_PRMLEN_SHIFT);
+ mac_ctrl_data |= (((u32)adapter->hw.preamble_len &
+ MAC_CTRL_PRMLEN_MASK) <<
+ MAC_CTRL_PRMLEN_SHIFT);
+
+ if (wufc) {
+ master_ctrl_data &= ~MASTER_CTRL_CLK_SEL_DIS;
/* turn on magic packet wol */
if (wufc & AT_WUFC_MAG)
- wol_ctrl_data = WOL_MAGIC_EN | WOL_MAGIC_PME_EN;
+ wol_ctrl_data |= WOL_MAGIC_EN | WOL_MAGIC_PME_EN;
if (wufc & AT_WUFC_LNKC) {
for (i = 0; i < AT_SUSPEND_LINK_TIMEOUT; i++) {
@@ -2367,13 +2405,10 @@ static int atl1c_suspend(struct pci_dev *pdev, pm_message_t state)
/* clear phy interrupt */
atl1c_read_phy_reg(hw, MII_ISR, &mii_intr_status_data);
/* Config MAC Ctrl register */
- mac_ctrl_data = MAC_CTRL_RX_EN;
+ mac_ctrl_data |= MAC_CTRL_RX_EN;
/* set to 10/100M halt duplex */
mac_ctrl_data |= atl1c_mac_speed_10_100 << MAC_CTRL_SPEED_SHIFT;
- mac_ctrl_data |= (((u32)adapter->hw.preamble_len &
- MAC_CTRL_PRMLEN_MASK) <<
- MAC_CTRL_PRMLEN_SHIFT);
-
+ mac_ctrl_data |= MAC_CTRL_DUPLX;
if (adapter->vlgrp)
mac_ctrl_data |= MAC_CTRL_RMV_VLAN;
@@ -2390,23 +2425,29 @@ static int atl1c_suspend(struct pci_dev *pdev, pm_message_t state)
AT_WRITE_REG(hw, REG_MAC_CTRL, mac_ctrl_data);
/* pcie patch */
- AT_READ_REG(hw, REG_PCIE_PHYMISC, &ctrl);
- ctrl |= PCIE_PHYMISC_FORCE_RCV_DET;
- AT_WRITE_REG(hw, REG_PCIE_PHYMISC, ctrl);
+ atl1c_pcie_patch(hw);
+ device_set_wakeup_enable(&pdev->dev, 1);
+
+ AT_WRITE_REG(hw, REG_GPHY_CTRL, GPHY_CTRL_DEFAULT |
+ GPHY_CTRL_EXT_RESET);
pci_enable_wake(pdev, pci_choose_state(pdev, state), 1);
goto suspend_exit;
}
+ AT_WRITE_REG(hw, REG_GPHY_CTRL, GPHY_CTRL_POWER_SAVING);
+
+ master_ctrl_data |= MASTER_CTRL_CLK_SEL_DIS;
+ mac_ctrl_data |= atl1c_mac_speed_10_100 << MAC_CTRL_SPEED_SHIFT;
+ mac_ctrl_data |= MAC_CTRL_DUPLX;
+ AT_WRITE_REG(hw, REG_MASTER_CTRL, master_ctrl_data);
+ AT_WRITE_REG(hw, REG_MAC_CTRL, mac_ctrl_data);
wol_dis:
/* WOL disabled */
AT_WRITE_REG(hw, REG_WOL_CTRL, 0);
/* pcie patch */
- AT_READ_REG(hw, REG_PCIE_PHYMISC, &ctrl);
- ctrl |= PCIE_PHYMISC_FORCE_RCV_DET;
- AT_WRITE_REG(hw, REG_PCIE_PHYMISC, ctrl);
-
+ atl1c_pcie_patch(hw);
atl1c_phy_disable(hw);
hw->phy_configured = false; /* re-init PHY when resume */
^ permalink raw reply related
* Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0
From: Badalian Vyacheslav @ 2009-08-04 7:57 UTC (permalink / raw)
To: "Chia-chi Yeh (葉家齊)"
Cc: David Miller, john.dykstra1, akpm, linux-kernel, netdev
In-Reply-To: <6c039e090908040036j38666152y2db1d4c55529eaff@mail.gmail.com>
Chia-chi Yeh (葉家齊) пишет:
> After thinking more deeply, I believe that IPv6 does the right thing
> and IPv4 does not. SO_BINDTODEVICE requires CAP_NET_RAW, so it is a
> privileged operation. Therefore, it looks weird to me if one can
> specify other interface than the bound one without the same
> capability. The following patch makes the behavior in IPv4 and IPv6
> identical. Thanks for your help.
>
> Chia-chi
>
> --- a/net/ipv4/ip_sockglue.c 2009-08-04 15:11:39.000000000 +0800
> +++ b/net/ipv4/ip_sockglue.c 2009-08-04 15:17:05.000000000 +0800
> @@ -213,7 +213,11 @@
> if (cmsg->cmsg_len != CMSG_LEN(sizeof(struct
> in_pktinfo)))
> return -EINVAL;
> info = (struct in_pktinfo *)CMSG_DATA(cmsg);
> - ipc->oif = info->ipi_ifindex;
> + if (info->ipi_ifindex) {
> + if (ipc->oif && info->ipi_ifindex != ipc->oif)
> + return -EINVAL;
> + ipc->oif = info->ipi_ifindex;
Hello
Sorry if its my mistake or i someone not understand :)
if (ipc->oif && info->ipi_ifindex != ipc->oif)
// if match: info->ipi_ifindex != ipc->oif
return ...
else
// else match: info->ipi_ifindex == ipc->oif
// but you do
ipc->oif = info->ipi_ifindex;
// why if you else match allready check for it?
Thanks
> + }
> ipc->addr = info->ipi_spec_dst.s_addr;
> break;
> }
>
> On Tue, Aug 4, 2009 at 12:23 PM, David Miller<davem@davemloft.net> wrote:
>> From: John Dykstra <john.dykstra1@gmail.com>
>> Date: Wed, 29 Jul 2009 19:10:21 -0500
>>
>>> I guess Dave's letting this stand. I'm posting this just to make sure
>>> this is an explicit decision.
>> I'm still thinking about this.
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply
* Re: [PATCH]atl1c:Do not call cancel_work_sync from the work itself
From: Andrew Morton @ 2009-08-04 7:52 UTC (permalink / raw)
To: jie.yang; +Cc: davem, netdev, linux-kernel
In-Reply-To: <12493523461127-git-send-email-jie.yang@atheros.com>
On Tue, 4 Aug 2009 10:19:06 +0800 <jie.yang@atheros.com> wrote:
> Do not call cancel_work_sync from the work itself, for it my cause
> recursive locking.
> detail info:
> events/1/10 is trying to acquire lock:
> (&adapter->reset_task){+.+...}, at: [<c043e384>] __cancel_work_timer+0x80/0x187
>
> but task is already holding lock:
> (&adapter->reset_task){+.+...}, at: [<c043ed6f>] worker_thread+0x127/0x234
> other info that might help us debug this:
>
> 2 locks held by events/1/10:
> #0: (events){+.+.+.}, at: [<c043ed6f>] worker_thread+0x127/0x234
> #1: (&adapter->reset_task){+.+...}, at: [<c043ed6f>] worker_thread+0x127/0x234
>
> stack backtrace:
> Pid: 10, comm: events/1 Not tainted 2.6.31-rc2 #12
> Call Trace:
> [<c04519c9>] validate_chain+0x4ae/0xb26
> [<c0451e8d>] ? validate_chain+0x972/0xb26
> [<c0437f4b>] ? lock_timer_base+0x1f/0x3e
> [<c04526f8>] __lock_acquire+0x6b7/0x745
> [<c0452816>] lock_acquire+0x90/0xad
> [<c043e384>] ? __cancel_work_timer+0x80/0x187
> [<c043e3b1>] __cancel_work_timer+0xad/0x187
> [<c043e384>] ? __cancel_work_timer+0x80/0x187
> [<c044f6b3>] ? mark_held_locks+0x3d/0x58
> [<c0690855>] ? _spin_unlock_irqrestore+0x36/0x3c
> [<c044f7d5>] ? trace_hardirqs_on_caller+0x107/0x12f
> [<c044f808>] ? trace_hardirqs_on+0xb/0xd
> [<c0437fb2>] ? try_to_del_timer_sync+0x48/0x4f
> [<c043e4a2>] cancel_work_sync+0xa/0xc
> [<f8955f95>] atl1c_down+0x1f/0xde [atl1c]
> [<f8956955>] atl1c_reset_task+0x1f/0x31 [atl1c]
> [<c043edae>] worker_thread+0x166/0x234
> [<c043ed6f>] ? worker_thread+0x127/0x234
> [<f8956936>] ? atl1c_reset_task+0x0/0x31 [atl1c]
> [<c0441ac5>] ? autoremove_wake_function+0x0/0x33
> [<c043ec48>] ? worker_thread+0x0/0x234
> [<c0441a25>] kthread+0x69/0x70
> [<c04419bc>] ? kthread+0x0/0x70
> [<c04034b7>] kernel_thread_helper+0x7/0x10
>
> So when atl1c_reset_task be scheduled just set a flag in ctrl_flag,
> to demonstrate it is in reset_task, when atl1c_down is call it will not call
> cancel_work_sync(&adapter->reset_task) if it sees the flag.
>
> Signed-off-by: jie yang <jie.yang@atheros.com>
> ---
> diff --git a/drivers/net/atl1c/atl1c.h b/drivers/net/atl1c/atl1c.h
> index 2a1120a..53242dc 100644
> --- a/drivers/net/atl1c/atl1c.h
> +++ b/drivers/net/atl1c/atl1c.h
> @@ -427,6 +427,7 @@ struct atl1c_hw {
> #define ATL1C_ASPM_CTRL_MON 0x0200
> #define ATL1C_HIB_DISABLE 0x0400
> #define ATL1C_LINK_CAP_1000M 0x0800
> +#define ATL1C_RESET_IN_WORK 0x1000
> #define ATL1C_FPGA_VERSION 0x8000
> u16 cmb_tpd;
> u16 cmb_rrd;
> diff --git a/drivers/net/atl1c/atl1c_main.c b/drivers/net/atl1c/atl1c_main.c
> index 1d601ce..dec88fa 100644
> --- a/drivers/net/atl1c/atl1c_main.c
> +++ b/drivers/net/atl1c/atl1c_main.c
> @@ -321,7 +321,10 @@ static void atl1c_del_timer(struct atl1c_adapter *adapter)
>
> static void atl1c_cancel_work(struct atl1c_adapter *adapter)
> {
> - cancel_work_sync(&adapter->reset_task);
> + if (adapter->hw.ctrl_flags & ATL1C_RESET_IN_WORK)
> + adapter->hw.ctrl_flags &= ~ATL1C_RESET_IN_WORK;/* clear the flag */
> + else
> + cancel_work_sync(&adapter->reset_task);
> cancel_work_sync(&adapter->link_chg_task);
> }
>
> @@ -1544,6 +1547,7 @@ static irqreturn_t atl1c_intr(int irq, void *data)
> /* reset MAC */
> hw->intr_mask &= ~ISR_ERROR;
> AT_WRITE_REG(hw, REG_IMR, hw->intr_mask);
> + adapter->hw.ctrl_flags |= ATL1C_RESET_IN_WORK;
> schedule_work(&adapter->reset_task);
> break;
> }
The use of non-atomic bit operations upon ctrl_flags looks unsafe. If
a CPU is running atl1c_cancel_work() and then an interrupt happens (on
this CPU or a different one), then the interrupt will set
ATL1C_RESET_IN_WORK. But the non-atomic operation in
atl1c_cancel_work() can just overwrite that change.
^ permalink raw reply
* Re: Kernel oops on setting sky2 interfaces down
From: Rene Mayrhofer @ 2009-08-04 7:38 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, Richard Leitner, mikem
In-Reply-To: <4A772A1D.1030904@mayrhofer.eu.org>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Rene Mayrhofer wrote:
>> To allow easier debugging, I have now put our whole kernel tree up in a
>> public (read-only) git repository at
>> https://www.gibraltar.at/git/linux-2.6-gibraltar.git. The branch for
>> this kernel is origin/gibraltar-3.0, although the above dump was
>> produced by a version slightly "older" then HEAD, which did not yet have
>> the latest PaX patch applied (no PaX and no lzma-squashfs in this kernel).
> I have now updated the branch with both patches (the one from Stephen
> and the other one Mike). Still trying if it changes anything with
> 2.6.30.4 (they didn't help with 2.6.28.10, though...).
Result with both patches: there is no immediate crash when setting all
sky2 interfaces down, but I get the following messages repeated roughly
every second:
2009-08-04T09:35:31.030812+02:00 gibraltar3-esys-master kernel: [
592.000071] sky2 0000:01:00.0: device status error
2009-08-04T09:35:32.030908+02:00 gibraltar3-esys-master kernel: [
593.000058] sky2 0000:01:00.0: device status error
2009-08-04T09:35:33.030839+02:00 gibraltar3-esys-master kernel: [
594.000082] sky2 0000:01:00.0: device status error
2009-08-04T09:35:34.030864+02:00 gibraltar3-esys-master kernel: [
595.000118] sky2 0000:01:00.0: device status error
2009-08-04T09:35:35.030975+02:00 gibraltar3-esys-master kernel: [
596.000259] sky2 0000:01:00.0: device status error
2009-08-04T09:35:36.030974+02:00 gibraltar3-esys-master kernel: [
597.000198] sky2 0000:01:00.0: device status error
2009-08-04T09:35:37.030980+02:00 gibraltar3-esys-master kernel: [
598.000203] sky2 0000:01:00.0: device status error
and the network interface fails to work (no ping, nothing with tcpdump,
etc.).
Does anybody have an idea on what might be wrong in sky2_down?
best regards,
Rene
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkp35WsACgkQq7SPDcPCS97wHQCcCYWO2qgg+LdW+BFUmeOXjGVT
B68AniD3Ur2NugPGhuvz3Fxy68Zl+3f4
=5MhE
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0
From: Chia-chi Yeh (葉家齊) @ 2009-08-04 7:36 UTC (permalink / raw)
To: David Miller; +Cc: john.dykstra1, akpm, linux-kernel, netdev
In-Reply-To: <20090803.212323.256579233.davem@davemloft.net>
After thinking more deeply, I believe that IPv6 does the right thing
and IPv4 does not. SO_BINDTODEVICE requires CAP_NET_RAW, so it is a
privileged operation. Therefore, it looks weird to me if one can
specify other interface than the bound one without the same
capability. The following patch makes the behavior in IPv4 and IPv6
identical. Thanks for your help.
Chia-chi
--- a/net/ipv4/ip_sockglue.c 2009-08-04 15:11:39.000000000 +0800
+++ b/net/ipv4/ip_sockglue.c 2009-08-04 15:17:05.000000000 +0800
@@ -213,7 +213,11 @@
if (cmsg->cmsg_len != CMSG_LEN(sizeof(struct
in_pktinfo)))
return -EINVAL;
info = (struct in_pktinfo *)CMSG_DATA(cmsg);
- ipc->oif = info->ipi_ifindex;
+ if (info->ipi_ifindex) {
+ if (ipc->oif && info->ipi_ifindex != ipc->oif)
+ return -EINVAL;
+ ipc->oif = info->ipi_ifindex;
+ }
ipc->addr = info->ipi_spec_dst.s_addr;
break;
}
On Tue, Aug 4, 2009 at 12:23 PM, David Miller<davem@davemloft.net> wrote:
> From: John Dykstra <john.dykstra1@gmail.com>
> Date: Wed, 29 Jul 2009 19:10:21 -0500
>
>> I guess Dave's letting this stand. I'm posting this just to make sure
>> this is an explicit decision.
>
> I'm still thinking about this.
>
^ permalink raw reply
* Re: [RFC] [PATCH] Avoid enqueuing skb for default qdiscs
From: Jarek Poplawski @ 2009-08-04 7:22 UTC (permalink / raw)
To: Krishna Kumar2; +Cc: davem, herbert, kaber, netdev
In-Reply-To: <OF104CB719.2A67F34E-ON65257608.001D0048-65257608.0026016D@in.ibm.com>
On Tue, Aug 04, 2009 at 12:25:07PM +0530, Krishna Kumar2 wrote:
> Jarek Poplawski <jarkao2@gmail.com> wrote on 08/04/2009 01:08:47 AM:
>
> Hi Jarek,
>
> > Since this quoted part looks very similarly I'm using it here, so
> > forget my note if something has changed.
>
> Thanks for your feedback. Yes, the inline patch is the same as the
> attached one.
>
> > >> @@ -306,6 +312,9 @@ extern struct Qdisc *qdisc_create_dflt(s
> > >> struct Qdisc_ops *ops, u32 parentid);
> > >> extern void qdisc_calculate_pkt_len(struct sk_buff *skb,
> > >> struct qdisc_size_table *stab);
> > >> +extern int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
> > >> + struct net_device *dev, struct netdev_queue *txq,
> > >> + spinlock_t *root_lock);
> >
> > Probably this would look better in pkt_sched.h around __qdisc_run()?
>
> I had it originally in pkt_sched.h, for some reason I had moved it to
> sch_generic.h, I will move it back.
>
> > >> +static inline int __dev_hw_xmit(struct sk_buff *skb, struct Qdisc *q,
> >
> > I guess, we don't touch "hw" in this function yet?
>
> Right - this was due to lack of a good name. Maybe I can call it
> __dev_xmit_skb?
Looks better to me.
>
> > >> - kfree_skb(qdisc->gso_skb);
> > >> - qdisc->gso_skb = NULL;
> > >> + if (qdisc->gso_skb) {
> > >> + kfree_skb(qdisc->gso_skb);
> > >> + qdisc->gso_skb = NULL;
> > >> + qdisc->q.qlen--;
> >
> > You don't need this here: qdiscs should zero it in ops->reset().
>
> I will keep it around, as you said in your other mail.
Actually I meant:
- qdisc->q.qlen--;
+ qdisc->q.qlen = 0;
because you can do it after another qdisc->q.qlen = 0.
>
> > >> + /* Can by-pass the queue discipline for default qdisc */
> > >> + qdisc->flags = TCQ_F_CAN_BYPASS;
> >
> > "|=" looks nicer to me.
>
> Agreed - and required if qdisc_create_dflt (and the functions it calls)
> is changed to set the flags.
>
> > >> } else {
> > >> qdisc = &noqueue_qdisc;
> > >> }
> >
> > I wonder if we shouldn't update bstats yet. Btw, try checkpatch.
>
> Rate estimator isn't used for default qdisc, so is anything
> required?
I meant "Sent" stats from "tc -s qdisc sh" could be misleading.
> Also, checkpatch suggested to change the original code:
> "if (unlikely((skb = dequeue_skb(q)) == NULL))"
> since it appears as "new code", hope that change is fine.
I mainly meant a whitespace warning.
>
> The patch after incorporating the review comments is both attached
> and inlined, since my mailer is still not working (I sent this mail
> about an hour back but this one didn't seem to have made it while
> one of my latter mails did).
I got 2 very similar messages this time, several minutes ago ;-)
Thanks,
Jarek P.
>
> Thanks,
>
> - KK
>
> (See attached file: patch4)
>
> ----------------------------------------------------------------------
> include/net/pkt_sched.h | 3 +
> include/net/sch_generic.h | 6 ++
> net/core/dev.c | 46 ++++++++++++-----
> net/sched/sch_generic.c | 93 ++++++++++++++++++++++--------------
> 4 files changed, 99 insertions(+), 49 deletions(-)
>
> diff -ruNp org/include/net/pkt_sched.h new/include/net/pkt_sched.h
> --- org/include/net/pkt_sched.h 2009-08-04 11:41:21.000000000 +0530
> +++ new/include/net/pkt_sched.h 2009-08-04 11:42:18.000000000 +0530
> @@ -87,6 +87,9 @@ extern struct qdisc_rate_table *qdisc_ge
> extern void qdisc_put_rtab(struct qdisc_rate_table *tab);
> extern void qdisc_put_stab(struct qdisc_size_table *tab);
> extern void qdisc_warn_nonwc(char *txt, struct Qdisc *qdisc);
> +extern int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
> + struct net_device *dev, struct netdev_queue
> *txq,
> + spinlock_t *root_lock);
>
> extern void __qdisc_run(struct Qdisc *q);
>
> diff -ruNp org/include/net/sch_generic.h new/include/net/sch_generic.h
> --- org/include/net/sch_generic.h 2009-03-24 08:54:16.000000000 +0530
> +++ new/include/net/sch_generic.h 2009-08-04 11:02:39.000000000 +0530
> @@ -45,6 +45,7 @@ struct Qdisc
> #define TCQ_F_BUILTIN 1
> #define TCQ_F_THROTTLED 2
> #define TCQ_F_INGRESS 4
> +#define TCQ_F_CAN_BYPASS 8
> #define TCQ_F_WARN_NONWC (1 << 16)
> int padded;
> struct Qdisc_ops *ops;
> @@ -182,6 +183,11 @@ struct qdisc_skb_cb {
> char data[];
> };
>
> +static inline int qdisc_qlen(struct Qdisc *q)
> +{
> + return q->q.qlen;
> +}
> +
> static inline struct qdisc_skb_cb *qdisc_skb_cb(struct sk_buff *skb)
> {
> return (struct qdisc_skb_cb *)skb->cb;
> diff -ruNp org/net/core/dev.c new/net/core/dev.c
> --- org/net/core/dev.c 2009-07-27 09:08:24.000000000 +0530
> +++ new/net/core/dev.c 2009-08-04 11:44:57.000000000 +0530
> @@ -1786,6 +1786,38 @@ static struct netdev_queue *dev_pick_tx(
> return netdev_get_tx_queue(dev, queue_index);
> }
>
> +static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
> + struct net_device *dev,
> + struct netdev_queue *txq)
> +{
> + spinlock_t *root_lock = qdisc_lock(q);
> + int rc;
> +
> + spin_lock(root_lock);
> + if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED, &q->state))) {
> + kfree_skb(skb);
> + rc = NET_XMIT_DROP;
> + } else if ((q->flags & TCQ_F_CAN_BYPASS) && !qdisc_qlen(q) &&
> + !test_and_set_bit(__QDISC_STATE_RUNNING, &q->state)) {
> + /*
> + * This is a work-conserving queue; there are no old skbs
> + * waiting to be sent out; and the qdisc is not running -
> + * xmit the skb directly.
> + */
> + if (sch_direct_xmit(skb, q, dev, txq, root_lock))
> + __qdisc_run(q);
> + else
> + clear_bit(__QDISC_STATE_RUNNING, &q->state);
> + rc = NET_XMIT_SUCCESS;
> + } else {
> + rc = qdisc_enqueue_root(skb, q);
> + qdisc_run(q);
> + }
> + spin_unlock(root_lock);
> +
> + return rc;
> +}
> +
> /**
> * dev_queue_xmit - transmit a buffer
> * @skb: buffer to transmit
> @@ -1859,19 +1891,7 @@ gso:
> skb->tc_verd = SET_TC_AT(skb->tc_verd,AT_EGRESS);
> #endif
> if (q->enqueue) {
> - spinlock_t *root_lock = qdisc_lock(q);
> -
> - spin_lock(root_lock);
> -
> - if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED,
> &q->state))) {
> - kfree_skb(skb);
> - rc = NET_XMIT_DROP;
> - } else {
> - rc = qdisc_enqueue_root(skb, q);
> - qdisc_run(q);
> - }
> - spin_unlock(root_lock);
> -
> + rc = __dev_xmit_skb(skb, q, dev, txq);
> goto out;
> }
>
> diff -ruNp org/net/sched/sch_generic.c new/net/sched/sch_generic.c
> --- org/net/sched/sch_generic.c 2009-05-25 07:48:07.000000000 +0530
> +++ new/net/sched/sch_generic.c 2009-08-04 11:00:38.000000000 +0530
> @@ -37,15 +37,11 @@
> * - updates to tree and tree walking are only done under the rtnl mutex.
> */
>
> -static inline int qdisc_qlen(struct Qdisc *q)
> -{
> - return q->q.qlen;
> -}
> -
> static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
> {
> q->gso_skb = skb;
> q->qstats.requeues++;
> + q->q.qlen++; /* it's still part of the queue */
> __netif_schedule(q);
>
> return 0;
> @@ -61,9 +57,11 @@ static inline struct sk_buff *dequeue_sk
>
> /* check the reason of requeuing without tx lock first */
> txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
> - if (!netif_tx_queue_stopped(txq) &&
> !netif_tx_queue_frozen(txq))
> + if (!netif_tx_queue_stopped(txq) &&
> + !netif_tx_queue_frozen(txq)) {
> q->gso_skb = NULL;
> - else
> + q->q.qlen--;
> + } else
> skb = NULL;
> } else {
> skb = q->dequeue(q);
> @@ -103,44 +101,23 @@ static inline int handle_dev_cpu_collisi
> }
>
> /*
> - * NOTE: Called under qdisc_lock(q) with locally disabled BH.
> - *
> - * __QDISC_STATE_RUNNING guarantees only one CPU can process
> - * this qdisc at a time. qdisc_lock(q) serializes queue accesses for
> - * this queue.
> - *
> - * netif_tx_lock serializes accesses to device driver.
> - *
> - * qdisc_lock(q) and netif_tx_lock are mutually exclusive,
> - * if one is grabbed, another must be free.
> - *
> - * Note, that this procedure can be called by a watchdog timer
> + * Transmit one skb, and handle the return status as required. Holding the
> + * __QDISC_STATE_RUNNING bit guarantees that only one CPU can execute this
> + * function.
> *
> * Returns to the caller:
> * 0 - queue is empty or throttled.
> * >0 - queue is not empty.
> - *
> */
> -static inline int qdisc_restart(struct Qdisc *q)
> +int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
> + struct net_device *dev, struct netdev_queue *txq,
> + spinlock_t *root_lock)
> {
> - struct netdev_queue *txq;
> int ret = NETDEV_TX_BUSY;
> - struct net_device *dev;
> - spinlock_t *root_lock;
> - struct sk_buff *skb;
> -
> - /* Dequeue packet */
> - if (unlikely((skb = dequeue_skb(q)) == NULL))
> - return 0;
> -
> - root_lock = qdisc_lock(q);
>
> /* And release qdisc */
> spin_unlock(root_lock);
>
> - dev = qdisc_dev(q);
> - txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
> -
> HARD_TX_LOCK(dev, txq, smp_processor_id());
> if (!netif_tx_queue_stopped(txq) &&
> !netif_tx_queue_frozen(txq))
> @@ -177,6 +154,44 @@ static inline int qdisc_restart(struct Q
> return ret;
> }
>
> +/*
> + * NOTE: Called under qdisc_lock(q) with locally disabled BH.
> + *
> + * __QDISC_STATE_RUNNING guarantees only one CPU can process
> + * this qdisc at a time. qdisc_lock(q) serializes queue accesses for
> + * this queue.
> + *
> + * netif_tx_lock serializes accesses to device driver.
> + *
> + * qdisc_lock(q) and netif_tx_lock are mutually exclusive,
> + * if one is grabbed, another must be free.
> + *
> + * Note, that this procedure can be called by a watchdog timer
> + *
> + * Returns to the caller:
> + * 0 - queue is empty or throttled.
> + * >0 - queue is not empty.
> + *
> + */
> +static inline int qdisc_restart(struct Qdisc *q)
> +{
> + struct netdev_queue *txq;
> + struct net_device *dev;
> + spinlock_t *root_lock;
> + struct sk_buff *skb;
> +
> + /* Dequeue packet */
> + skb = dequeue_skb(q);
> + if (unlikely(!skb))
> + return 0;
> +
> + root_lock = qdisc_lock(q);
> + dev = qdisc_dev(q);
> + txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
> +
> + return sch_direct_xmit(skb, q, dev, txq, root_lock);
> +}
> +
> void __qdisc_run(struct Qdisc *q)
> {
> unsigned long start_time = jiffies;
> @@ -547,8 +562,11 @@ void qdisc_reset(struct Qdisc *qdisc)
> if (ops->reset)
> ops->reset(qdisc);
>
> - kfree_skb(qdisc->gso_skb);
> - qdisc->gso_skb = NULL;
> + if (qdisc->gso_skb) {
> + kfree_skb(qdisc->gso_skb);
> + qdisc->gso_skb = NULL;
> + qdisc->q.qlen--;
> + }
> }
> EXPORT_SYMBOL(qdisc_reset);
>
> @@ -605,6 +623,9 @@ static void attach_one_default_qdisc(str
> printk(KERN_INFO "%s: activation failed\n",
> dev->name);
> return;
> }
> +
> + /* Can by-pass the queue discipline for default qdisc */
> + qdisc->flags |= TCQ_F_CAN_BYPASS;
> } else {
> qdisc = &noqueue_qdisc;
> }
^ permalink raw reply
* Re: [RFC] [PATCH] Avoid enqueuing skb for default qdiscs
From: Krishna Kumar2 @ 2009-08-04 6:55 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: davem, herbert, kaber, netdev
In-Reply-To: <20090803193847.GA3471@ami.dom.local>
[-- Attachment #1: Type: text/plain, Size: 11318 bytes --]
Jarek Poplawski <jarkao2@gmail.com> wrote on 08/04/2009 01:08:47 AM:
Hi Jarek,
> Since this quoted part looks very similarly I'm using it here, so
> forget my note if something has changed.
Thanks for your feedback. Yes, the inline patch is the same as the
attached one.
> >> @@ -306,6 +312,9 @@ extern struct Qdisc *qdisc_create_dflt(s
> >> struct Qdisc_ops *ops, u32 parentid);
> >> extern void qdisc_calculate_pkt_len(struct sk_buff *skb,
> >> struct qdisc_size_table *stab);
> >> +extern int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
> >> + struct net_device *dev, struct netdev_queue *txq,
> >> + spinlock_t *root_lock);
>
> Probably this would look better in pkt_sched.h around __qdisc_run()?
I had it originally in pkt_sched.h, for some reason I had moved it to
sch_generic.h, I will move it back.
> >> +static inline int __dev_hw_xmit(struct sk_buff *skb, struct Qdisc *q,
>
> I guess, we don't touch "hw" in this function yet?
Right - this was due to lack of a good name. Maybe I can call it
__dev_xmit_skb?
> >> - kfree_skb(qdisc->gso_skb);
> >> - qdisc->gso_skb = NULL;
> >> + if (qdisc->gso_skb) {
> >> + kfree_skb(qdisc->gso_skb);
> >> + qdisc->gso_skb = NULL;
> >> + qdisc->q.qlen--;
>
> You don't need this here: qdiscs should zero it in ops->reset().
I will keep it around, as you said in your other mail.
> >> + /* Can by-pass the queue discipline for default qdisc */
> >> + qdisc->flags = TCQ_F_CAN_BYPASS;
>
> "|=" looks nicer to me.
Agreed - and required if qdisc_create_dflt (and the functions it calls)
is changed to set the flags.
> >> } else {
> >> qdisc = &noqueue_qdisc;
> >> }
>
> I wonder if we shouldn't update bstats yet. Btw, try checkpatch.
Rate estimator isn't used for default qdisc, so is anything
required? Also, checkpatch suggested to change the original code:
"if (unlikely((skb = dequeue_skb(q)) == NULL))"
since it appears as "new code", hope that change is fine.
The patch after incorporating the review comments is both attached
and inlined, since my mailer is still not working (I sent this mail
about an hour back but this one didn't seem to have made it while
one of my latter mails did).
Thanks,
- KK
(See attached file: patch4)
----------------------------------------------------------------------
include/net/pkt_sched.h | 3 +
include/net/sch_generic.h | 6 ++
net/core/dev.c | 46 ++++++++++++-----
net/sched/sch_generic.c | 93 ++++++++++++++++++++++--------------
4 files changed, 99 insertions(+), 49 deletions(-)
diff -ruNp org/include/net/pkt_sched.h new/include/net/pkt_sched.h
--- org/include/net/pkt_sched.h 2009-08-04 11:41:21.000000000 +0530
+++ new/include/net/pkt_sched.h 2009-08-04 11:42:18.000000000 +0530
@@ -87,6 +87,9 @@ extern struct qdisc_rate_table *qdisc_ge
extern void qdisc_put_rtab(struct qdisc_rate_table *tab);
extern void qdisc_put_stab(struct qdisc_size_table *tab);
extern void qdisc_warn_nonwc(char *txt, struct Qdisc *qdisc);
+extern int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
+ struct net_device *dev, struct netdev_queue
*txq,
+ spinlock_t *root_lock);
extern void __qdisc_run(struct Qdisc *q);
diff -ruNp org/include/net/sch_generic.h new/include/net/sch_generic.h
--- org/include/net/sch_generic.h 2009-03-24 08:54:16.000000000 +0530
+++ new/include/net/sch_generic.h 2009-08-04 11:02:39.000000000 +0530
@@ -45,6 +45,7 @@ struct Qdisc
#define TCQ_F_BUILTIN 1
#define TCQ_F_THROTTLED 2
#define TCQ_F_INGRESS 4
+#define TCQ_F_CAN_BYPASS 8
#define TCQ_F_WARN_NONWC (1 << 16)
int padded;
struct Qdisc_ops *ops;
@@ -182,6 +183,11 @@ struct qdisc_skb_cb {
char data[];
};
+static inline int qdisc_qlen(struct Qdisc *q)
+{
+ return q->q.qlen;
+}
+
static inline struct qdisc_skb_cb *qdisc_skb_cb(struct sk_buff *skb)
{
return (struct qdisc_skb_cb *)skb->cb;
diff -ruNp org/net/core/dev.c new/net/core/dev.c
--- org/net/core/dev.c 2009-07-27 09:08:24.000000000 +0530
+++ new/net/core/dev.c 2009-08-04 11:44:57.000000000 +0530
@@ -1786,6 +1786,38 @@ static struct netdev_queue *dev_pick_tx(
return netdev_get_tx_queue(dev, queue_index);
}
+static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
+ struct net_device *dev,
+ struct netdev_queue *txq)
+{
+ spinlock_t *root_lock = qdisc_lock(q);
+ int rc;
+
+ spin_lock(root_lock);
+ if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED, &q->state))) {
+ kfree_skb(skb);
+ rc = NET_XMIT_DROP;
+ } else if ((q->flags & TCQ_F_CAN_BYPASS) && !qdisc_qlen(q) &&
+ !test_and_set_bit(__QDISC_STATE_RUNNING, &q->state)) {
+ /*
+ * This is a work-conserving queue; there are no old skbs
+ * waiting to be sent out; and the qdisc is not running -
+ * xmit the skb directly.
+ */
+ if (sch_direct_xmit(skb, q, dev, txq, root_lock))
+ __qdisc_run(q);
+ else
+ clear_bit(__QDISC_STATE_RUNNING, &q->state);
+ rc = NET_XMIT_SUCCESS;
+ } else {
+ rc = qdisc_enqueue_root(skb, q);
+ qdisc_run(q);
+ }
+ spin_unlock(root_lock);
+
+ return rc;
+}
+
/**
* dev_queue_xmit - transmit a buffer
* @skb: buffer to transmit
@@ -1859,19 +1891,7 @@ gso:
skb->tc_verd = SET_TC_AT(skb->tc_verd,AT_EGRESS);
#endif
if (q->enqueue) {
- spinlock_t *root_lock = qdisc_lock(q);
-
- spin_lock(root_lock);
-
- if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED,
&q->state))) {
- kfree_skb(skb);
- rc = NET_XMIT_DROP;
- } else {
- rc = qdisc_enqueue_root(skb, q);
- qdisc_run(q);
- }
- spin_unlock(root_lock);
-
+ rc = __dev_xmit_skb(skb, q, dev, txq);
goto out;
}
diff -ruNp org/net/sched/sch_generic.c new/net/sched/sch_generic.c
--- org/net/sched/sch_generic.c 2009-05-25 07:48:07.000000000 +0530
+++ new/net/sched/sch_generic.c 2009-08-04 11:00:38.000000000 +0530
@@ -37,15 +37,11 @@
* - updates to tree and tree walking are only done under the rtnl mutex.
*/
-static inline int qdisc_qlen(struct Qdisc *q)
-{
- return q->q.qlen;
-}
-
static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
{
q->gso_skb = skb;
q->qstats.requeues++;
+ q->q.qlen++; /* it's still part of the queue */
__netif_schedule(q);
return 0;
@@ -61,9 +57,11 @@ static inline struct sk_buff *dequeue_sk
/* check the reason of requeuing without tx lock first */
txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
- if (!netif_tx_queue_stopped(txq) &&
!netif_tx_queue_frozen(txq))
+ if (!netif_tx_queue_stopped(txq) &&
+ !netif_tx_queue_frozen(txq)) {
q->gso_skb = NULL;
- else
+ q->q.qlen--;
+ } else
skb = NULL;
} else {
skb = q->dequeue(q);
@@ -103,44 +101,23 @@ static inline int handle_dev_cpu_collisi
}
/*
- * NOTE: Called under qdisc_lock(q) with locally disabled BH.
- *
- * __QDISC_STATE_RUNNING guarantees only one CPU can process
- * this qdisc at a time. qdisc_lock(q) serializes queue accesses for
- * this queue.
- *
- * netif_tx_lock serializes accesses to device driver.
- *
- * qdisc_lock(q) and netif_tx_lock are mutually exclusive,
- * if one is grabbed, another must be free.
- *
- * Note, that this procedure can be called by a watchdog timer
+ * Transmit one skb, and handle the return status as required. Holding the
+ * __QDISC_STATE_RUNNING bit guarantees that only one CPU can execute this
+ * function.
*
* Returns to the caller:
* 0 - queue is empty or throttled.
* >0 - queue is not empty.
- *
*/
-static inline int qdisc_restart(struct Qdisc *q)
+int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
+ struct net_device *dev, struct netdev_queue *txq,
+ spinlock_t *root_lock)
{
- struct netdev_queue *txq;
int ret = NETDEV_TX_BUSY;
- struct net_device *dev;
- spinlock_t *root_lock;
- struct sk_buff *skb;
-
- /* Dequeue packet */
- if (unlikely((skb = dequeue_skb(q)) == NULL))
- return 0;
-
- root_lock = qdisc_lock(q);
/* And release qdisc */
spin_unlock(root_lock);
- dev = qdisc_dev(q);
- txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
-
HARD_TX_LOCK(dev, txq, smp_processor_id());
if (!netif_tx_queue_stopped(txq) &&
!netif_tx_queue_frozen(txq))
@@ -177,6 +154,44 @@ static inline int qdisc_restart(struct Q
return ret;
}
+/*
+ * NOTE: Called under qdisc_lock(q) with locally disabled BH.
+ *
+ * __QDISC_STATE_RUNNING guarantees only one CPU can process
+ * this qdisc at a time. qdisc_lock(q) serializes queue accesses for
+ * this queue.
+ *
+ * netif_tx_lock serializes accesses to device driver.
+ *
+ * qdisc_lock(q) and netif_tx_lock are mutually exclusive,
+ * if one is grabbed, another must be free.
+ *
+ * Note, that this procedure can be called by a watchdog timer
+ *
+ * Returns to the caller:
+ * 0 - queue is empty or throttled.
+ * >0 - queue is not empty.
+ *
+ */
+static inline int qdisc_restart(struct Qdisc *q)
+{
+ struct netdev_queue *txq;
+ struct net_device *dev;
+ spinlock_t *root_lock;
+ struct sk_buff *skb;
+
+ /* Dequeue packet */
+ skb = dequeue_skb(q);
+ if (unlikely(!skb))
+ return 0;
+
+ root_lock = qdisc_lock(q);
+ dev = qdisc_dev(q);
+ txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
+
+ return sch_direct_xmit(skb, q, dev, txq, root_lock);
+}
+
void __qdisc_run(struct Qdisc *q)
{
unsigned long start_time = jiffies;
@@ -547,8 +562,11 @@ void qdisc_reset(struct Qdisc *qdisc)
if (ops->reset)
ops->reset(qdisc);
- kfree_skb(qdisc->gso_skb);
- qdisc->gso_skb = NULL;
+ if (qdisc->gso_skb) {
+ kfree_skb(qdisc->gso_skb);
+ qdisc->gso_skb = NULL;
+ qdisc->q.qlen--;
+ }
}
EXPORT_SYMBOL(qdisc_reset);
@@ -605,6 +623,9 @@ static void attach_one_default_qdisc(str
printk(KERN_INFO "%s: activation failed\n",
dev->name);
return;
}
+
+ /* Can by-pass the queue discipline for default qdisc */
+ qdisc->flags |= TCQ_F_CAN_BYPASS;
} else {
qdisc = &noqueue_qdisc;
}
[-- Attachment #2: patch4 --]
[-- Type: application/octet-stream, Size: 9455 bytes --]
From: Krishna Kumar <krkumar2@in.ibm.com>
dev_queue_xmit enqueue's a skb and calls qdisc_run which
dequeue's the skb and xmits it. In most cases (after
instrumenting the code), the skb that is enqueue'd is the
same one that is dequeue'd (unless the queue gets stopped
or multiple cpu's write to the same queue and ends in a
race with qdisc_run). For default qdiscs, we can remove
this path and simply xmit the skb since this is a work
conserving queue.
The patch uses a new flag - TCQ_F_CAN_BYPASS to identify
the default fast queue. I plan to use this flag for the
previous patch also (rename if required). The controversial
part of the patch is incrementing qlen when a skb is
requeued, this is to avoid checks like the second line below:
+ } else if ((q->flags & TCQ_F_CAN_BYPASS) && !qdisc_qlen(q) &&
>> THIS LINE: !q->gso_skb &&
+ !test_and_set_bit(__QDISC_STATE_RUNNING, &q->state)) {
Results of a 4 hour testing for multiple netperf sessions
(1, 2, 4, 8, 12 sessions on a 4 cpu system-X and 1, 2, 4,
8, 16, 32 sessions on a 16 cpu P6). Aggregate Mb/s across
the iterations:
-----------------------------------------------------------------
| System-X | P6
-----------------------------------------------------------------
Size | ORG BW NEW BW | ORG BW NEW BW
-----|---------------------------|-------------------------------
16K | 154264 156234 | 155350 157569
64K | 154364 154825 | 155790 158845
128K | 154644 154803 | 153418 155572
256K | 153882 152007 | 154784 154596
-----------------------------------------------------------------
Netperf reported Service demand reduced by 15% on the P6 but
no noticeable difference on the system-X box.
Please review.
Thanks,
- KK
Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
include/net/pkt_sched.h | 3 +
include/net/sch_generic.h | 6 ++
net/core/dev.c | 46 ++++++++++++-----
net/sched/sch_generic.c | 93 ++++++++++++++++++++++--------------
4 files changed, 99 insertions(+), 49 deletions(-)
diff -ruNp org/include/net/pkt_sched.h new/include/net/pkt_sched.h
--- org/include/net/pkt_sched.h 2009-08-04 11:41:21.000000000 +0530
+++ new/include/net/pkt_sched.h 2009-08-04 11:42:18.000000000 +0530
@@ -87,6 +87,9 @@ extern struct qdisc_rate_table *qdisc_ge
extern void qdisc_put_rtab(struct qdisc_rate_table *tab);
extern void qdisc_put_stab(struct qdisc_size_table *tab);
extern void qdisc_warn_nonwc(char *txt, struct Qdisc *qdisc);
+extern int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
+ struct net_device *dev, struct netdev_queue *txq,
+ spinlock_t *root_lock);
extern void __qdisc_run(struct Qdisc *q);
diff -ruNp org/include/net/sch_generic.h new/include/net/sch_generic.h
--- org/include/net/sch_generic.h 2009-03-24 08:54:16.000000000 +0530
+++ new/include/net/sch_generic.h 2009-08-04 11:02:39.000000000 +0530
@@ -45,6 +45,7 @@ struct Qdisc
#define TCQ_F_BUILTIN 1
#define TCQ_F_THROTTLED 2
#define TCQ_F_INGRESS 4
+#define TCQ_F_CAN_BYPASS 8
#define TCQ_F_WARN_NONWC (1 << 16)
int padded;
struct Qdisc_ops *ops;
@@ -182,6 +183,11 @@ struct qdisc_skb_cb {
char data[];
};
+static inline int qdisc_qlen(struct Qdisc *q)
+{
+ return q->q.qlen;
+}
+
static inline struct qdisc_skb_cb *qdisc_skb_cb(struct sk_buff *skb)
{
return (struct qdisc_skb_cb *)skb->cb;
diff -ruNp org/net/core/dev.c new/net/core/dev.c
--- org/net/core/dev.c 2009-07-27 09:08:24.000000000 +0530
+++ new/net/core/dev.c 2009-08-04 11:44:57.000000000 +0530
@@ -1786,6 +1786,38 @@ static struct netdev_queue *dev_pick_tx(
return netdev_get_tx_queue(dev, queue_index);
}
+static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
+ struct net_device *dev,
+ struct netdev_queue *txq)
+{
+ spinlock_t *root_lock = qdisc_lock(q);
+ int rc;
+
+ spin_lock(root_lock);
+ if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED, &q->state))) {
+ kfree_skb(skb);
+ rc = NET_XMIT_DROP;
+ } else if ((q->flags & TCQ_F_CAN_BYPASS) && !qdisc_qlen(q) &&
+ !test_and_set_bit(__QDISC_STATE_RUNNING, &q->state)) {
+ /*
+ * This is a work-conserving queue; there are no old skbs
+ * waiting to be sent out; and the qdisc is not running -
+ * xmit the skb directly.
+ */
+ if (sch_direct_xmit(skb, q, dev, txq, root_lock))
+ __qdisc_run(q);
+ else
+ clear_bit(__QDISC_STATE_RUNNING, &q->state);
+ rc = NET_XMIT_SUCCESS;
+ } else {
+ rc = qdisc_enqueue_root(skb, q);
+ qdisc_run(q);
+ }
+ spin_unlock(root_lock);
+
+ return rc;
+}
+
/**
* dev_queue_xmit - transmit a buffer
* @skb: buffer to transmit
@@ -1859,19 +1891,7 @@ gso:
skb->tc_verd = SET_TC_AT(skb->tc_verd,AT_EGRESS);
#endif
if (q->enqueue) {
- spinlock_t *root_lock = qdisc_lock(q);
-
- spin_lock(root_lock);
-
- if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED, &q->state))) {
- kfree_skb(skb);
- rc = NET_XMIT_DROP;
- } else {
- rc = qdisc_enqueue_root(skb, q);
- qdisc_run(q);
- }
- spin_unlock(root_lock);
-
+ rc = __dev_xmit_skb(skb, q, dev, txq);
goto out;
}
diff -ruNp org/net/sched/sch_generic.c new/net/sched/sch_generic.c
--- org/net/sched/sch_generic.c 2009-05-25 07:48:07.000000000 +0530
+++ new/net/sched/sch_generic.c 2009-08-04 11:00:38.000000000 +0530
@@ -37,15 +37,11 @@
* - updates to tree and tree walking are only done under the rtnl mutex.
*/
-static inline int qdisc_qlen(struct Qdisc *q)
-{
- return q->q.qlen;
-}
-
static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
{
q->gso_skb = skb;
q->qstats.requeues++;
+ q->q.qlen++; /* it's still part of the queue */
__netif_schedule(q);
return 0;
@@ -61,9 +57,11 @@ static inline struct sk_buff *dequeue_sk
/* check the reason of requeuing without tx lock first */
txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
- if (!netif_tx_queue_stopped(txq) && !netif_tx_queue_frozen(txq))
+ if (!netif_tx_queue_stopped(txq) &&
+ !netif_tx_queue_frozen(txq)) {
q->gso_skb = NULL;
- else
+ q->q.qlen--;
+ } else
skb = NULL;
} else {
skb = q->dequeue(q);
@@ -103,44 +101,23 @@ static inline int handle_dev_cpu_collisi
}
/*
- * NOTE: Called under qdisc_lock(q) with locally disabled BH.
- *
- * __QDISC_STATE_RUNNING guarantees only one CPU can process
- * this qdisc at a time. qdisc_lock(q) serializes queue accesses for
- * this queue.
- *
- * netif_tx_lock serializes accesses to device driver.
- *
- * qdisc_lock(q) and netif_tx_lock are mutually exclusive,
- * if one is grabbed, another must be free.
- *
- * Note, that this procedure can be called by a watchdog timer
+ * Transmit one skb, and handle the return status as required. Holding the
+ * __QDISC_STATE_RUNNING bit guarantees that only one CPU can execute this
+ * function.
*
* Returns to the caller:
* 0 - queue is empty or throttled.
* >0 - queue is not empty.
- *
*/
-static inline int qdisc_restart(struct Qdisc *q)
+int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
+ struct net_device *dev, struct netdev_queue *txq,
+ spinlock_t *root_lock)
{
- struct netdev_queue *txq;
int ret = NETDEV_TX_BUSY;
- struct net_device *dev;
- spinlock_t *root_lock;
- struct sk_buff *skb;
-
- /* Dequeue packet */
- if (unlikely((skb = dequeue_skb(q)) == NULL))
- return 0;
-
- root_lock = qdisc_lock(q);
/* And release qdisc */
spin_unlock(root_lock);
- dev = qdisc_dev(q);
- txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
-
HARD_TX_LOCK(dev, txq, smp_processor_id());
if (!netif_tx_queue_stopped(txq) &&
!netif_tx_queue_frozen(txq))
@@ -177,6 +154,44 @@ static inline int qdisc_restart(struct Q
return ret;
}
+/*
+ * NOTE: Called under qdisc_lock(q) with locally disabled BH.
+ *
+ * __QDISC_STATE_RUNNING guarantees only one CPU can process
+ * this qdisc at a time. qdisc_lock(q) serializes queue accesses for
+ * this queue.
+ *
+ * netif_tx_lock serializes accesses to device driver.
+ *
+ * qdisc_lock(q) and netif_tx_lock are mutually exclusive,
+ * if one is grabbed, another must be free.
+ *
+ * Note, that this procedure can be called by a watchdog timer
+ *
+ * Returns to the caller:
+ * 0 - queue is empty or throttled.
+ * >0 - queue is not empty.
+ *
+ */
+static inline int qdisc_restart(struct Qdisc *q)
+{
+ struct netdev_queue *txq;
+ struct net_device *dev;
+ spinlock_t *root_lock;
+ struct sk_buff *skb;
+
+ /* Dequeue packet */
+ skb = dequeue_skb(q);
+ if (unlikely(!skb))
+ return 0;
+
+ root_lock = qdisc_lock(q);
+ dev = qdisc_dev(q);
+ txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
+
+ return sch_direct_xmit(skb, q, dev, txq, root_lock);
+}
+
void __qdisc_run(struct Qdisc *q)
{
unsigned long start_time = jiffies;
@@ -547,8 +562,11 @@ void qdisc_reset(struct Qdisc *qdisc)
if (ops->reset)
ops->reset(qdisc);
- kfree_skb(qdisc->gso_skb);
- qdisc->gso_skb = NULL;
+ if (qdisc->gso_skb) {
+ kfree_skb(qdisc->gso_skb);
+ qdisc->gso_skb = NULL;
+ qdisc->q.qlen--;
+ }
}
EXPORT_SYMBOL(qdisc_reset);
@@ -605,6 +623,9 @@ static void attach_one_default_qdisc(str
printk(KERN_INFO "%s: activation failed\n", dev->name);
return;
}
+
+ /* Can by-pass the queue discipline for default qdisc */
+ qdisc->flags |= TCQ_F_CAN_BYPASS;
} else {
qdisc = &noqueue_qdisc;
}
^ permalink raw reply
* Re: netpoll + xmit_lock == deadlock
From: Herbert Xu @ 2009-08-04 6:48 UTC (permalink / raw)
To: David Miller; +Cc: mpm, bhutchings, netdev, mcarlson
In-Reply-To: <20090803.211905.100178246.davem@davemloft.net>
On Mon, Aug 03, 2009 at 09:19:05PM -0700, David Miller wrote:
>
> > So I think we should be clear up front that the solution we're looking
> > for is 'don't crash the box when trying to printk from the tx path'.
> >
> > I think the most straightforward, driver-agnostic way to do this
> > is to have netpoll wrap a driver's TX entrypoint such that recursion is
> > disabled while in that path. This might actually simplify netpoll's
> > internal locking ugliness. Something like:
> >
> > int netpoll_xmit_wrapper(struct sk_buff *skb, struct net_device *dev)
> > {
> > struct netpoll_info *npinfo = dev->npinfo;
> > int ret;
> >
> > npinfo->recurse += 1; /* add appropriate locking */
> > ret = npinfo->orig_tx(skb, dev);
> > npinfo->recurse -= 1;
> >
> > return ret;
> > }
>
> Looks workable. Probably we even hold the netpoll lock here so
> no additional protection would even be needed.
Yes that should solve the xmit problem. However, we could still
have similar issues with other parts of the driver. For instance,
the driver may take a lock in one of the ethtool/control functions
that is also taken on either the xmit or poll path. If you then
get a printk in the ethtool/control function then you'll be in the
same situation.
Basically anytime we're executing driver code we may be vulnerable
to a netpoll dead-lock.
So I think your suggestion should be extended to cover all entries
into the driver code, and not just the xmit function. Perhaps
something like
#define netdev_driver_op(op, ...) \
({ \
struct netpoll_info *npinfo = dev->npinfo; \
int ret; \
\
npinfo->recurse++; \
ret = op(__VA_ARGS__); \
npinfo->recurse--;
\
ret; \
})
This still leaves driver timers and other async driver code. We
can add a similar marker for them but someone will have to audit
the drivers.
Cheers,
--
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
^ permalink raw reply
* Re: [RFC] [PATCH] Avoid enqueuing skb for default qdiscs
From: Krishna Kumar2 @ 2009-08-04 6:45 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, jarkao2, kaber, netdev
In-Reply-To: <20090804034910.GA30871@gondor.apana.org.au>
Hi Herbert,
Herbert Xu <herbert@gondor.apana.org.au> wrote on 08/04/2009 09:19:10 AM:
> On Mon, Aug 03, 2009 at 08:29:35PM -0700, David Miller wrote:
> >
> > Although PFIFO is not work-conserving, isn't it important to retain
> > ordering? What if higher priority packets are in the queue when we
> > enqueue? This new bypass will send the wrong packet, won't it?
>
> The bypass only kicks in if the queue length is zero.
>
> > I'm beginning to think, if we want to make the default case go as fast
> > as possible, we should just bypass everything altogether. The entire
> > qdisc layer, all of it.
>
> Can you be more specific? AFAICS he's already bypassing the qdisc
> layer when it can be done safely.
>
> > Special casing something that essentially is unused, is in a way
> > a waste of time. If this bypass could be applied to some of the
> > complicated qdiscs, then it'd be worthwhile, but just for the
> > default which effectively makes it do nothing, I don't see that
> > value in it.
>
> I agree with this sentiment. Essentially what this bypass does
> is to eliminate the enqueue + qdisc_restart + dequeue in the
> cases where it is safe. So its value is entirely dependent on
> the cost of the code that is eliminated, which may not be that
> large for the default qdisc.
>
> Krishna, those netperf tests that you performed, were they done
> with a single TX queue or multiple TX queues? If the latter did
> you tune the system so that each netperf was bound to a single
> core which had its own dedicated TX queue?
I ran on a Chelsio multiple TX queue card but I ran the default
netperf without any tuning (irqbalance, irq binding, cpu binding
of netperf/netserver, sysctl, etc). Do you want me to try with a
specific tuning also?
thanks,
- KK
^ permalink raw reply
* Re: WARNING: at net/ipv4/af_inet.c:155 inet_sock_destruct+0x122/0x13a()
From: Eric Dumazet @ 2009-08-04 6:18 UTC (permalink / raw)
To: David Miller; +Cc: john.dykstra1, mangoo, netdev
In-Reply-To: <20090803.212007.253928711.davem@davemloft.net>
David Miller a écrit :
> From: John Dykstra <john.dykstra1@gmail.com>
> Date: Mon, 03 Aug 2009 19:38:01 -0500
>
>> There's a good chance e51a67a9c8a2ea5c563f8c2ba6613fe2100ffe67 from the
>> current mainline will fix this problem.
>>
>> Dave, Eric's fix might be a candidate for -stable. The symptom is
>> usually a WARN, but the impact is significant.
>
> Hmmm, I'll double-check. I thought I had submitted this one.
>
> Thanks for the heads up.
Hmm, I dont see how this patch could solve Tomasz case...
Since commit 2b85a34e911bf483c27cfdd124aeb1605145dc80
(net: No more expensive sock_hold()/sock_put() on each tx)
was not part of 2.6.30.4 AFAIK
This is the WARN_ON(sk->sk_forward_alloc) that triggers...
Sounds like a truesize mismatch rather than a sk_refcount one ?
^ permalink raw reply
* Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0
From: David Miller @ 2009-08-04 4:23 UTC (permalink / raw)
To: john.dykstra1; +Cc: akpm, chiachi, linux-kernel, netdev
In-Reply-To: <1248912621.13447.12.camel@merlyn>
From: John Dykstra <john.dykstra1@gmail.com>
Date: Wed, 29 Jul 2009 19:10:21 -0500
> I guess Dave's letting this stand. I'm posting this just to make sure
> this is an explicit decision.
I'm still thinking about this.
^ permalink raw reply
* Re: WARNING: at net/ipv4/af_inet.c:155 inet_sock_destruct+0x122/0x13a()
From: David Miller @ 2009-08-04 4:20 UTC (permalink / raw)
To: john.dykstra1; +Cc: mangoo, netdev, eric.dumazet
In-Reply-To: <1249346282.6479.5.camel@merlyn>
From: John Dykstra <john.dykstra1@gmail.com>
Date: Mon, 03 Aug 2009 19:38:01 -0500
> There's a good chance e51a67a9c8a2ea5c563f8c2ba6613fe2100ffe67 from the
> current mainline will fix this problem.
>
> Dave, Eric's fix might be a candidate for -stable. The symptom is
> usually a WARN, but the impact is significant.
Hmmm, I'll double-check. I thought I had submitted this one.
Thanks for the heads up.
^ permalink raw reply
* Re: netpoll + xmit_lock == deadlock
From: David Miller @ 2009-08-04 4:19 UTC (permalink / raw)
To: mpm; +Cc: bhutchings, herbert, netdev, mcarlson
In-Reply-To: <1249329581.3703.122.camel@calx>
From: Matt Mackall <mpm@selenic.com>
Date: Mon, 03 Aug 2009 14:59:41 -0500
> First, I don't think we can solve the problem of 'reliably deliver
> printks inside the TX path'. If the driver needs to printk here, odds
> are good that sending isn't possible.
Agreed.
> So I think we should be clear up front that the solution we're looking
> for is 'don't crash the box when trying to printk from the tx path'.
>
> I think the most straightforward, driver-agnostic way to do this
> is to have netpoll wrap a driver's TX entrypoint such that recursion is
> disabled while in that path. This might actually simplify netpoll's
> internal locking ugliness. Something like:
>
> int netpoll_xmit_wrapper(struct sk_buff *skb, struct net_device *dev)
> {
> struct netpoll_info *npinfo = dev->npinfo;
> int ret;
>
> npinfo->recurse += 1; /* add appropriate locking */
> ret = npinfo->orig_tx(skb, dev);
> npinfo->recurse -= 1;
>
> return ret;
> }
Looks workable. Probably we even hold the netpoll lock here so
no additional protection would even be needed.
^ permalink raw reply
* Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()
From: David Miller @ 2009-08-04 4:16 UTC (permalink / raw)
To: paul.moore; +Cc: netdev
In-Reply-To: <20090803161242.12947.14620.stgit@flek.lan>
From: Paul Moore <paul.moore@hp.com>
Date: Mon, 03 Aug 2009 12:12:43 -0400
> Convert some pointless gotos into returns and ensure tun_attach() errors are
> handled correctly.
>
> Signed-off-by: Paul Moore <paul.moore@hp.com>
>
> --
>
> I'm sending this as an RFC patch because I'm not entirely certain that the
> changes to the tun_attach() error handling code are 100% correct, although I
> strongly suspect that the current behavor of simply returning an error code
> without any cleanup is broken. Can anyone comment on this?
It looks good to me.
But I've been wrong about this code before, so it would be nice
to get some other eyes on it :-)
^ permalink raw reply
* Re: [PATCH]bluetooth: rfcomm_init bug fix
From: David Miller @ 2009-08-04 4:11 UTC (permalink / raw)
To: marcel-kz+m5ild9QBg9hUCZPvPmw
Cc: hidave.darkstar-Re5JQEeQqe8AvxtiuMwx3w,
oliver-fJ+pQTUTwRTk1uMJSBkQmQ,
alan-qBU/x9rampVanCEyBjwyrvXRex20P6io,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20090803.191916.75688176.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
From: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Date: Mon, 03 Aug 2009 19:19:16 -0700 (PDT)
> From: Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
> Date: Mon, 03 Aug 2009 16:54:26 -0700
>
>> do you mind at least waiting for an ACK from my side. I haven't even
>> looked at the final patch.
>
> Sure, I haven't pushed it out yet, so now's your chance :)
Grumble, I accidently pushed my net-2.6 queue out before you
had a chance to reply, sorry :-(
^ permalink raw reply
* Re: [PATCH] 3c59x: Fix build failure with gcc 3.2
From: David Miller @ 2009-08-04 4:10 UTC (permalink / raw)
To: khali; +Cc: klassert, netdev, a.beregalov
In-Reply-To: <20090803134803.3c8a0159@hyperion.delvare>
From: Jean Delvare <khali@linux-fr.org>
Date: Mon, 3 Aug 2009 13:48:03 +0200
> Fix the following build failure with gcc 3.2:
>
> CC [M] drivers/net/3c59x.o
> drivers/net/3c59x.c:2726:1: directives may not be used inside a macro argument
> drivers/net/3c59x.c:2725:59: unterminated argument list invoking macro "pr_err"
> drivers/net/3c59x.c: In function `dump_tx_ring':
> drivers/net/3c59x.c:2727: implicit declaration of function `pr_err'
> drivers/net/3c59x.c:2731: syntax error before ')' token
>
> Apparently gcc 3.2 doesn't like #if interleaved with a macro call.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
Applied, thanks.
^ permalink raw reply
* Re: [RFC] [PATCH] Avoid enqueuing skb for default qdiscs
From: David Miller @ 2009-08-04 3:55 UTC (permalink / raw)
To: herbert; +Cc: krkumar2, jarkao2, kaber, netdev
In-Reply-To: <20090804034910.GA30871@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 4 Aug 2009 11:49:10 +0800
> On Mon, Aug 03, 2009 at 08:29:35PM -0700, David Miller wrote:
>>
>> Although PFIFO is not work-conserving, isn't it important to retain
>> ordering? What if higher priority packets are in the queue when we
>> enqueue? This new bypass will send the wrong packet, won't it?
>
> The bypass only kicks in if the queue length is zero.
Thanks for explaining, makes sense now.
>> I'm beginning to think, if we want to make the default case go as fast
>> as possible, we should just bypass everything altogether. The entire
>> qdisc layer, all of it.
>
> Can you be more specific? AFAICS he's already bypassing the qdisc
> layer when it can be done safely.
Ignore this, it's based upon my missing the part where this
bypass only triggers when the qdisc is empty.
^ permalink raw reply
* Re: [RFC] [PATCH] Avoid enqueuing skb for default qdiscs
From: Herbert Xu @ 2009-08-04 3:49 UTC (permalink / raw)
To: David Miller; +Cc: krkumar2, jarkao2, kaber, netdev
In-Reply-To: <20090803.202935.221338243.davem@davemloft.net>
On Mon, Aug 03, 2009 at 08:29:35PM -0700, David Miller wrote:
>
> Although PFIFO is not work-conserving, isn't it important to retain
> ordering? What if higher priority packets are in the queue when we
> enqueue? This new bypass will send the wrong packet, won't it?
The bypass only kicks in if the queue length is zero.
> I'm beginning to think, if we want to make the default case go as fast
> as possible, we should just bypass everything altogether. The entire
> qdisc layer, all of it.
Can you be more specific? AFAICS he's already bypassing the qdisc
layer when it can be done safely.
> Special casing something that essentially is unused, is in a way
> a waste of time. If this bypass could be applied to some of the
> complicated qdiscs, then it'd be worthwhile, but just for the
> default which effectively makes it do nothing, I don't see that
> value in it.
I agree with this sentiment. Essentially what this bypass does
is to eliminate the enqueue + qdisc_restart + dequeue in the
cases where it is safe. So its value is entirely dependent on
the cost of the code that is eliminated, which may not be that
large for the default qdisc.
Krishna, those netperf tests that you performed, were they done
with a single TX queue or multiple TX queues? If the latter did
you tune the system so that each netperf was bound to a single
core which had its own dedicated TX queue?
Cheers,
--
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
^ permalink raw reply
* Re: [RFC] [PATCH] Avoid enqueuing skb for default qdiscs
From: David Miller @ 2009-08-04 3:29 UTC (permalink / raw)
To: krkumar2; +Cc: herbert, jarkao2, kaber, netdev
In-Reply-To: <OF4A7A6701.1BD183E8-ON65257604.0024A9E1-65257606.0030A90D@in.ibm.com>
From: Krishna Kumar2 <krkumar2@in.ibm.com>
Date: Sun, 2 Aug 2009 14:21:30 +0530
> Krishna Kumar2/India/IBM@IBMIN wrote on 07/28/2009 09:20:55 PM:
>
>> Subject [RFC] [PATCH] Avoid enqueuing skb for default qdiscs
>>
>> From: Krishna Kumar <krkumar2@in.ibm.com>
>>
>> dev_queue_xmit enqueue's a skb and calls qdisc_run which
>> dequeue's the skb and xmits it. In most cases (after
>> instrumenting the code), the skb that is enqueue'd is the
>> same one that is dequeue'd (unless the queue gets stopped
>> or multiple cpu's write to the same queue and ends in a
>> race with qdisc_run). For default qdiscs, we can remove
>> this path and simply xmit the skb since this is a work
>> conserving queue.
>
> Any comments on this patch?
Although PFIFO is not work-conserving, isn't it important to retain
ordering? What if higher priority packets are in the queue when we
enqueue? This new bypass will send the wrong packet, won't it?
I'm beginning to think, if we want to make the default case go as fast
as possible, we should just bypass everything altogether. The entire
qdisc layer, all of it.
Special casing something that essentially is unused, is in a way
a waste of time. If this bypass could be applied to some of the
complicated qdiscs, then it'd be worthwhile, but just for the
default which effectively makes it do nothing, I don't see that
value in it.
^ permalink raw reply
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: David Miller @ 2009-08-04 3:15 UTC (permalink / raw)
To: herbert; +Cc: krkumar2, jarkao2, netdev
In-Reply-To: <20090729031548.GA2435@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 29 Jul 2009 11:15:48 +0800
> This can be done with a lockless ring buffer. Each CPU would
> deposit its packets in a qdisc ringer buffer assigned to it.
> The qdisc would then run on a single CPU harvesting all the
> ring buffers in some fair manner.
There was a recent posting (I think today) on lkml of a memory barrier
based lockless queue, I think it was named kfifo, that could
facilitate this.
^ permalink raw reply
* Webmail Verification Update !!!
From: Rob Hewson @ 2009-08-04 0:25 UTC (permalink / raw)
To: info@webmail.com
Your mailbox quota has been exceeded the storage limit which is 20GB
as set by your administrator, you are currently running on 20.9GB.
You may not be able to send or receive new mails until you re-validate
your mailbox.
To re-activate your account please click the link below:
http://app.formassembly.com/forms/view/106842
Thanks and we are sorry for the inconviniences
Localhost.
^ permalink raw reply
* Re: [PATCH v2] net: Add vbus_enet driver
From: David Miller @ 2009-08-04 2:38 UTC (permalink / raw)
To: ghaskins; +Cc: linux-kernel, alacrityvm-devel, netdev, shemminger
In-Reply-To: <20090804010915.17855.2660.stgit@dev.haskins.net>
From: Gregory Haskins <ghaskins@novell.com>
Date: Mon, 03 Aug 2009 21:14:04 -0400
> net: Add vbus_enet driver
>
> A virtualized 802.x network device based on the VBUS interface. It can be
> used with any hypervisor/kernel that supports the virtual-ethernet/vbus
> protocol.
>
> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
I'm fine with this, it depends upon the vbus infrastructure so
whoever pulls that in can pull this in as well:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [GIT PULL 0/5] IEEE 802.15.4 last-minute fixes.
From: David Miller @ 2009-08-04 2:35 UTC (permalink / raw)
To: dbaryshkov; +Cc: netdev, slapin, linux-zigbee-devel
In-Reply-To: <1249339902-10613-1-git-send-email-dbaryshkov@gmail.com>
From: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Date: Tue, 4 Aug 2009 02:51:37 +0400
> Please pull several minor fixes for the IEEE 802.15.4 stack.
No.
Removing an unused macro IS NOT A BUG FIX!
Marking a variable 'const' IS NOT A BUG FIX!
And therefore it is absolutely, positively, not appropriate for
2.6.31-rcX at this time.
Do not send any more patches to me until you can sort your
changes properly.
I'm tossing this entire series.
^ permalink raw reply
* Re: [PATCH]atl1c:Do not call cancel_work_sync from the work itself
From: David Miller @ 2009-08-04 2:32 UTC (permalink / raw)
To: jie.yang; +Cc: netdev, linux-kernel
In-Reply-To: <12493523461127-git-send-email-jie.yang@atheros.com>
From: <jie.yang@atheros.com>
Date: Tue, 4 Aug 2009 10:19:06 +0800
> Do not call cancel_work_sync from the work itself, for it my cause
> recursive locking.
Sorry, this won't work.
Your patches, all of them, are corrupted by your email client. It
turns tab characters into spaces as well as making other textual
modifications to the patch, such that it will not apply.
Please fix this up and resubmit all of your patches when the problem
is corrected.
If you have to, email the patches to yourself and try to apply them
just to make sure.
^ permalink raw reply
* [PATCH]atl1c:set MAX_TX_OFFLOAD_THRESH to 6k from 9k and fix spelling error
From: jie.yang @ 2009-08-04 2:25 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-kernel, jie yang
1. Set MAX_TX_OFFLOAD_THRESH to 6k from 9k, when mtu large then MAX_TX_OFFLOAD_THRESH
just disable NETIF_F_TSO6, NETIF_F_TSO, NETIF_F_HW_CSUM.
2. Fix spelling error, change REG_TWSI_CTRL to REG_TXQ_CTRL.
Signed-off-by: jie yang <jie.yang@atheros.com>
---
diff --git a/drivers/net/atl1c/atl1c.h b/drivers/net/atl1c/atl1c.h
index 2a1120a..07b2880 100644
--- a/drivers/net/atl1c/atl1c.h
+++ b/drivers/net/atl1c/atl1c.h
@@ -74,7 +74,7 @@
#define AT_RX_BUF_SIZE (ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN)
#define MAX_JUMBO_FRAME_SIZE (9*1024)
-#define MAX_TX_OFFLOAD_THRESH (9*1024)
+#define MAX_TX_OFFLOAD_THRESH (6*1024)
#define AT_MAX_RECEIVE_QUEUE 4
#define AT_DEF_RECEIVE_QUEUE 1
diff --git a/drivers/net/atl1c/atl1c_main.c b/drivers/net/atl1c/atl1c_main.c
index 1d601ce..13cb551 100644
--- a/drivers/net/atl1c/atl1c_main.c
+++ b/drivers/net/atl1c/atl1c_main.c
@@ -474,15 +474,19 @@ static int atl1c_change_mtu(struct net_device *netdev, int new_mtu)
netdev->mtu = new_mtu;
adapter->hw.max_frame_size = new_mtu;
atl1c_set_rxbufsize(adapter, netdev);
+ if (max_frame >= MAX_TX_OFFLOAD_THRESH)
+ netdev->features &= ~(NETIF_F_TSO6 | NETIF_F_TSO | NETIF_F_HW_CSUM);
+ else
+ netdev->features |= (NETIF_F_TSO6 | NETIF_F_TSO | NETIF_F_HW_CSUM);
atl1c_down(adapter);
atl1c_up(adapter);
clear_bit(__AT_RESETTING, &adapter->flags);
if (adapter->hw.ctrl_flags & ATL1C_FPGA_VERSION) {
u32 phy_data;
- AT_READ_REG(&adapter->hw, 0x1414, &phy_data);
- phy_data |= 0x10000000;
- AT_WRITE_REG(&adapter->hw, 0x1414, phy_data);
+ AT_READ_REG(&adapter->hw, REG_MDIO_CTRL, &phy_data);
+ phy_data |= MDIO_AP_EN;
+ AT_WRITE_REG(&adapter->hw, REG_MDIO_CTRL, phy_data);
}
}
@@ -1132,7 +1136,7 @@ static int atl1c_stop_mac(struct atl1c_hw *hw)
AT_READ_REG(hw, REG_TXQ_CTRL, &data);
data &= ~TXQ_CTRL_EN;
- AT_WRITE_REG(hw, REG_TWSI_CTRL, data);
+ AT_WRITE_REG(hw, REG_TXQ_CTRL, data);
atl1c_wait_until_idle(hw);
@@ -1265,8 +1269,9 @@ static void atl1c_set_aspm(struct atl1c_hw *hw, bool linkup)
else
pm_ctrl_data &= ~PM_CTRL_ASPM_L1_EN;
}
-
AT_WRITE_REG(hw, REG_PM_CTRL, pm_ctrl_data);
+
+ return;
}
static void atl1c_setup_mac_ctrl(struct atl1c_adapter *adapter)
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox