| Message ID | 1222302922-13200-1-git-send-email-kexin.hao@windriver.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jeff Garzik |
| Headers | show |
Kevin Hao wrote: > When NETIF_F_LLTX is set, the atlx driver will use a private lock. > But in recent kernels this implementation seems redundant and > can cause problems where AF_PACKET sees things twice. Since > NETIF_F_LLTX is marked as deprecated and shouldn't be used in > new driver, this patch removes NETIF_F_LLTX and adds a mmiowb > before sending packet. > > Signed-off-by: Kevin Hao <kexin.hao@windriver.com> Can you explain a bit more concretely the problem this solves, and your testing? Ultimately we'll want to merge this code with the atl1 code, so we need to be confident that we can and should make the same change there. -- Chris > --- > drivers/net/atlx/atl2.c | 24 +----------------------- > drivers/net/atlx/atl2.h | 1 - > 2 files changed, 1 insertions(+), 24 deletions(-) > > diff --git a/drivers/net/atlx/atl2.c b/drivers/net/atlx/atl2.c > index b2995ac..3df4ee1 100644 > --- a/drivers/net/atlx/atl2.c > +++ b/drivers/net/atlx/atl2.c > @@ -116,7 +116,6 @@ static int __devinit atl2_sw_init(struct atl2_adapter *adapter) > hw->max_frame_size = adapter->netdev->mtu; > > spin_lock_init(&adapter->stats_lock); > - spin_lock_init(&adapter->tx_lock); > > set_bit(__ATL2_DOWN, &adapter->flags); > > @@ -749,11 +748,7 @@ static void atl2_down(struct atl2_adapter *adapter) > * reschedule our watchdog timer */ > set_bit(__ATL2_DOWN, &adapter->flags); > > -#ifdef NETIF_F_LLTX > - netif_stop_queue(netdev); > -#else > netif_tx_disable(netdev); > -#endif > > /* reset MAC to disable all RX/TX */ > atl2_reset_hw(&adapter->hw); > @@ -829,7 +824,6 @@ static inline int TxdFreeBytes(struct atl2_adapter *adapter) > static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev) > { > struct atl2_adapter *adapter = netdev_priv(netdev); > - unsigned long flags; > struct tx_pkt_header *txph; > u32 offset, copy_len; > int txs_unused; > @@ -845,16 +839,6 @@ static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev) > return NETDEV_TX_OK; > } > > -#ifdef NETIF_F_LLTX > - local_irq_save(flags); > - if (!spin_trylock(&adapter->tx_lock)) { > - /* Collision - tell upper layer to requeue */ > - local_irq_restore(flags); > - return NETDEV_TX_LOCKED; > - } > -#else > - spin_lock_irqsave(&adapter->tx_lock, flags); > -#endif > txs_unused = TxsFreeUnit(adapter); > txbuf_unused = TxdFreeBytes(adapter); > > @@ -862,7 +846,6 @@ static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev) > txs_unused < 1) { > /* not enough resources */ > netif_stop_queue(netdev); > - spin_unlock_irqrestore(&adapter->tx_lock, flags); > return NETDEV_TX_BUSY; > } > > @@ -908,8 +891,7 @@ static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev) > ATL2_WRITE_REGW(&adapter->hw, REG_MB_TXD_WR_IDX, > (adapter->txd_write_ptr >> 2)); > > - spin_unlock_irqrestore(&adapter->tx_lock, flags); > - > + mmiowb(); > netdev->trans_start = jiffies; > dev_kfree_skb_any(skb); > return NETDEV_TX_OK; > @@ -1447,10 +1429,6 @@ static int __devinit atl2_probe(struct pci_dev *pdev, > netdev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX); > #endif > > -#ifdef NETIF_F_LLTX > - netdev->features |= NETIF_F_LLTX; > -#endif > - > /* Init PHY as early as possible due to power saving issue */ > atl2_phy_init(&adapter->hw); > > diff --git a/drivers/net/atlx/atl2.h b/drivers/net/atlx/atl2.h > index 6e1f28f..09974df 100644 > --- a/drivers/net/atlx/atl2.h > +++ b/drivers/net/atlx/atl2.h > @@ -462,7 +462,6 @@ struct atl2_adapter { > u16 link_duplex; > > spinlock_t stats_lock; > - spinlock_t tx_lock; > > struct work_struct reset_task; > struct work_struct link_chg_task; -- 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
Chris Snook wrote: > Kevin Hao wrote: >> When NETIF_F_LLTX is set, the atlx driver will use a private lock. >> But in recent kernels this implementation seems redundant and >> can cause problems where AF_PACKET sees things twice. Since >> NETIF_F_LLTX is marked as deprecated and shouldn't be used in >> new driver, this patch removes NETIF_F_LLTX and adds a mmiowb >> before sending packet. >> >> Signed-off-by: Kevin Hao <kexin.hao@windriver.com> > > Can you explain a bit more concretely the problem this solves, and your > testing? Ultimately we'll want to merge this code with the atl1 code, > so we need to be confident that we can and should make the same change > there. > > -- Chris > >> --- >> drivers/net/atlx/atl2.c | 24 +----------------------- >> drivers/net/atlx/atl2.h | 1 - >> 2 files changed, 1 insertions(+), 24 deletions(-) Not directly addressing your question, but LLTX is indeed deprecated and in general we want to move away from it. Jeff -- 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
Jeff Garzik wrote: > Chris Snook wrote: >> Kevin Hao wrote: >>> When NETIF_F_LLTX is set, the atlx driver will use a private lock. >>> But in recent kernels this implementation seems redundant and >>> can cause problems where AF_PACKET sees things twice. Since >>> NETIF_F_LLTX is marked as deprecated and shouldn't be used in >>> new driver, this patch removes NETIF_F_LLTX and adds a mmiowb >>> before sending packet. >>> >>> Signed-off-by: Kevin Hao <kexin.hao@windriver.com> >> >> Can you explain a bit more concretely the problem this solves, and >> your testing? Ultimately we'll want to merge this code with the atl1 >> code, so we need to be confident that we can and should make the same >> change there. >> >> -- Chris >> >>> --- >>> drivers/net/atlx/atl2.c | 24 +----------------------- >>> drivers/net/atlx/atl2.h | 1 - >>> 2 files changed, 1 insertions(+), 24 deletions(-) > > Not directly addressing your question, but LLTX is indeed deprecated and > in general we want to move away from it. > > Jeff I'm all in favor of removing legacy cruft. I want to know a little more about it so we can remove it from the atl1 code as well. Ultimately most of this will be shared code. -- Chris -- 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
On Thu, 25 Sep 2008 18:34:29 -0400 Chris Snook <csnook@redhat.com> wrote: > > I'm all in favor of removing legacy cruft. I want to know a little > more about it so we can remove it from the atl1 code as well. > Ultimately most of this will be shared code. Yes, LLTX can (and should) be removed from the atl1 code. I had an exchange with Herbert Xu and David Miller on LKML a few weeks ago, and they gave me a few tips on how to do it, but I haven't yet had the time. What takes an experienced netdev warrior about a half hour to implement unfortunately takes me days of after-dayjob time as I track through all the calls that that are affected and test the modified driver. Now back to atl2... I'd like to hear from Kevin also whether he tested the LLTX removal. I'm building the driver with his patch as we speak, and I'll test it myself this weekend, but Kevin I'd still like to know if you tested the patch. Thanks, Jay -- 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
On Thu, 2008-09-25 at 18:58 -0500, Jay Cliburn wrote: > On Thu, 25 Sep 2008 18:34:29 -0400 > Chris Snook <csnook@redhat.com> wrote: > > > > > I'm all in favor of removing legacy cruft. I want to know a little > > more about it so we can remove it from the atl1 code as well. > > Ultimately most of this will be shared code. > > Yes, LLTX can (and should) be removed from the atl1 code. I had an > exchange with Herbert Xu and David Miller on LKML a few weeks ago, and > they gave me a few tips on how to do it, but I haven't yet had the > time. What takes an experienced netdev warrior about a half hour to > implement unfortunately takes me days of after-dayjob time as I track > through all the calls that that are affected and test the modified > driver. > > Now back to atl2... I'd like to hear from Kevin also whether he tested > the LLTX removal. I'm building the driver with his patch as we speak, > and I'll test it myself this weekend, but Kevin I'd still like to know > if you tested the patch. Yes, I have tested this driver on a Eee PC. It works well. Thanks, Kevin > > Thanks, > Jay -- 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
Kevin Hao wrote: > On Thu, 2008-09-25 at 18:58 -0500, Jay Cliburn wrote: >> On Thu, 25 Sep 2008 18:34:29 -0400 >> Chris Snook <csnook@redhat.com> wrote: >> >>> I'm all in favor of removing legacy cruft. I want to know a little >>> more about it so we can remove it from the atl1 code as well. >>> Ultimately most of this will be shared code. >> Yes, LLTX can (and should) be removed from the atl1 code. I had an >> exchange with Herbert Xu and David Miller on LKML a few weeks ago, and >> they gave me a few tips on how to do it, but I haven't yet had the >> time. What takes an experienced netdev warrior about a half hour to >> implement unfortunately takes me days of after-dayjob time as I track >> through all the calls that that are affected and test the modified >> driver. >> >> Now back to atl2... I'd like to hear from Kevin also whether he tested >> the LLTX removal. I'm building the driver with his patch as we speak, >> and I'll test it myself this weekend, but Kevin I'd still like to know >> if you tested the patch. > > Yes, I have tested this driver on a Eee PC. It works well. Thanks, please mention this with your patch submissions. -- Chris -- 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
On Thu, 25 Sep 2008 08:35:22 +0800 Kevin Hao <kexin.hao@windriver.com> wrote: > When NETIF_F_LLTX is set, the atlx driver will use a private lock. > But in recent kernels this implementation seems redundant and > can cause problems where AF_PACKET sees things twice. Since > NETIF_F_LLTX is marked as deprecated and shouldn't be used in > new driver, this patch removes NETIF_F_LLTX and adds a mmiowb > before sending packet. > > Signed-off-by: Kevin Hao <kexin.hao@windriver.com> Seems to work fine with large file (3.5 GiB) two-way scp and two-way iperf runs. [root@osprey ~]# ifconfig eth1 eth1 Link encap:Ethernet HWaddr 00:13:74:00:5C:38 inet addr:192.168.1.33 Bcast:192.168.1.255 Mask:255.255.255.0 inet6 addr: fe80::213:74ff:fe00:5c38/64 Scope:Link UP BROADCAST RUNNING MULTICAST MTU:1492 Metric:1 RX packets:10054352 errors:0 dropped:0 overruns:0 frame:1 TX packets:12364268 errors:0 dropped:0 overruns:0 carrier:6 collisions:0 txqueuelen:1000 RX bytes:11990462653 (11.1 GiB) TX bytes:12351847745 (11.5 GiB) Memory:dbfc0000-dc000000 Acked-by: Jay Cliburn <jacliburn@bellsouth.net> > --- > drivers/net/atlx/atl2.c | 24 +----------------------- > drivers/net/atlx/atl2.h | 1 - > 2 files changed, 1 insertions(+), 24 deletions(-) > > diff --git a/drivers/net/atlx/atl2.c b/drivers/net/atlx/atl2.c > index b2995ac..3df4ee1 100644 > --- a/drivers/net/atlx/atl2.c > +++ b/drivers/net/atlx/atl2.c > @@ -116,7 +116,6 @@ static int __devinit atl2_sw_init(struct > atl2_adapter *adapter) hw->max_frame_size = adapter->netdev->mtu; > > spin_lock_init(&adapter->stats_lock); > - spin_lock_init(&adapter->tx_lock); > > set_bit(__ATL2_DOWN, &adapter->flags); > > @@ -749,11 +748,7 @@ static void atl2_down(struct atl2_adapter > *adapter) > * reschedule our watchdog timer */ > set_bit(__ATL2_DOWN, &adapter->flags); > > -#ifdef NETIF_F_LLTX > - netif_stop_queue(netdev); > -#else > netif_tx_disable(netdev); > -#endif > > /* reset MAC to disable all RX/TX */ > atl2_reset_hw(&adapter->hw); > @@ -829,7 +824,6 @@ static inline int TxdFreeBytes(struct > atl2_adapter *adapter) static int atl2_xmit_frame(struct sk_buff > *skb, struct net_device *netdev) { > struct atl2_adapter *adapter = netdev_priv(netdev); > - unsigned long flags; > struct tx_pkt_header *txph; > u32 offset, copy_len; > int txs_unused; > @@ -845,16 +839,6 @@ static int atl2_xmit_frame(struct sk_buff *skb, > struct net_device *netdev) return NETDEV_TX_OK; > } > > -#ifdef NETIF_F_LLTX > - local_irq_save(flags); > - if (!spin_trylock(&adapter->tx_lock)) { > - /* Collision - tell upper layer to requeue */ > - local_irq_restore(flags); > - return NETDEV_TX_LOCKED; > - } > -#else > - spin_lock_irqsave(&adapter->tx_lock, flags); > -#endif > txs_unused = TxsFreeUnit(adapter); > txbuf_unused = TxdFreeBytes(adapter); > > @@ -862,7 +846,6 @@ static int atl2_xmit_frame(struct sk_buff *skb, > struct net_device *netdev) txs_unused < 1) { > /* not enough resources */ > netif_stop_queue(netdev); > - spin_unlock_irqrestore(&adapter->tx_lock, flags); > return NETDEV_TX_BUSY; > } > > @@ -908,8 +891,7 @@ static int atl2_xmit_frame(struct sk_buff *skb, > struct net_device *netdev) ATL2_WRITE_REGW(&adapter->hw, > REG_MB_TXD_WR_IDX, (adapter->txd_write_ptr >> 2)); > > - spin_unlock_irqrestore(&adapter->tx_lock, flags); > - > + mmiowb(); > netdev->trans_start = jiffies; > dev_kfree_skb_any(skb); > return NETDEV_TX_OK; > @@ -1447,10 +1429,6 @@ static int __devinit atl2_probe(struct pci_dev > *pdev, netdev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX); > #endif > > -#ifdef NETIF_F_LLTX > - netdev->features |= NETIF_F_LLTX; > -#endif > - > /* Init PHY as early as possible due to power saving issue > */ atl2_phy_init(&adapter->hw); > > diff --git a/drivers/net/atlx/atl2.h b/drivers/net/atlx/atl2.h > index 6e1f28f..09974df 100644 > --- a/drivers/net/atlx/atl2.h > +++ b/drivers/net/atlx/atl2.h > @@ -462,7 +462,6 @@ struct atl2_adapter { > u16 link_duplex; > > spinlock_t stats_lock; > - spinlock_t tx_lock; > > struct work_struct reset_task; > struct work_struct link_chg_task; -- 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
diff --git a/drivers/net/atlx/atl2.c b/drivers/net/atlx/atl2.c index b2995ac..3df4ee1 100644 --- a/drivers/net/atlx/atl2.c +++ b/drivers/net/atlx/atl2.c @@ -116,7 +116,6 @@ static int __devinit atl2_sw_init(struct atl2_adapter *adapter) hw->max_frame_size = adapter->netdev->mtu; spin_lock_init(&adapter->stats_lock); - spin_lock_init(&adapter->tx_lock); set_bit(__ATL2_DOWN, &adapter->flags); @@ -749,11 +748,7 @@ static void atl2_down(struct atl2_adapter *adapter) * reschedule our watchdog timer */ set_bit(__ATL2_DOWN, &adapter->flags); -#ifdef NETIF_F_LLTX - netif_stop_queue(netdev); -#else netif_tx_disable(netdev); -#endif /* reset MAC to disable all RX/TX */ atl2_reset_hw(&adapter->hw); @@ -829,7 +824,6 @@ static inline int TxdFreeBytes(struct atl2_adapter *adapter) static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev) { struct atl2_adapter *adapter = netdev_priv(netdev); - unsigned long flags; struct tx_pkt_header *txph; u32 offset, copy_len; int txs_unused; @@ -845,16 +839,6 @@ static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev) return NETDEV_TX_OK; } -#ifdef NETIF_F_LLTX - local_irq_save(flags); - if (!spin_trylock(&adapter->tx_lock)) { - /* Collision - tell upper layer to requeue */ - local_irq_restore(flags); - return NETDEV_TX_LOCKED; - } -#else - spin_lock_irqsave(&adapter->tx_lock, flags); -#endif txs_unused = TxsFreeUnit(adapter); txbuf_unused = TxdFreeBytes(adapter); @@ -862,7 +846,6 @@ static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev) txs_unused < 1) { /* not enough resources */ netif_stop_queue(netdev); - spin_unlock_irqrestore(&adapter->tx_lock, flags); return NETDEV_TX_BUSY; } @@ -908,8 +891,7 @@ static int atl2_xmit_frame(struct sk_buff *skb, struct net_device *netdev) ATL2_WRITE_REGW(&adapter->hw, REG_MB_TXD_WR_IDX, (adapter->txd_write_ptr >> 2)); - spin_unlock_irqrestore(&adapter->tx_lock, flags); - + mmiowb(); netdev->trans_start = jiffies; dev_kfree_skb_any(skb); return NETDEV_TX_OK; @@ -1447,10 +1429,6 @@ static int __devinit atl2_probe(struct pci_dev *pdev, netdev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX); #endif -#ifdef NETIF_F_LLTX - netdev->features |= NETIF_F_LLTX; -#endif - /* Init PHY as early as possible due to power saving issue */ atl2_phy_init(&adapter->hw); diff --git a/drivers/net/atlx/atl2.h b/drivers/net/atlx/atl2.h index 6e1f28f..09974df 100644 --- a/drivers/net/atlx/atl2.h +++ b/drivers/net/atlx/atl2.h @@ -462,7 +462,6 @@ struct atl2_adapter { u16 link_duplex; spinlock_t stats_lock; - spinlock_t tx_lock; struct work_struct reset_task; struct work_struct link_chg_task;
When NETIF_F_LLTX is set, the atlx driver will use a private lock. But in recent kernels this implementation seems redundant and can cause problems where AF_PACKET sees things twice. Since NETIF_F_LLTX is marked as deprecated and shouldn't be used in new driver, this patch removes NETIF_F_LLTX and adds a mmiowb before sending packet. Signed-off-by: Kevin Hao <kexin.hao@windriver.com> --- drivers/net/atlx/atl2.c | 24 +----------------------- drivers/net/atlx/atl2.h | 1 - 2 files changed, 1 insertions(+), 24 deletions(-)