Improvements to octeon_ethernet.
Subsystems (4)
cover Improvements to octeon_ethernet.
Here are a couple of improvements to the octeon_ethernet in drivers/staging/octeon. The first patch is just cleanup, the rest are genuine bug fixes.
I will reply with the four patches.
We may want to merge via Ralf's linux-mips.org tree as Octeon is infact a MIPS based SOC and he has a bunch of other patches queued there that these depend on.
David Daney (4):
Staging: octeon: remove unneeded includes
Staging: Octeon: Run phy bus accesses on a workqueue.
MIPS: Octeon: Do proper acknowledgment of CIU timer interrupts.
Staging: Octeon: Free transmit SKBs in a timely manner.
arch/mips/cavium-octeon/octeon-irq.c | 67 +++++++++++++- drivers/staging/octeon/Kconfig | 1 - drivers/staging/octeon/ethernet-defines.h | 5 +- drivers/staging/octeon/ethernet-mdio.h | 1 - drivers/staging/octeon/ethernet-rgmii.c | 56 +++++++++--- drivers/staging/octeon/ethernet-sgmii.c | 1 - drivers/staging/octeon/ethernet-spi.c | 1 - drivers/staging/octeon/ethernet-tx.c | 137 ++++++++++++++++++++++------ drivers/staging/octeon/ethernet-tx.h | 6 +- drivers/staging/octeon/ethernet-xaui.c | 1 - drivers/staging/octeon/ethernet.c | 141 ++++++++++++++++------------- drivers/staging/octeon/octeon-ethernet.h | 11 ++- 12 files changed, 303 insertions(+), 125 deletions(-)
PATCH 1/4 Staging: octeon: remove unneeded includes
--- drivers/staging/octeon/ethernet-mdio.h | 1 - drivers/staging/octeon/ethernet-rgmii.c | 1 - drivers/staging/octeon/ethernet-sgmii.c | 1 - drivers/staging/octeon/ethernet-spi.c | 1 - drivers/staging/octeon/ethernet-xaui.c | 1 - drivers/staging/octeon/ethernet.c | 1 - 6 files changed, 0 insertions(+), 6 deletions(-)
drivers/staging/octeon/ethernet-mdio.h +0 -1
@@ -32,7 +32,6 @@#include <linux/ip.h> #include <linux/string.h> #include <linux/ethtool.h> -#include <linux/mii.h> #include <linux/seq_file.h> #include <linux/proc_fs.h> #include <net/dst.h>
drivers/staging/octeon/ethernet-rgmii.c +0 -1
@@ -26,7 +26,6 @@**********************************************************************/ #include <linux/kernel.h> #include <linux/netdevice.h> -#include <linux/mii.h> #include <net/dst.h> #include <asm/octeon/octeon.h>
drivers/staging/octeon/ethernet-sgmii.c +0 -1
@@ -26,7 +26,6 @@**********************************************************************/ #include <linux/kernel.h> #include <linux/netdevice.h> -#include <linux/mii.h> #include <net/dst.h> #include <asm/octeon/octeon.h>
drivers/staging/octeon/ethernet-spi.c +0 -1
@@ -26,7 +26,6 @@**********************************************************************/ #include <linux/kernel.h> #include <linux/netdevice.h> -#include <linux/mii.h> #include <net/dst.h> #include <asm/octeon/octeon.h>
drivers/staging/octeon/ethernet-xaui.c +0 -1
@@ -26,7 +26,6 @@**********************************************************************/ #include <linux/kernel.h> #include <linux/netdevice.h> -#include <linux/mii.h> #include <net/dst.h> #include <asm/octeon/octeon.h>
drivers/staging/octeon/ethernet.c +0 -1
@@ -29,7 +29,6 @@#include <linux/module.h> #include <linux/netdevice.h> #include <linux/etherdevice.h> -#include <linux/delay.h> #include <linux/phy.h> #include <net/dst.h>
Signature · 1 line
1.6.6
PATCH 2/4 Staging: Octeon: Run phy bus accesses on a workqueue.
In reply to David DaneyWhen directly accessing a phy, we must acquire the mdio bus lock. To do that we cannot be in interrupt context, so we need to move these operations to a workqueue.
--- drivers/staging/octeon/ethernet-rgmii.c | 55 +++++++++++---- drivers/staging/octeon/ethernet.c | 113 +++++++++++++++++------------- drivers/staging/octeon/octeon-ethernet.h | 4 + 3 files changed, 109 insertions(+), 63 deletions(-)
drivers/staging/octeon/ethernet-rgmii.c +42 -13
@@ -26,6 +26,7 @@**********************************************************************/ #include <linux/kernel.h> #include <linux/netdevice.h> +#include <linux/phy.h> #include <net/dst.h> #include <asm/octeon/octeon.h>
@@ -47,14 +48,20 @@ static int number_rgmii_ports;static void cvm_oct_rgmii_poll (struct net_device *dev) {struct octeon_ethernet *priv =netdev_priv (dev); -unsigned long flags; +unsigned long flags =0 ;cvmx_helper_link_info_t link_info; +int use_global_register_lock = (priv->phydev ==NULL ); -/* -* Take the global register lock since we are going to touch -* registers that affect more than one port. -*/ -spin_lock_irqsave (&global_register_lock, flags); +BUG_ON (in_interrupt ()); +if (use_global_register_lock) { +/* +* Take the global register lock since we are going to +* touch registers that affect more than one port. +*/ +spin_lock_irqsave (&global_register_lock, flags); + }else { +mutex_lock (&priv->phydev->bus->mdio_lock); + } link_info =cvmx_helper_link_get (priv->port);if (link_info.u64 == priv->link_info) {
@@ -114,7 +121,11 @@ static void cvm_oct_rgmii_poll(struct net_device *dev) dev->name); } } -spin_unlock_irqrestore (&global_register_lock, flags); + +if (use_global_register_lock) +spin_unlock_irqrestore (&global_register_lock, flags); +else +mutex_unlock (&priv->phydev->bus->mdio_lock);return ; }
@@ -150,7 +161,12 @@ static void cvm_oct_rgmii_poll(struct net_device *dev) link_info =cvmx_helper_link_autoconf (priv->port); priv->link_info = link_info.u64; } -spin_unlock_irqrestore (&global_register_lock, flags); + +if (use_global_register_lock) +spin_unlock_irqrestore (&global_register_lock, flags); +else { +mutex_unlock (&priv->phydev->bus->mdio_lock); + }if (priv->phydev ==NULL ) {/* Tell core. */
@@ -212,8 +228,11 @@ static irqreturn_t cvm_oct_rgmii_rml_interrupt(int cpl, void *dev_id)struct net_device *dev = cvm_oct_device[cvmx_helper_get_ipd_port (interface, index)]; -if (dev) -cvm_oct_rgmii_poll (dev); +struct octeon_ethernet *priv =netdev_priv (dev); + +if (dev && !atomic_read (&cvm_oct_poll_queue_stopping)) +queue_work (cvm_oct_poll_queue, &priv->port_work); + gmx_rx_int_reg.u64 =0 ; gmx_rx_int_reg.s.phy_dupx =1 ; gmx_rx_int_reg.s.phy_link =1 ;
@@ -251,8 +270,11 @@ static irqreturn_t cvm_oct_rgmii_rml_interrupt(int cpl, void *dev_id)struct net_device *dev = cvm_oct_device[cvmx_helper_get_ipd_port (interface, index)]; -if (dev) -cvm_oct_rgmii_poll (dev); +struct octeon_ethernet *priv =netdev_priv (dev); + +if (dev && !atomic_read (&cvm_oct_poll_queue_stopping)) +queue_work (cvm_oct_poll_queue, &priv->port_work); + gmx_rx_int_reg.u64 =0 ; gmx_rx_int_reg.s.phy_dupx =1 ; gmx_rx_int_reg.s.phy_link =1 ;
@@ -301,6 +323,12 @@ int cvm_oct_rgmii_stop(struct net_device *dev)return 0 ; } +static void cvm_oct_rgmii_immediate_poll (struct work_struct *work) +{ + struct octeon_ethernet *priv = container_of(work, struct octeon_ethernet, port_work); +cvm_oct_rgmii_poll (cvm_oct_device[priv->port]); +} +int cvm_oct_rgmii_init (struct net_device *dev) {struct octeon_ethernet *priv =netdev_priv (dev);
@@ -308,7 +336,7 @@ int cvm_oct_rgmii_init(struct net_device *dev)cvm_oct_common_init (dev); dev->netdev_ops->ndo_stop (dev); - +INIT_WORK (&priv->port_work, cvm_oct_rgmii_immediate_poll);/* * Due to GMX errata in CN3XXX series chips, it is necessary * to take the link down immediately when the PHY changes
@@ -396,4 +424,5 @@ void cvm_oct_rgmii_uninit(struct net_device *dev) number_rgmii_ports--;if (number_rgmii_ports ==0 )free_irq (OCTEON_IRQ_RML , &number_rgmii_ports); +cancel_work_sync (&priv->port_work); }
drivers/staging/octeon/ethernet.c +63 -50
169 lines not on this page · raw message
drivers/staging/octeon/octeon-ethernet.h +4 -0
@@ -61,6 +61,8 @@ struct octeon_ethernet {void (*poll) (struct net_device *dev);struct hrtimer tx_restart_timer;ktime_t tx_restart_interval; +struct delayed_work tx_clean_work; +struct work_struct port_work;/* may be unused. */ };/**
@@ -97,6 +99,8 @@ extern int pow_send_group;extern int pow_receive_group;extern char pow_send_list[];extern struct net_device *cvm_oct_device[]; +extern struct workqueue_struct *cvm_oct_poll_queue; +extern atomic_t cvm_oct_poll_queue_stopping;extern int max_rx_cpus;extern int rx_napi_weight;
Signature · 1 line
1.6.6
PATCH 4/4 Staging: Octeon: Free transmit SKBs in a timely manner.
In reply to David DaneyIf we wait for the once-per-second cleanup to free transmit SKBs, sockets with small transmit buffer sizes might spend most of their time blocked waiting for the cleanup.
Normally we do a cleanup for each transmitted packet. We add a watchdog type timer so that we also schedule a timeout for 150uS after a packet is transmitted. The watchdog is reset for each transmitted packet, so for high packet rates, it never expires. At these high rates, the cleanups are done for each packet so the extra watchdog initiated cleanups are not needed.
--- drivers/staging/octeon/Kconfig | 1 - drivers/staging/octeon/ethernet-defines.h | 5 +- drivers/staging/octeon/ethernet-tx.c | 137 +++++++++++++++++++++++------ drivers/staging/octeon/ethernet-tx.h | 6 +- drivers/staging/octeon/ethernet.c | 47 +++++----- drivers/staging/octeon/octeon-ethernet.h | 9 +-- 6 files changed, 142 insertions(+), 63 deletions(-)
drivers/staging/octeon/Kconfig +0 -1
@@ -3,7 +3,6 @@ config OCTEON_ETHERNET depends on CPU_CAVIUM_OCTEON select PHYLIB select MDIO_OCTEON - select HIGH_RES_TIMERS help This driver supports the builtin ethernet ports on Cavium Networks' products in the Octeon family. This driver supports the
drivers/staging/octeon/ethernet-defines.h +3 -2
@@ -95,10 +95,11 @@/*#define DONT_WRITEBACK(x) 0 */ /* Maximum number of SKBs to try to free per xmit packet. */ -#define MAX_SKB_TO_FREE 10#define MAX_OUT_QUEUE_DEPTH 1000 -#define FAU_NUM_PACKET_BUFFERS_TO_FREE (CVMX_FAU_REG_END - sizeof(uint32_t)) +#define FAU_TOTAL_TX_TO_CLEAN (CVMX_FAU_REG_END - sizeof(uint32_t)) +#define FAU_NUM_PACKET_BUFFERS_TO_FREE (FAU_TOTAL_TX_TO_CLEAN - sizeof(uint32_t)) +#define TOTAL_NUMBER_OF_PORTS (CVMX_PIP_NUM_INPUT_PORTS+1)
drivers/staging/octeon/ethernet-tx.c +110 -27
222 lines not on this page · raw message
drivers/staging/octeon/ethernet-tx.h +3 -3
@@ -29,6 +29,6 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev);int cvm_oct_xmit_pow (struct sk_buff *skb,struct net_device *dev);int cvm_oct_transmit_qos (struct net_device *dev,void *work_queue_entry,int do_free,int qos); -void cvm_oct_tx_shutdown (struct net_device *dev); -void cvm_oct_free_tx_skbs (struct octeon_ethernet *priv); -enum hrtimer_restart cvm_oct_restart_tx (struct hrtimer *timer); +void cvm_oct_tx_initialize (void ); +void cvm_oct_tx_shutdown (void ); +void cvm_oct_tx_shutdown_dev (struct net_device *dev);
drivers/staging/octeon/ethernet.c +23 -24
122 lines not on this page · raw message
drivers/staging/octeon/octeon-ethernet.h +3 -6
@@ -4,7 +4,7 @@* Contact: [email protected] * This file is part of the OCTEON SDK * -* Copyright (c) 2003-2007 Cavium Networks +* Copyright (c) 2003-2010 Cavium Networks * * This file is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License, Version 2, as
@@ -31,8 +31,6 @@#ifndef OCTEON_ETHERNET_H #define OCTEON_ETHERNET_H -#include <linux/hrtimer.h> -/** * This is the definition of the Ethernet driver's private * driver state stored in netdev_priv(dev).
@@ -59,9 +57,7 @@ struct octeon_ethernet {uint64_t link_info;/* Called periodically to check link status */ void (*poll) (struct net_device *dev); -struct hrtimer tx_restart_timer; -ktime_t tx_restart_interval; -struct delayed_work tx_clean_work; +struct delayed_work port_periodic_work;struct work_struct port_work;/* may be unused. */ };
@@ -101,6 +97,7 @@ extern char pow_send_list[];extern struct net_device *cvm_oct_device[];extern struct workqueue_struct *cvm_oct_poll_queue;extern atomic_t cvm_oct_poll_queue_stopping; +extern u64 cvm_oct_tx_poll_interval;extern int max_rx_cpus;extern int rx_napi_weight;
Signature · 1 line
1.6.6
PATCH 3/4 MIPS: Octeon: Do proper acknowledgment of CIU timer interrupts.
In reply to David Daney--- arch/mips/cavium-octeon/octeon-irq.c | 67 ++++++++++++++++++++++++++++++++-- 1 files changed, 63 insertions(+), 4 deletions(-)
arch/mips/cavium-octeon/octeon-irq.c +63 -4
@@ -51,9 +51,6 @@ static void octeon_irq_core_eoi(unsigned int irq)*/ if (desc->status &IRQ_DISABLED )return ; - -/* There is a race here. We should fix it. */ -/* * We don't need to disable IRQs to make these atomic since * they are already disabled earlier in the low level
@@ -202,6 +199,29 @@ static void octeon_irq_ciu0_ack_v2(unsigned int irq) }/* +* CIU timer type interrupts must be acknoleged by writing a '1' bit +* to their sum0 bit. +*/ +static void octeon_irq_ciu0_timer_ack (unsigned int irq) +{ +int index =cvmx_get_core_num () *2 ; +uint64_t mask =1ull << (irq -OCTEON_IRQ_WORKQ0 ); +cvmx_write_csr (CVMX_CIU_INTX_SUM0 (index), mask); +} + +static void octeon_irq_ciu0_timer_ack_v1 (unsigned int irq) +{ +octeon_irq_ciu0_timer_ack (irq); +octeon_irq_ciu0_ack (irq); +} + +static void octeon_irq_ciu0_timer_ack_v2 (unsigned int irq) +{ +octeon_irq_ciu0_timer_ack (irq); +octeon_irq_ciu0_ack_v2 (irq); +} + +/* * Enable the irq on the current core for chips that have the EN*_W1{S,C} * registers. */
@@ -304,6 +324,28 @@ static struct irq_chip octeon_irq_chip_ciu0 = {#endif }; +static struct irq_chip octeon_irq_chip_ciu0_timer_v2 = { + .name ="CIU0-T" , + .enable = octeon_irq_ciu0_enable_v2, + .disable = octeon_irq_ciu0_disable_all_v2, + .ack = octeon_irq_ciu0_timer_ack_v2, + .eoi = octeon_irq_ciu0_eoi_v2, +#ifdef CONFIG_SMP + .set_affinity = octeon_irq_ciu0_set_affinity_v2, +#endif +}; + +static struct irq_chip octeon_irq_chip_ciu0_timer = { + .name ="CIU0-T" , + .enable = octeon_irq_ciu0_enable, + .disable = octeon_irq_ciu0_disable, + .ack = octeon_irq_ciu0_timer_ack_v1, + .eoi = octeon_irq_ciu0_eoi, +#ifdef CONFIG_SMP + .set_affinity = octeon_irq_ciu0_set_affinity, +#endif +}; +static void octeon_irq_ciu1_ack (unsigned int irq) {
@@ -587,6 +629,7 @@ void __init arch_init_irq(void) {int irq;struct irq_chip *chip0; +struct irq_chip *chip0_timer;struct irq_chip *chip1;#ifdef CONFIG_SMP
@@ -602,9 +645,11 @@ void __init arch_init_irq(void) OCTEON_IS_MODEL(OCTEON_CN56XX_PASS2_X) || OCTEON_IS_MODEL(OCTEON_CN52XX_PASS2_X)) { chip0 = &octeon_irq_chip_ciu0_v2; + chip0_timer = &octeon_irq_chip_ciu0_timer_v2; chip1 = &octeon_irq_chip_ciu1_v2; } else { chip0 = &octeon_irq_chip_ciu0; + chip0_timer = &octeon_irq_chip_ciu0_timer; chip1 = &octeon_irq_chip_ciu1; }
@@ -618,7 +663,21 @@ void __init arch_init_irq(void)/* 24 - 87 CIU_INT_SUM0 */ for (irq =OCTEON_IRQ_WORKQ0 ; irq <=OCTEON_IRQ_BOOTDMA ; irq++) { -set_irq_chip_and_handler (irq, chip0, handle_percpu_irq); +switch (irq) { +case OCTEON_IRQ_GMX_DRP0 : +case OCTEON_IRQ_GMX_DRP1 : +case OCTEON_IRQ_IPD_DRP : +case OCTEON_IRQ_KEY_ZERO : +case OCTEON_IRQ_TIMER0 : +case OCTEON_IRQ_TIMER1 : +case OCTEON_IRQ_TIMER2 : +case OCTEON_IRQ_TIMER3 : +set_irq_chip_and_handler (irq, chip0_timer, handle_percpu_irq); +break ; +default : +set_irq_chip_and_handler (irq, chip0, handle_percpu_irq); +break ; + } }/* 88 - 151 CIU_INT_SUM1 */
Signature · 1 line
1.6.6
Re: [PATCH 4/4] Staging: Octeon: Free transmit SKBs in a timely manner.
In reply to David DaneyQuoted 10 lines · David Daney
If we wait for the once-per-second cleanup to free transmit SKBs, sockets with small transmit buffer sizes might spend most of their time blocked waiting for the cleanup.
Normally we do a cleanup for each transmitted packet. We add a watchdog type timer so that we also schedule a timeout for 150uS after a packet is transmitted. The watchdog is reset for each transmitted packet, so for high packet rates, it never expires. At these high rates, the cleanups are done for each packet so the extra watchdog initiated cleanups are not needed.
s/needed/fired/
Hmm, but re-arming a timer for each transmited packet must have a cost ?
Quoted 2 lines
Signed-off-by David Daney <[email protected]>
Is there any particular reason periodic is spelled preiodic ?
Quoted 14 lines
---
}drivers/staging/octeon/ethernet.c +3
-4-static void cvm_oct_tx_clean_worker (struct work_struct *work) +static void cvm_oct_preiodic_worker (struct work_struct *work) { -INIT_DELAYED_WORK (&priv->tx_clean_work, - cvm_oct_tx_clean_worker); - +INIT_DELAYED_WORK (&priv->port_periodic_work, + cvm_oct_preiodic_worker);
Re: [PATCH 4/4] Staging: Octeon: Free transmit SKBs in a timely manner.
Quoted 5 lines · Eric Dumazet
Quoted 4 lines · David Daney
If we wait for the once-per-second cleanup to free transmit SKBs, sockets with small transmit buffer sizes might spend most of their time blocked waiting for the cleanup.
Quoted 9 lines
Quoted 6 lines
Normally we do a cleanup for each transmitted packet. We add a watchdog type timer so that we also schedule a timeout for 150uS after a packet is transmitted. The watchdog is reset for each transmitted packet, so for high packet rates, it never expires. At these high rates, the cleanups are done for each packet so the extra watchdog initiated cleanups are not needed.
s/needed/fired/
or perhaps s/are not needed/are neither needed nor fired/
Quoted 2 lines
Hmm, but re-arming a timer for each transmited packet must have a cost ?
The cost is fairly low (less than 10 processor clock cycles). We didn't add this for amusement, people actually do things like only send UDP packets from userspace. Since we can fill the transmit queue faster than it is emptied, the socket transmit buffer is quickly consumed. If we don't free the SKBs in short order, the transmitting process get to take a long sleep (until our previous once per second clean up task was run).
Quoted 4 lines
Quoted 2 lines
Signed-off-by David Daney<[email protected]>Is there any particular reason periodic is spelled preiodic ?
Ha! No good reason. I will correct the spelling.
Thanks,
David Daney
Quoted 19 lines
Quoted 15 lines
---
}drivers/staging/octeon/ethernet.c +3
-4-static void cvm_oct_tx_clean_worker (struct work_struct *work) +static void cvm_oct_preiodic_worker (struct work_struct *work) { -INIT_DELAYED_WORK (&priv->tx_clean_work, - cvm_oct_tx_clean_worker); - +INIT_DELAYED_WORK (&priv->port_periodic_work, + cvm_oct_preiodic_worker);
Re: [PATCH 4/4] Staging: Octeon: Free transmit SKBs in a timely manner.
Quoted 21 lines · David Daney
Quoted 5 lines · Eric Dumazet
Quoted 4 lines · David Daney
If we wait for the once-per-second cleanup to free transmit SKBs, sockets with small transmit buffer sizes might spend most of their time blocked waiting for the cleanup.
Quoted 9 lines
Quoted 6 lines
Normally we do a cleanup for each transmitted packet. We add a watchdog type timer so that we also schedule a timeout for 150uS after a packet is transmitted. The watchdog is reset for each transmitted packet, so for high packet rates, it never expires. At these high rates, the cleanups are done for each packet so the extra watchdog initiated cleanups are not needed.
s/needed/fired/
or perhaps s/are not needed/are neither needed nor fired/
Quoted 2 lines
Hmm, but re-arming a timer for each transmited packet must have a cost ?
Quoted 7 lines
The cost is fairly low (less than 10 processor clock cycles). We didn't add this for amusement, people actually do things like only send UDP packets from userspace. Since we can fill the transmit queue faster than it is emptied, the socket transmit buffer is quickly consumed. If we don't free the SKBs in short order, the transmitting process get to take a long sleep (until our previous once per second clean up task was run).
I understand this, but traditionaly, NIC drivers dont use a timer, but a 'TX complete' interrupt, that usually fires a few us after packet submission on Gigabit speed.
A fast program could try to send X small udp packets in less than 150 us, X being greater than the size of your TX ring.
So your patch makes the window smaller, but it still is there (at physical layer, we'll see a burst of packets, a ~100us delay, then a second burst)
Quoted 9 lines · David Daney
Here are a couple of improvements to the
octeon_ethernetindrivers/staging/octeon. The first patch is just cleanup, the rest are genuine bug fixes.I will reply with the four patches.
We may want to merge via Ralf's linux-mips.org tree as Octeon is infact a MIPS based SOC and he has a bunch of other patches queued there that these depend on.
That's fine with me for them to go through that way.
thanks,
greg k-h
Re: [PATCH 4/4] Staging: Octeon: Free transmit SKBs in a timely manner.
In reply to Eric DumazetQuoted 30 lines · Eric Dumazet
Quoted 21 lines · David Daney
Quoted 5 lines · Eric Dumazet
Quoted 4 lines · David Daney
If we wait for the once-per-second cleanup to free transmit SKBs, sockets with small transmit buffer sizes might spend most of their time blocked waiting for the cleanup.
Quoted 9 lines
Quoted 6 lines
Normally we do a cleanup for each transmitted packet. We add a watchdog type timer so that we also schedule a timeout for 150uS after a packet is transmitted. The watchdog is reset for each transmitted packet, so for high packet rates, it never expires. At these high rates, the cleanups are done for each packet so the extra watchdog initiated cleanups are not needed.
s/needed/fired/
or perhaps s/are not needed/are neither needed nor fired/
Quoted 2 lines
Hmm, but re-arming a timer for each transmited packet must have a cost ?
Quoted 7 lines
The cost is fairly low (less than 10 processor clock cycles). We didn't add this for amusement, people actually do things like only send UDP packets from userspace. Since we can fill the transmit queue faster than it is emptied, the socket transmit buffer is quickly consumed. If we don't free the SKBs in short order, the transmitting process get to take a long sleep (until our previous once per second clean up task was run).
Quoted 4 lines
I understand this, but traditionaly, NIC drivers dont use a timer, but a 'TX complete' interrupt, that usually fires a few us after packet submission on Gigabit speed.
Indeed. Lacking this type of interrupt, the watchdog seemed the best short term solution.
I am investigating the possibility of feeding TX complete notifications back through the RX path where it is possible to generate interrupts. The drawback to this is that it takes a lot more CPU cycles as well as added cache pressure.
Quoted 2 lines
A fast program could try to send X small udp packets in less than 150 us, X being greater than the size of your TX ring.
My TX queue (it is not a ring) size can be made arbitrarily large (currently 1000). 64bytes * 1000 packets * 10 bits/packet / 10e9 bits/sec == 640uS. My watchdog will fire after less than 1/4 of the ring capacity is freed.
Quoted 5 lines
So your patch makes the window smaller, but it still is there (at physical layer, we'll see a burst of packets, a ~100us delay, then a second burst)
With this patch, there will be no burstiness using default socket buffer sizes and packets of arbitrary size on a standard 1gig port.
On the 10gig ports there is the possibility for burstiness as you aptly explain. However, in practice it would be difficult to arrange things to achieve sufficiently high packet rates, so we can live with it like this.
David Daney
PATCH 4/4 Staging: Octeon: Free transmit SKBs in a timely manner (v2).
In reply to David DaneyIf we wait for the once-per-second cleanup to free transmit SKBs, sockets with small transmit buffer sizes might spend most of their time blocked waiting for the cleanup.
Normally we do a cleanup for each transmitted packet. We add a watchdog type timer so that we also schedule a timeout for 150uS after a packet is transmitted. The watchdog is reset for each transmitted packet, so for high packet rates, it never expires. At these high rates, the cleanups are done for each packet so the extra watchdog initiated cleanups are neither needed nor triggered.
---
This version has spelling and comment changes based on feedback from Eric Dumazet.
drivers/staging/octeon/Kconfig | 1 - drivers/staging/octeon/ethernet-defines.h | 5 +- drivers/staging/octeon/ethernet-tx.c | 137 +++++++++++++++++++++++------ drivers/staging/octeon/ethernet-tx.h | 6 +- drivers/staging/octeon/ethernet.c | 47 +++++----- drivers/staging/octeon/octeon-ethernet.h | 9 +-- 6 files changed, 142 insertions(+), 63 deletions(-)
drivers/staging/octeon/Kconfig +0 -1
@@ -3,7 +3,6 @@ config OCTEON_ETHERNET depends on CPU_CAVIUM_OCTEON select PHYLIB select MDIO_OCTEON - select HIGH_RES_TIMERS help This driver supports the builtin ethernet ports on Cavium Networks' products in the Octeon family. This driver supports the
drivers/staging/octeon/ethernet-defines.h +3 -2
@@ -95,10 +95,11 @@/*#define DONT_WRITEBACK(x) 0 */ /* Maximum number of SKBs to try to free per xmit packet. */ -#define MAX_SKB_TO_FREE 10#define MAX_OUT_QUEUE_DEPTH 1000 -#define FAU_NUM_PACKET_BUFFERS_TO_FREE (CVMX_FAU_REG_END - sizeof(uint32_t)) +#define FAU_TOTAL_TX_TO_CLEAN (CVMX_FAU_REG_END - sizeof(uint32_t)) +#define FAU_NUM_PACKET_BUFFERS_TO_FREE (FAU_TOTAL_TX_TO_CLEAN - sizeof(uint32_t)) +#define TOTAL_NUMBER_OF_PORTS (CVMX_PIP_NUM_INPUT_PORTS+1)
drivers/staging/octeon/ethernet-tx.c +110 -27
222 lines not on this page · raw message
drivers/staging/octeon/ethernet-tx.h +3 -3
@@ -29,6 +29,6 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev);int cvm_oct_xmit_pow (struct sk_buff *skb,struct net_device *dev);int cvm_oct_transmit_qos (struct net_device *dev,void *work_queue_entry,int do_free,int qos); -void cvm_oct_tx_shutdown (struct net_device *dev); -void cvm_oct_free_tx_skbs (struct octeon_ethernet *priv); -enum hrtimer_restart cvm_oct_restart_tx (struct hrtimer *timer); +void cvm_oct_tx_initialize (void ); +void cvm_oct_tx_shutdown (void ); +void cvm_oct_tx_shutdown_dev (struct net_device *dev);
drivers/staging/octeon/ethernet.c +23 -24
122 lines not on this page · raw message
drivers/staging/octeon/octeon-ethernet.h +3 -6
@@ -4,7 +4,7 @@* Contact: [email protected] * This file is part of the OCTEON SDK * -* Copyright (c) 2003-2007 Cavium Networks +* Copyright (c) 2003-2010 Cavium Networks * * This file is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License, Version 2, as
@@ -31,8 +31,6 @@#ifndef OCTEON_ETHERNET_H #define OCTEON_ETHERNET_H -#include <linux/hrtimer.h> -/** * This is the definition of the Ethernet driver's private * driver state stored in netdev_priv(dev).
@@ -59,9 +57,7 @@ struct octeon_ethernet {uint64_t link_info;/* Called periodically to check link status */ void (*poll) (struct net_device *dev); -struct hrtimer tx_restart_timer; -ktime_t tx_restart_interval; -struct delayed_work tx_clean_work; +struct delayed_work port_periodic_work;struct work_struct port_work;/* may be unused. */ };
@@ -101,6 +97,7 @@ extern char pow_send_list[];extern struct net_device *cvm_oct_device[];extern struct workqueue_struct *cvm_oct_poll_queue;extern atomic_t cvm_oct_poll_queue_stopping; +extern u64 cvm_oct_tx_poll_interval;extern int max_rx_cpus;extern int rx_napi_weight;
Signature · 1 line
1.6.6
Re: [PATCH 1/4] Staging: octeon: remove unneeded includes
In reply to David DaneyThanks, queued for 2.6.34,
Ralf
Re: [PATCH 2/4] Staging: Octeon: Run phy bus accesses on a workqueue.
In reply to David DaneyThanks, queued for 2.6.34,
Ralf
Re: [PATCH 3/4] MIPS: Octeon: Do proper acknowledgment of CIU timer interrupts.
In reply to David DaneyThanks, queued for 2.6.34,
Ralf
Re: [PATCH 4/4] Staging: Octeon: Free transmit SKBs in a timely manner (v2).
In reply to David DaneyThanks, queued for 2.6.34,
Ralf