* [PATCH net-next v1 2/6] tg3: Remove IRQF_SAMPLE_RANDOM flag from internal tests
From: David Decotigny @ 2011-12-16 18:19 UTC (permalink / raw)
To: Matt Carlson, Michael Chan, netdev, linux-kernel
Cc: Javier Martinez Canillas, Robin Getz, Matt Mackall,
Maciej Żenczykowski, David Decotigny
In-Reply-To: <cover.1324059527.git.decot@googlers.com>
From: Maciej Żenczykowski <maze@google.com>
This complements commit ab392d2d6d4 ("Remove IRQF_SAMPLE_RANDOM flag
from network drivers"), removing IRQF_SAMPLE_RANDOM flag from internal
and self tests.
Tested: no visible regression on 1 actual host + ethtool -t passes.
Signed-off-by: David Decotigny <decot@googlers.com>
---
drivers/net/ethernet/broadcom/tg3.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index e04c4f9..a65b419 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -9387,7 +9387,7 @@ static int tg3_test_interrupt(struct tg3 *tp)
}
err = request_irq(tnapi->irq_vec, tg3_test_isr,
- IRQF_SHARED | IRQF_SAMPLE_RANDOM, dev->name, tnapi);
+ IRQF_SHARED, dev->name, tnapi);
if (err)
return err;
--
1.7.3.1
^ permalink raw reply related
* [PATCH net-next v1 5/6] tg3: implementation of a non-NAPI mode
From: David Decotigny @ 2011-12-16 18:19 UTC (permalink / raw)
To: Matt Carlson, Michael Chan, netdev, linux-kernel
Cc: Javier Martinez Canillas, Robin Getz, Matt Mackall, Tom Herbert,
David Decotigny
In-Reply-To: <cover.1324059527.git.decot@googlers.com>
From: Tom Herbert <therbert@google.com>
The tg3 NIC has a hard limit of 511 descriptors for the receive ring.
Under heavy load of small packets, this device receive queue may not
be serviced fast enough to prevent packets drops. This could be due
to a variety of reasons such as lengthy processing delays of packets
in the stack, softirqs being disabled too long, etc. If the driver is
run in non-NAPI mode, the RX queue is serviced in the device
interrupt, which is much less likely to be deferred for a substantial
period of time.
There are some effects in not using NAPI that need to be considered.
It does increase the chance of live-lock in interrupt handler,
although since the tg3 does interrupt coalescing this is very unlikely
to occur. Also, more code is being run with interrupts disabled
potentially deferring other hardware interrupts. The amount of time
spent in the interrupt handler should be minimized by dequeuing
packets of the device queue and queuing them to a host queue as
quickly as possible.
The default mode of operation remains NAPI and its performances are
kept unchanged (code unchanged). Non-NAPI mode is enabled by
commenting-out CONFIG_TIGON3_NAPI Kconfig parameter.
Signed-off-by: David Decotigny <decot@googlers.com>
---
drivers/net/ethernet/broadcom/Kconfig | 8 ++
drivers/net/ethernet/broadcom/tg3.c | 151 +++++++++++++++++++++++++++++++--
drivers/net/ethernet/broadcom/tg3.h | 5 +
3 files changed, 157 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index f15e72e..e808b3d 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig
@@ -107,6 +107,14 @@ config TIGON3
To compile this driver as a module, choose M here: the module
will be called tg3. This is recommended.
+if TIGON3
+config TIGON3_NAPI
+ bool "Use Rx Polling (NAPI)"
+ default y
+ ---help---
+ Use NAPI for Tigon3 driver. If unsure, say Y.
+endif # TIGON3
+
config BNX2X
tristate "Broadcom NetXtremeII 10Gb support"
depends on PCI
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index ecd6ea5..e3f221d 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -5349,7 +5349,11 @@ static void tg3_tx(struct tg3_napi *tnapi)
pkts_compl++;
bytes_compl += skb->len;
+#ifdef CONFIG_TIGON3_NAPI
dev_kfree_skb(skb);
+#else
+ dev_kfree_skb_any(skb);
+#endif
if (unlikely(tx_bug)) {
tg3_tx_recover(tp);
@@ -5370,11 +5374,15 @@ static void tg3_tx(struct tg3_napi *tnapi)
if (unlikely(netif_tx_queue_stopped(txq) &&
(tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi)))) {
+#ifdef CONFIG_TIGON3_NAPI
__netif_tx_lock(txq, smp_processor_id());
if (netif_tx_queue_stopped(txq) &&
(tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi)))
netif_tx_wake_queue(txq);
__netif_tx_unlock(txq);
+#else
+ netif_tx_wake_queue(txq);
+#endif
}
}
@@ -5694,7 +5702,11 @@ static inline int tg3_coal_adaptive_rx(struct tg3 *tp, int received)
* If both the host and chip were to write into the same ring, cache line
* eviction could occur since both entities want it in an exclusive state.
*/
+#ifdef CONFIG_TIGON3_NAPI
static int tg3_rx(struct tg3_napi *tnapi, int budget)
+#else
+static int tg3_rx(struct tg3_napi *tnapi)
+#endif
{
struct tg3 *tp = tnapi->tp;
u32 work_mask, rx_std_posted = 0;
@@ -5714,7 +5726,11 @@ static int tg3_rx(struct tg3_napi *tnapi, int budget)
received = 0;
std_prod_idx = tpr->rx_std_prod_idx;
jmb_prod_idx = tpr->rx_jmb_prod_idx;
- while (sw_idx != hw_idx && budget > 0) {
+ while (sw_idx != hw_idx
+#ifdef CONFIG_TIGON3_NAPI
+ && budget > 0
+#endif
+ ) {
struct ring_info *ri;
struct tg3_rx_buffer_desc *desc = &tnapi->rx_rcb[sw_idx];
unsigned int len;
@@ -5819,10 +5835,16 @@ static int tg3_rx(struct tg3_napi *tnapi, int budget)
__vlan_hwaccel_put_tag(skb,
desc->err_vlan & RXD_VLAN_MASK);
+#ifdef CONFIG_TIGON3_NAPI
napi_gro_receive(&tnapi->napi, skb);
+#else
+ netif_rx(skb);
+#endif
received++;
+#ifdef CONFIG_TIGON3_NAPI
budget--;
+#endif
next_pkt:
(*post_ptr)++;
@@ -5868,7 +5890,10 @@ next_pkt_nopost:
tpr->rx_jmb_prod_idx);
}
mmiowb();
- } else if (work_mask) {
+ }
+#ifdef CONFIG_TIGON3_NAPI
+ /* TG3_FLG3_ENABLE_RSS is only set in NAPI mode. */
+ else if (work_mask) {
/* rx_std_buffers[] and rx_jmb_buffers[] entries must be
* updated before the producer indices can be updated.
*/
@@ -5880,6 +5905,7 @@ next_pkt_nopost:
if (tnapi != &tp->napi[1])
napi_schedule(&tp->napi[1].napi);
}
+#endif
return received;
}
@@ -5893,7 +5919,12 @@ static void tg3_poll_link(struct tg3 *tp)
if (sblk->status & SD_STATUS_LINK_CHG) {
sblk->status = SD_STATUS_UPDATED |
(sblk->status & ~SD_STATUS_LINK_CHG);
+
+#ifdef CONFIG_TIGON3_NAPI
spin_lock(&tp->lock);
+#else
+ spin_lock_bh(&tp->lock);
+#endif
if (tg3_flag(tp, USE_PHYLIB)) {
tw32_f(MAC_STATUS,
(MAC_STATUS_SYNC_CHANGED |
@@ -5903,11 +5934,16 @@ static void tg3_poll_link(struct tg3 *tp)
udelay(40);
} else
tg3_setup_phy(tp, 0);
+#ifdef CONFIG_TIGON3_NAPI
spin_unlock(&tp->lock);
+#else
+ spin_unlock_bh(&tp->lock);
+#endif
}
}
}
+#ifdef CONFIG_TIGON3_NAPI
static int tg3_rx_prodring_xfer(struct tg3 *tp,
struct tg3_rx_prodring_set *dpr,
struct tg3_rx_prodring_set *spr)
@@ -6207,6 +6243,58 @@ tx_recovery:
return work_done;
}
+#else /* !CONFIG_TIGON3_NAPI */
+
+static void tg3_poll_link_task(struct work_struct *work)
+{
+ struct tg3 *tp = container_of(work, struct tg3, poll_link_task);
+ tg3_poll_link(tp);
+}
+
+static void tg3_int_work(struct tg3_napi *tnapi)
+{
+ struct tg3 *tp = tnapi->tp;
+ struct tg3_hw_status *sblk = tnapi->hw_status;
+
+ if (tg3_flag(tp, TAGGED_STATUS)) {
+ tnapi->last_irq_tag = sblk->status_tag;
+ tnapi->last_tag = tnapi->last_irq_tag;
+ }
+
+ if (!(tg3_flag(tp, USE_LINKCHG_REG) || tg3_flag(tp, POLL_SERDES))) {
+ if (sblk->status & SD_STATUS_LINK_CHG)
+ schedule_work(&tp->poll_link_task);
+ }
+
+ /* run TX completion thread */
+ if (tnapi->hw_status->idx[0].tx_consumer != tnapi->tx_cons) {
+ tg3_tx(tnapi);
+ if (unlikely(tg3_flag(tp, TX_RECOVERY_PENDING)))
+ goto tx_recovery;
+ }
+
+ /* run RX thread */
+ if (*(tnapi->rx_rcb_prod_idx) != tnapi->rx_rcb_ptr)
+ tg3_rx(tnapi);
+
+ if (unlikely(tg3_flag(tp, TX_RECOVERY_PENDING)))
+ goto tx_recovery;
+
+ if (!(tg3_flag(tp, TAGGED_STATUS)))
+ sblk->status &= ~SD_STATUS_UPDATED;
+
+ /* Reenable interrupts */
+ tg3_int_reenable(tnapi);
+
+ return;
+
+tx_recovery:
+ schedule_work(&tp->reset_task);
+}
+
+#endif /* CONFIG_TIGON3_NAPI */
+
+#ifdef CONFIG_TIGON3_NAPI
static void tg3_napi_disable(struct tg3 *tp)
{
int i;
@@ -6239,11 +6327,14 @@ static void tg3_napi_fini(struct tg3 *tp)
for (i = 0; i < tp->irq_cnt; i++)
netif_napi_del(&tp->napi[i].napi);
}
+#endif
static inline void tg3_netif_stop(struct tg3 *tp)
{
tp->dev->trans_start = jiffies; /* prevent tx timeout */
+#ifdef CONFIG_TIGON3_NAPI
tg3_napi_disable(tp);
+#endif
netif_tx_disable(tp->dev);
}
@@ -6255,7 +6346,9 @@ static inline void tg3_netif_start(struct tg3 *tp)
*/
netif_tx_wake_all_queues(tp->dev);
+#ifdef CONFIG_TIGON3_NAPI
tg3_napi_enable(tp);
+#endif
tp->napi[0].hw_status->status |= SD_STATUS_UPDATED;
tg3_enable_ints(tp);
}
@@ -6303,7 +6396,11 @@ static irqreturn_t tg3_msi_1shot(int irq, void *dev_id)
prefetch(&tnapi->rx_rcb[tnapi->rx_rcb_ptr]);
if (likely(!tg3_irq_sync(tp)))
+#ifdef CONFIG_TIGON3_NAPI
napi_schedule(&tnapi->napi);
+#else
+ tg3_int_work(tnapi);
+#endif
return IRQ_HANDLED;
}
@@ -6329,7 +6426,11 @@ static irqreturn_t tg3_msi(int irq, void *dev_id)
*/
tw32_mailbox(tnapi->int_mbox, 0x00000001);
if (likely(!tg3_irq_sync(tp)))
+#ifdef CONFIG_TIGON3_NAPI
napi_schedule(&tnapi->napi);
+#else
+ tg3_int_work(tnapi);
+#endif
return IRQ_RETVAL(1);
}
@@ -6371,7 +6472,11 @@ static irqreturn_t tg3_interrupt(int irq, void *dev_id)
sblk->status &= ~SD_STATUS_UPDATED;
if (likely(tg3_has_work(tnapi))) {
prefetch(&tnapi->rx_rcb[tnapi->rx_rcb_ptr]);
+#ifdef CONFIG_TIGON3_NAPI
napi_schedule(&tnapi->napi);
+#else
+ tg3_int_work(tnapi);
+#endif
} else {
/* No work, shared interrupt perhaps? re-enable
* interrupts, and flush that PCI write
@@ -6429,7 +6534,11 @@ static irqreturn_t tg3_interrupt_tagged(int irq, void *dev_id)
prefetch(&tnapi->rx_rcb[tnapi->rx_rcb_ptr]);
+#ifdef CONFIG_TIGON3_NAPI
napi_schedule(&tnapi->napi);
+#else
+ tg3_int_work(tnapi);
+#endif
out:
return IRQ_RETVAL(handled);
@@ -6470,7 +6579,9 @@ static int tg3_restart_hw(struct tg3 *tp, int reset_phy)
tg3_full_unlock(tp);
del_timer_sync(&tp->timer);
tp->irq_sync = 0;
+#ifdef CONFIG_TIGON3_NAPI
tg3_napi_enable(tp);
+#endif
dev_close(tp->dev);
tg3_full_lock(tp, 0);
}
@@ -6804,10 +6915,15 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
budget = tg3_tx_avail(tnapi);
- /* We are running in BH disabled context with netif_tx_lock
- * and TX reclaim runs via tp->napi.poll inside of a software
- * interrupt. Furthermore, IRQ processing runs lockless so we have
- * no IRQ context deadlocks to worry about either. Rejoice!
+ /* When in NAPI mode, we are running in BH disabled context
+ * with netif_tx_lock and TX reclaim runs via tp->napi.poll
+ * inside of a software interrupt. Furthermore, IRQ
+ * processing runs lockless so we have no IRQ context
+ * deadlocks to worry about either. Rejoice!
+ *
+ * When in non-NAPI mode, we are running in BH disabled
+ * context with netif_tx_lock and TX reclaim runs lockless in
+ * interrupt context.
*/
if (unlikely(budget <= (skb_shinfo(skb)->nr_frags + 1))) {
if (!netif_tx_queue_stopped(txq)) {
@@ -9847,9 +9963,10 @@ static int tg3_open(struct net_device *dev)
if (err)
goto err_out1;
+#ifdef CONFIG_TIGON3_NAPI
tg3_napi_init(tp);
-
tg3_napi_enable(tp);
+#endif
for (i = 0; i < tp->irq_cnt; i++) {
struct tg3_napi *tnapi = &tp->napi[i];
@@ -9942,8 +10059,10 @@ err_out3:
}
err_out2:
+#ifdef CONFIG_TIGON3_NAPI
tg3_napi_disable(tp);
tg3_napi_fini(tp);
+#endif
tg3_free_consistent(tp);
err_out1:
@@ -9958,7 +10077,9 @@ static int tg3_close(struct net_device *dev)
int i;
struct tg3 *tp = netdev_priv(dev);
+#ifdef CONFIG_TIGON3_NAPI
tg3_napi_disable(tp);
+#endif
tg3_reset_task_cancel(tp);
netif_tx_stop_all_queues(dev);
@@ -9988,7 +10109,9 @@ static int tg3_close(struct net_device *dev)
memset(&tp->net_stats_prev, 0, sizeof(tp->net_stats_prev));
memset(&tp->estats_prev, 0, sizeof(tp->estats_prev));
+#ifdef CONFIG_TIGON3_NAPI
tg3_napi_fini(tp);
+#endif
tg3_free_consistent(tp);
@@ -14223,10 +14346,13 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
tg3_flag_set(tp, 1SHOT_MSI);
}
+#ifdef CONFIG_TIGON3_NAPI
+ /* Do not enable MSIX in non-NAPI mode. */
if (tg3_flag(tp, 57765_PLUS)) {
tg3_flag_set(tp, SUPPORT_MSIX);
tp->irq_max = TG3_IRQ_MAX_VECS;
}
+#endif
}
if (tg3_flag(tp, 5755_PLUS))
@@ -15601,6 +15727,9 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
spin_lock_init(&tp->lock);
spin_lock_init(&tp->indirect_lock);
INIT_WORK(&tp->reset_task, tg3_reset_task);
+#ifndef CONFIG_TIGON3_NAPI
+ INIT_WORK(&tp->poll_link_task, tg3_poll_link_task);
+#endif
tp->regs = pci_ioremap_bar(pdev, BAR_0);
if (!tp->regs) {
@@ -15821,6 +15950,14 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
goto err_out_apeunmap;
}
+#ifdef CONFIG_TIGON3_NAPI
+ netdev_info(dev, "Tigon3 driver %s loaded in NAPI mode.\n",
+ DRV_MODULE_VERSION);
+#else
+ netdev_info(dev, "Tigon3 driver %s loaded in non-NAPI mode.\n",
+ DRV_MODULE_VERSION);
+#endif
+
netdev_info(dev, "Tigon3 [partno(%s) rev %04x] (%s) MAC address %pM\n",
tp->board_part_number,
tp->pci_chip_rev_id,
diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
index 695cf14..b023e96 100644
--- a/drivers/net/ethernet/broadcom/tg3.h
+++ b/drivers/net/ethernet/broadcom/tg3.h
@@ -2832,7 +2832,9 @@ struct tg3_rx_prodring_set {
#define TG3_IRQ_MAX_VECS TG3_IRQ_MAX_VECS_RSS
struct tg3_napi {
+#ifdef CONFIG_TIGON3_NAPI
struct napi_struct napi ____cacheline_aligned;
+#endif
struct tg3 *tp;
struct tg3_hw_status *hw_status;
@@ -3167,6 +3169,9 @@ struct tg3 {
struct tg3_hw_stats *hw_stats;
dma_addr_t stats_mapping;
struct work_struct reset_task;
+#ifndef CONFIG_TIGON3_NAPI
+ struct work_struct poll_link_task;
+#endif
int nvram_lock_cnt;
u32 nvram_size;
--
1.7.3.1
^ permalink raw reply related
* [PATCH net-next v1 6/6] tg3: use netif_tx_start_queue instead of wake_queue when no reschedule needed
From: David Decotigny @ 2011-12-16 18:19 UTC (permalink / raw)
To: Matt Carlson, Michael Chan, netdev, linux-kernel
Cc: Javier Martinez Canillas, Robin Getz, Matt Mackall, Ying Cai,
David Decotigny
In-Reply-To: <cover.1324059527.git.decot@googlers.com>
From: Ying Cai <ycai@google.com>
This commit replaces netif_tx_wake_queue() with netif_tx_start_queue()
when __netif_schedule() is not needed. It also adds code to deal with
race condition between netif_tx_start_queue() and netif_tx_stop_queue().
Signed-off-by: David Decotigny <decot@googlers.com>
---
drivers/net/ethernet/broadcom/tg3.c | 23 ++++++++++++-----------
1 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index e3f221d..311e073 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -6860,9 +6860,10 @@ static int tg3_tso_bug(struct tg3 *tp, struct sk_buff *skb)
{
struct sk_buff *segs, *nskb;
u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
+ struct tg3_napi *tnapi = &tp->napi[0];
/* Estimate the number of fragments in the worst case */
- if (unlikely(tg3_tx_avail(&tp->napi[0]) <= frag_cnt_est)) {
+ if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) {
netif_stop_queue(tp->dev);
/* netif_tx_stop_queue() must be done before checking
@@ -6871,10 +6872,10 @@ static int tg3_tso_bug(struct tg3 *tp, struct sk_buff *skb)
* netif_tx_queue_stopped().
*/
smp_mb();
- if (tg3_tx_avail(&tp->napi[0]) <= frag_cnt_est)
+ if (tg3_tx_avail(tnapi) <= TG3_TX_WAKEUP_THRESH(tnapi))
return NETDEV_TX_BUSY;
- netif_wake_queue(tp->dev);
+ netif_start_queue(tp->dev);
}
segs = skb_gso_segment(skb, tp->dev->features & ~NETIF_F_TSO);
@@ -6926,14 +6927,14 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
* interrupt context.
*/
if (unlikely(budget <= (skb_shinfo(skb)->nr_frags + 1))) {
- if (!netif_tx_queue_stopped(txq)) {
- netif_tx_stop_queue(txq);
+ /* This is a hard error, log it. */
+ netdev_err(dev, "BUG! Tx Ring full when queue awake!\n");
- /* This is a hard error, log it. */
- netdev_err(dev,
- "BUG! Tx Ring full when queue awake!\n");
- }
- return NETDEV_TX_BUSY;
+ netif_tx_stop_queue(txq);
+ smp_mb();
+ if (tg3_tx_avail(tnapi) <= TG3_TX_WAKEUP_THRESH(tnapi))
+ return NETDEV_TX_BUSY;
+ netif_tx_start_queue(txq);
}
entry = tnapi->tx_prod;
@@ -7100,7 +7101,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
*/
smp_mb();
if (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi))
- netif_tx_wake_queue(txq);
+ netif_tx_start_queue(txq);
}
mmiowb();
--
1.7.3.1
^ permalink raw reply related
* [PATCH net-next v1 4/6] tg3: move functions related to reset_task together
From: David Decotigny @ 2011-12-16 18:19 UTC (permalink / raw)
To: Matt Carlson, Michael Chan, netdev, linux-kernel
Cc: Javier Martinez Canillas, Robin Getz, Matt Mackall,
David Decotigny
In-Reply-To: <cover.1324059527.git.decot@googlers.com>
This prepares next patches: simply move functions related to
reset_task close to its definition.
Signed-off-by: David Decotigny <decot@googlers.com>
---
drivers/net/ethernet/broadcom/tg3.c | 25 +++++++++++++------------
1 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 9deb6a6..ecd6ea5 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -416,6 +416,7 @@ static const struct {
static inline void tg3_full_lock(struct tg3 *tp, int irq_sync);
static inline void tg3_full_unlock(struct tg3 *tp);
+static inline void tg3_reset_task_schedule(struct tg3 *tp);
static void tg3_write32(struct tg3 *tp, u32 off, u32 val)
{
@@ -6080,18 +6081,6 @@ static int tg3_poll_work(struct tg3_napi *tnapi, int work_done, int budget)
return work_done;
}
-static inline void tg3_reset_task_schedule(struct tg3 *tp)
-{
- if (!test_and_set_bit(TG3_FLAG_RESET_TASK_PENDING, tp->tg3_flags))
- schedule_work(&tp->reset_task);
-}
-
-static inline void tg3_reset_task_cancel(struct tg3 *tp)
-{
- cancel_work_sync(&tp->reset_task);
- tg3_flag_clear(tp, RESET_TASK_PENDING);
-}
-
static int tg3_poll_msix(struct napi_struct *napi, int budget)
{
struct tg3_napi *tnapi = container_of(napi, struct tg3_napi, napi);
@@ -6543,6 +6532,18 @@ out:
tg3_flag_clear(tp, RESET_TASK_PENDING);
}
+static inline void tg3_reset_task_schedule(struct tg3 *tp)
+{
+ if (!test_and_set_bit(TG3_FLAG_RESET_TASK_PENDING, tp->tg3_flags))
+ schedule_work(&tp->reset_task);
+}
+
+static inline void tg3_reset_task_cancel(struct tg3 *tp)
+{
+ cancel_work_sync(&tp->reset_task);
+ tg3_flag_clear(tp, RESET_TASK_PENDING);
+}
+
static void tg3_tx_timeout(struct net_device *dev)
{
struct tg3 *tp = netdev_priv(dev);
--
1.7.3.1
^ permalink raw reply related
* Re: [PATCH 00/10 net-next] Introduce per interface ipv4 statistics
From: David Miller @ 2011-12-16 18:27 UTC (permalink / raw)
To: eric.dumazet; +Cc: igorm, netdev
In-Reply-To: <1324050080.25554.34.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 16 Dec 2011 16:41:20 +0100
> 1) Why is it needed ? Any RFC requires this bloat ?
I'm not allowing something like this into ipv4, there are too many
terrible side effects in ipv6 because we do it there.
The mere necessity of having to have some device handle at every single packet
processing spot is incredibly painful, and makes changes in the ipv6 packet
paths 10 times more difficult than they otherwise would be.
I'm not allowing this difficulty to be added to the ipv4 side as well.
^ permalink raw reply
* Re: [PATCH 00/10 net-next] Introduce per interface ipv4 statistics
From: David Miller @ 2011-12-16 18:28 UTC (permalink / raw)
To: igorm; +Cc: eric.dumazet, netdev
In-Reply-To: <CAFdo_mWY3030_WsL_awXFMaviMA5MGJP_gMs0cM=6KeGQft6-Q@mail.gmail.com>
From: Igor Maravić <igorm@etf.rs>
Date: Fri, 16 Dec 2011 16:58:21 +0100
>> 1) Why is it needed ? Any RFC requires this bloat ?
>>
>
> RFC4293 defines ipIfStatsTable.
That's too bad.
^ permalink raw reply
* Re: twice past the taps, thence out to net?
From: Jesse Brandeburg @ 2011-12-16 18:28 UTC (permalink / raw)
To: Eric Dumazet
Cc: Rick Jones, Stephen Hemminger, Vijay Subramanian, tcpdump-workers,
netdev, Matthew Vick, Jeff Kirsher
In-Reply-To: <1324009676.2562.9.camel@edumazet-laptop>
On Thu, Dec 15, 2011 at 8:27 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le jeudi 15 décembre 2011 à 14:22 -0800, Rick Jones a écrit :
>> On 12/15/2011 11:00 AM, Eric Dumazet wrote:
>> >> Device's work better if the driver proactively manages stop_queue/wake_queue.
>> >> Old devices used TX_BUSY, but newer devices tend to manage the queue
>> >> themselves.
>> >>
>> >
>> > Some 'new' drivers like igb can be fooled in case skb is gso segmented ?
>> >
>> > Because igb_xmit_frame_ring() needs skb_shinfo(skb)->nr_frags + 4
>> > descriptors, igb should stop its queue not at MAX_SKB_FRAGS + 4, but
>> > MAX_SKB_FRAGS*4
can you please help me understand the need for MAX_SKB_FRAGS * 4 as
the requirement? Currently driver uses logic like
in hard_start_tx: hey I just finished a tx, I should stop the qdisc if
I don't have room (in tx descriptors) for a worst case transmit skb
(MAX_SKB_FRAGS + 4) the next time I'm called.
when cleaning from interrupt: My cleanup is done, do I have enough
free tx descriptors (should be MAX_SKB_FRAGS + 4) for a worst case
transmit? If yes, restart qdisc.
I'm missing the jump from the above logic to using MAX_SKB_FRAGS * 4
(== (18 * 4) == 72) as the minimum number of descriptors I need for a
worst case TSO. Each descriptor can point to up to 16kB of contiguous
memory, typically we use 1 for offload context setup, 1 for skb->data,
and 1 for each page. I think we may be overestimating with
MAX_SKB_FRAGS + 4, but that should be no big deal.
^ permalink raw reply
* Re: [PATCH 00/10 net-next] Introduce per interface ipv4 statistics
From: Christoph Lameter @ 2011-12-16 18:30 UTC (permalink / raw)
To: Eric Dumazet; +Cc: igorm, netdev, davem
In-Reply-To: <1324058911.25554.82.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1807 bytes --]
On Fri, 16 Dec 2011, Eric Dumazet wrote:
> Le vendredi 16 décembre 2011 à 11:29 -0600, Christoph Lameter a écrit :
>
> > I have some latency critical processes here and I wish I could get
> > networking in general off the processor where the latency sensitive stuff
> > is running should that process decide to do some calls that cause network
> > I/O.
> >
> > Traditional networking is a slow process these days.
>
>
> Most of the slow process is in the process scheduler actually and cache
> line misses in large TCP structures, but also in many layers (Qdisc...),
> not counting icache footprint.
>
> We slowly improve things, but its always a tradeoff (did I said code
> bloat ?)
>
> If you have dedicated network thread(s) in your application, bound to
> the right cpu(s), then all network stack can be run on the cpus you
> decide [ Also needs using correct irq affinities for the NIC interrupts]
>
> This means your latency critical threads should delegate their network
> IO (like disk IO) to other threads, _and_ avoid being blocked in
> scheduler land.
Right. So why can the OS not do this? If I do read/write via a socket then
do the usual processing as much as possible on the current cpu. Never
schedule any operations later on this cpu. Run scheduled tasks only on the
cpus that are allowed to be used for network stuff.
> Given the nature of socket api, I am not sure adding a layer to
> transparently delegate network IO to a pool of dedicated cpus would be a
> win.
The problem is that the socket layer is drag for low latency apps. They
only use the socket api in non latency critical sections. If an action in
a non latency critical section causes the processor to be interrupted
later in a latency critical section then this is not good.
^ permalink raw reply
* Re: [PATCH 00/10 net-next] Introduce per interface ipv4 statistics
From: David Miller @ 2011-12-16 18:31 UTC (permalink / raw)
To: eric.dumazet; +Cc: igorm, netdev
In-Reply-To: <1324053184.25554.47.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 16 Dec 2011 17:33:04 +0100
> Le vendredi 16 décembre 2011 à 16:58 +0100, Igor Maravić a écrit :
>> Also I did it because ipv6 has per interface stats.
>
> I was considering _removing_ them, or make it optional.
It is not the only huge problem with this feature.
The other one is, as I said, it requires having some inetdev available in
every single corner of every packet processing path, just so we can hit
some statistic.
Which means we have all of this special case code to make sure we have at
least a reference to the loopback device when a physical device is brought
down or removed.
It's painful, ugly, and not something I want propagating into ipv4 as well.
There is no way I'm letting this feature in, I've already wasted enough time
when trying to make ipv6 changes because of this facility.
^ permalink raw reply
* Re: [PATCH] asix: new device id
From: David Miller @ 2011-12-16 18:32 UTC (permalink / raw)
To: jkosina; +Cc: aurel, linux-kernel, netdev
In-Reply-To: <alpine.LRH.2.00.1112161737040.15661@twin.jikos.cz>
From: Jiri Kosina <jkosina@suse.cz>
Date: Fri, 16 Dec 2011 17:38:13 +0100 (CET)
> On Fri, 16 Dec 2011, Aurelien Jacobs wrote:
>
>> Adds the device id needed for the USB Ethernet Adapter delivered by
>> ASUS with their Zenbook.
>>
>> Signed-off-by: Aurelien Jacobs <aurel@gnuage.org>
>> ---
>> drivers/net/usb/asix.c | 4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/usb/asix.c b/drivers/net/usb/asix.c
>> index e6fed4d..e95f0e6 100644
>> --- a/drivers/net/usb/asix.c
>> +++ b/drivers/net/usb/asix.c
>> @@ -1655,6 +1655,10 @@ static const struct usb_device_id products [] = {
>> // ASIX 88772a
>> USB_DEVICE(0x0db0, 0xa877),
>> .driver_info = (unsigned long) &ax88772_info,
>> +}, {
>> + // Asus USB Ethernet Adapter
>> + USB_DEVICE (0x0b95, 0x7e2b),
>> + .driver_info = (unsigned long) &ax88772_info,
>> },
>> { }, // END
>> };
>
> Adding netdev to CC so that it doesn't get lost.
Only submitting the actual patch, unquoted, to netdev makes sure it gets
queued up in patchwork, and therefore not lost.
^ permalink raw reply
* Re: [PATCH 00/10 net-next] Introduce per interface ipv4 statistics
From: David Miller @ 2011-12-16 18:39 UTC (permalink / raw)
To: cl; +Cc: eric.dumazet, igorm, netdev
In-Reply-To: <alpine.DEB.2.00.1112161128060.26765@router.home>
From: Christoph Lameter <cl@linux.com>
Date: Fri, 16 Dec 2011 11:29:57 -0600 (CST)
> I have some latency critical processes here and I wish I could get
I hear there's FPGAs for that.... what drives me nuts about these
discussions is that custom silicon for trading platforms is all but
inevitable, and will save tons of space and power utilization to boot.
Madly trying to focus on lowering latency in a general purpose OS so
much just to compare two numbers and execute a trade is silly when in
the end it'll be implemented in a discrete circuit.
This stuff is special purpose, so let's treat it as such.
Lowering latency is a good goal, don't get me wrong, but the extreme
goals set out in these cases is for a very specific small group of
users.
^ permalink raw reply
* Re: [PATCH net-next v1 0/6] tg3: adaptive interrupt coalescing, non-napi mode
From: David Miller @ 2011-12-16 18:42 UTC (permalink / raw)
To: decot; +Cc: mcarlson, mchan, netdev, linux-kernel, martinez.javier, rgetz,
mpm
In-Reply-To: <cover.1324059527.git.decot@googlers.com>
From: David Decotigny <decot@googlers.com>
Date: Fri, 16 Dec 2011 10:19:43 -0800
> This series implements adaptive interrupt coalescing for tg3 NIC,
> improving performance substancially. It also implements non-NAPI mode
> for specific system loads.
I specifically removed the dynamic IRQ coalescing from this driver years
ago.
It's too susceptible to state changes.
I am sure that you have some nice benchmark for which one scheme helps,
but more generally it is not possible to make it such that you will avoid
the case where the network flow pattern changes by the time you change
the chip configuration and thus the result is suboptimal.
I highly recommend these changes are not applied, because they will hurt
someone.
^ permalink raw reply
* Re: [PATCH 0/13] Dumping AF_UNIX sockets via netlink
From: David Miller @ 2011-12-16 18:50 UTC (permalink / raw)
To: xemul; +Cc: netdev
In-Reply-To: <4EE9EB2A.4040909@parallels.com>
From: Pavel Emelyanov <xemul@parallels.com>
Date: Thu, 15 Dec 2011 16:42:18 +0400
> Make the unix_diag.ko module, which is the AF_UNIX client for the sock_diag.
>
> Use the sock_i_ino() as the primary ID key for sockets. This is currently the
> only unique (except for the sk address itself) ID of a unix socket and is de
> facto used in the ss tool to identify sockets. Thus the basic nlk request and
> response structures operate on this ID. Other socket info (sun_name, peer, etc.)
> are reported in the respective NLA-s (patches 8 through 12).
>
> There's a locking trickery in patch #11. I've tried to study it carefully and
> checked with lockdep, but anyway, please, pay special attention to it.
>
> The patch for ss tool is also included.
>
> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
Looks good, applied.
I'm slightly confused by the module alias strings these diag modules
are using, can you explain it to me?
On one side it looks like it wants to see a suffix of "-${AF_INET}-${IPPROTO_TCP}"
but in the macros you pass in one numerical value, which is AF_INET minus the
protocol value.
How does that work?
Thanks.
^ permalink raw reply
* Re: [PATCH net-next 1/3] ethtool: Clarify use of size field for ETHTOOL_GRXFHINDIR
From: David Miller @ 2011-12-16 18:53 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers, mcarlson
In-Reply-To: <1323993076.2773.23.camel@bwh-desktop>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 15 Dec 2011 23:51:16 +0000
> In order to find out the device's RX flow hash table size, ethtool
> initially uses ETHTOOL_GRXFHINDIR with a buffer size of zero. This
> must be supported, but it is not necessary to support any other user
> buffer size less than the device table size.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 2/3] ethtool: Centralise validation of ETHTOOL_{G,S}RXFHINDIR parameters
From: David Miller @ 2011-12-16 18:53 UTC (permalink / raw)
To: bhutchings
Cc: netdev, linux-net-drivers, mcarlson, eilong, dm, sbhatewara,
pv-drivers
In-Reply-To: <1323993301.2773.26.camel@bwh-desktop>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 15 Dec 2011 23:55:01 +0000
> Add a new ethtool operation (get_rxfh_indir_size) to get the
> indirectional table size. Use this to validate the user buffer size
> before calling get_rxfh_indir or set_rxfh_indir. Use get_rxnfc to get
> the number of RX rings, and validate the contents of the new
> indirection table before calling set_rxfh_indir. Remove this
> validation from drivers.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 3/3] ethtool: Define and apply a default policy for RX flow hash indirection
From: David Miller @ 2011-12-16 18:54 UTC (permalink / raw)
To: bhutchings
Cc: linux-net-drivers, mcarlson, eilong, dm, sbhatewara, netdev,
pv-drivers
In-Reply-To: <1323993409.2773.28.camel@bwh-desktop>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 15 Dec 2011 23:56:49 +0000
> All drivers that support modification of the RX flow hash indirection
> table initialise it in the same way: RX rings are assigned to table
> entries in rotation. Make that default policy explicit by having them
> call a ethtool_rxfh_indir_default() function.
>
> In the ethtool core, add support for a zero size value for
> ETHTOOL_SRXFHINDIR, which resets the table to this default.
>
> Partly-suggested-by: Matt Carlson <mcarlson@broadcom.com>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Applied.
> + * @size: On entry, the array size of the user buffer, which may be zero.
> + * On return from %ETHTOOL_GRXFHINDIR, the array size of the hardware
> + * indirection table.
^^^
Trailing whitespace I had to fixup while applying this, just FYI.
^ permalink raw reply
* Re: [PATCH net-next 1/2] sch_sfq: store flow_keys in skb->cb[]
From: David Miller @ 2011-12-16 18:54 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1324013979.2562.31.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 16 Dec 2011 06:39:39 +0100
> Instead of using automatic flow_keys variable in sfq_hash(), use storage
> from skb->cb[] so that we can reuse it later if we want to rehash queues
> when perturbation timer fires.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Where is "2/2"?
^ permalink raw reply
* Re: tc filter show not displaying anything
From: John A. Sullivan III @ 2011-12-16 19:06 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1324014178.2562.33.camel@edumazet-laptop>
On Fri, 2011-12-16 at 06:42 +0100, Eric Dumazet wrote:
> Le vendredi 16 décembre 2011 à 00:39 -0500, John A. Sullivan III a
> écrit :
>
> > Ouch! That was right out of the book so to speak. Thanks for pointing
> > it out - now I see it is right in the man page. Is best practice to not
> > perturb and live with the potentially unbalanced queues or just to set
> > it even higher? Thanks - John
> >
>
> I'll fix this today, because rehashing up to 128 packets is not that
> expensive.
>
> In the meantime, just use a higher timer (say 60 seconds), and if your
> kernel is recent enough, use a higher 'divisor' value (default 1024, can
> be up to 65536) to lower risk of hash collisions.
<snip>
Thanks. Alas, no divisor parameter in Debian Squeeze:
sfq [ limit NUMBER ] [ perturb SECS ] [ quantum BYTES ]
^ permalink raw reply
* Re: [PATCH] tg3: Make the RSS indir tbl admin configurable
From: Matt Carlson @ 2011-12-16 19:19 UTC (permalink / raw)
To: Ben Hutchings
Cc: Matthew Carlson, davem@davemloft.net, netdev@vger.kernel.org,
Michael Chan
In-Reply-To: <1324004674.2825.226.camel@deadeye>
On Thu, Dec 15, 2011 at 07:04:34PM -0800, Ben Hutchings wrote:
> On Thu, 2011-12-15 at 16:47 -0800, Matt Carlson wrote:
> > This patch adds the ethtool callbacks necessary to change the rss
> > indirection table from userspace. When setting the indirection table
> > through set_rxfh_indir, an indirection table size of zero is
> > interpreted to mean that the admin wants to relinquish control of the
> > table to the driver. Should the number of interrupts change (e.g.
> > across a close / open call, or through a reset), any indirection table
> > values that exceed the number of RSS queues or interrupt vectors will
> > be automatically scaled back to values within range.
> >
> > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> > Signed-off-by: Michael Chan <mchan@broadcom.com>
> > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
>
> No I didn't!
>
> Ben.
Sorry. This part of procedure I'm unsure about. I didn't want to lose
the ack of your contributions to this patch. The SOB does imply that
you are O.K. with it as-is though, which you may or may not be aligned
with. I apologize if I was putting words in your mouth.
What should I have done instead? Would Dave put the SOB on your behalf
as he accepts it?
> > Reviewed-by: Benjamin Li <benli@broadcom.com>
> > ---
> > drivers/net/ethernet/broadcom/tg3.c | 126 ++++++++++++++++++++++++++++++++++-
> > drivers/net/ethernet/broadcom/tg3.h | 1 +
> > 2 files changed, 126 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> > index 8bf11ca..87f70f6 100644
> > --- a/drivers/net/ethernet/broadcom/tg3.c
> > +++ b/drivers/net/ethernet/broadcom/tg3.c
> > @@ -8229,9 +8229,19 @@ void tg3_rss_init_indir_tbl(struct tg3 *tp)
> >
> > if (tp->irq_cnt <= 2)
> > memset(&tp->rss_ind_tbl[0], 0, sizeof(tp->rss_ind_tbl));
> > - else
> > + else if (tg3_flag(tp, USER_INDIR_TBL)) {
> > + /* Validate table against current IRQ count */
> > + for (i = 0; i < TG3_RSS_INDIR_TBL_SIZE; i++) {
> > + if (tp->rss_ind_tbl[i] >= tp->irq_cnt - 1) {
> > + /* Bring the index within range */
> > + tp->rss_ind_tbl[i] = tp->rss_ind_tbl[i] %
> > + (tp->irq_cnt - 1);
> > + }
> > + }
> > + } else {
> > for (i = 0; i < TG3_RSS_INDIR_TBL_SIZE; i++)
> > tp->rss_ind_tbl[i] = i % (tp->irq_cnt - 1);
> > + }
> > }
> >
> > void tg3_rss_write_indir_tbl(struct tg3 *tp)
> > @@ -10719,6 +10729,117 @@ static int tg3_get_sset_count(struct net_device *dev, int sset)
> > }
> > }
> >
> > +static int tg3_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
> > + u32 *rules __always_unused)
> > +{
> > + struct tg3 *tp = netdev_priv(dev);
> > +
> > + if (!tg3_flag(tp, SUPPORT_MSIX))
> > + return -EOPNOTSUPP;
> > +
> > + switch (info->cmd) {
> > + case ETHTOOL_GRXRINGS:
> > + if (netif_running(tp->dev))
> > + info->data = tp->irq_cnt;
> > + else {
> > + info->data = num_online_cpus();
> > + if (info->data > TG3_IRQ_MAX_VECS_RSS)
> > + info->data = TG3_IRQ_MAX_VECS_RSS;
> > + }
> > +
> > + /* The first interrupt vector only
> > + * handles link interrupts.
> > + */
> > + info->data -= 1;
> > + return 0;
> > +
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > +static int tg3_get_rxfh_indir(struct net_device *dev,
> > + struct ethtool_rxfh_indir *indir)
> > +{
> > + struct tg3 *tp = netdev_priv(dev);
> > + int i;
> > +
> > + if (!tg3_flag(tp, SUPPORT_MSIX))
> > + return -EOPNOTSUPP;
> > +
> > + if (!indir->size) {
> > + indir->size = TG3_RSS_INDIR_TBL_SIZE;
> > + return 0;
> > + }
> > +
> > + if (indir->size != TG3_RSS_INDIR_TBL_SIZE)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < TG3_RSS_INDIR_TBL_SIZE; i++)
> > + indir->ring_index[i] = tp->rss_ind_tbl[i];
> > +
> > + return 0;
> > +}
> > +
> > +static int tg3_set_rxfh_indir(struct net_device *dev,
> > + const struct ethtool_rxfh_indir *indir)
> > +{
> > + struct tg3 *tp = netdev_priv(dev);
> > + size_t i;
> > +
> > + if (!tg3_flag(tp, SUPPORT_MSIX))
> > + return -EOPNOTSUPP;
> > +
> > + if (!indir->size) {
> > + tg3_flag_clear(tp, USER_INDIR_TBL);
> > + tg3_rss_init_indir_tbl(tp);
> > + } else {
> > + int limit;
> > +
> > + /* Validate size and indices */
> > + if (indir->size != TG3_RSS_INDIR_TBL_SIZE)
> > + return -EINVAL;
> > +
> > + if (netif_running(dev))
> > + limit = tp->irq_cnt;
> > + else {
> > + limit = num_online_cpus();
> > + if (limit > TG3_IRQ_MAX_VECS_RSS)
> > + limit = TG3_IRQ_MAX_VECS_RSS;
> > + }
> > +
> > + /* The first interrupt vector only
> > + * handles link interrupts.
> > + */
> > + limit -= 1;
> > +
> > + /* Check the indices in the table.
> > + * Leave the existing table unmodified
> > + * if an error is detected.
> > + */
> > + for (i = 0; i < TG3_RSS_INDIR_TBL_SIZE; i++)
> > + if (indir->ring_index[i] >= limit)
> > + return -EINVAL;
> > +
> > + tg3_flag_set(tp, USER_INDIR_TBL);
> > +
> > + for (i = 0; i < TG3_RSS_INDIR_TBL_SIZE; i++)
> > + tp->rss_ind_tbl[i] = indir->ring_index[i];
> > + }
> > +
> > + if (!netif_running(dev) || !tg3_flag(tp, ENABLE_RSS))
> > + return 0;
> > +
> > + /* It is legal to write the indirection
> > + * table while the device is running.
> > + */
> > + tg3_full_lock(tp, 0);
> > + tg3_rss_write_indir_tbl(tp);
> > + tg3_full_unlock(tp);
> > +
> > + return 0;
> > +}
> > +
> > static void tg3_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
> > {
> > switch (stringset) {
> > @@ -11949,6 +12070,9 @@ static const struct ethtool_ops tg3_ethtool_ops = {
> > .get_coalesce = tg3_get_coalesce,
> > .set_coalesce = tg3_set_coalesce,
> > .get_sset_count = tg3_get_sset_count,
> > + .get_rxnfc = tg3_get_rxnfc,
> > + .get_rxfh_indir = tg3_get_rxfh_indir,
> > + .set_rxfh_indir = tg3_set_rxfh_indir,
> > };
> >
> > static void __devinit tg3_get_eeprom_size(struct tg3 *tp)
> > diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
> > index aea8f72..4800595 100644
> > --- a/drivers/net/ethernet/broadcom/tg3.h
> > +++ b/drivers/net/ethernet/broadcom/tg3.h
> > @@ -2932,6 +2932,7 @@ enum TG3_FLAGS {
> > TG3_FLAG_APE_HAS_NCSI,
> > TG3_FLAG_4K_FIFO_LIMIT,
> > TG3_FLAG_RESET_TASK_PENDING,
> > + TG3_FLAG_USER_INDIR_TBL,
> > TG3_FLAG_5705_PLUS,
> > TG3_FLAG_IS_5788,
> > TG3_FLAG_5750_PLUS,
>
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
>
^ permalink raw reply
* Re: [PATCH net-next v1 5/6] tg3: implementation of a non-NAPI mode
From: Ben Hutchings @ 2011-12-16 19:30 UTC (permalink / raw)
To: David Decotigny
Cc: Matt Carlson, Michael Chan, netdev, linux-kernel,
Javier Martinez Canillas, Robin Getz, Matt Mackall, Tom Herbert
In-Reply-To: <6d55ce25f5f237a4538f6a2fcd2609b877669297.1324059527.git.decot@googlers.com>
On Fri, 2011-12-16 at 10:19 -0800, David Decotigny wrote:
> From: Tom Herbert <therbert@google.com>
>
> The tg3 NIC has a hard limit of 511 descriptors for the receive ring.
> Under heavy load of small packets, this device receive queue may not
> be serviced fast enough to prevent packets drops. This could be due
> to a variety of reasons such as lengthy processing delays of packets
> in the stack, softirqs being disabled too long, etc.
[...]
I think those are bugs to be fixed, not worked around.
Various drivers had NAPI as a compile-time option for a while, and
pretty much all of those have now been made to use NAPI unconditionally
(I think tulip is the only one left). Adding such an option back is
unlikely to be accepted now.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH net-next 1/2] sch_sfq: store flow_keys in skb->cb[]
From: Eric Dumazet @ 2011-12-16 19:31 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20111216.135420.664062202208615256.davem@davemloft.net>
Le vendredi 16 décembre 2011 à 13:54 -0500, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 16 Dec 2011 06:39:39 +0100
>
> > Instead of using automatic flow_keys variable in sfq_hash(), use storage
> > from skb->cb[] so that we can reuse it later if we want to rehash queues
> > when perturbation timer fires.
> >
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>
> Where is "2/2"?
I got distracted by other stuff today, will finish later.
You can delete this one, I'll submit a proper serie when done.
Thanks !
^ permalink raw reply
* Re: twice past the taps, thence out to net?
From: Eric Dumazet @ 2011-12-16 19:34 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: Rick Jones, Stephen Hemminger, Vijay Subramanian, tcpdump-workers,
netdev, Matthew Vick, Jeff Kirsher
In-Reply-To: <CAEuXFEy02BAL4+=RNOi7u=-4WBvwPpvTOJKPcwND2yxW+hw9FA@mail.gmail.com>
Le vendredi 16 décembre 2011 à 10:28 -0800, Jesse Brandeburg a écrit :
> On Thu, Dec 15, 2011 at 8:27 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Le jeudi 15 décembre 2011 à 14:22 -0800, Rick Jones a écrit :
> >> On 12/15/2011 11:00 AM, Eric Dumazet wrote:
> >> >> Device's work better if the driver proactively manages stop_queue/wake_queue.
> >> >> Old devices used TX_BUSY, but newer devices tend to manage the queue
> >> >> themselves.
> >> >>
> >> >
> >> > Some 'new' drivers like igb can be fooled in case skb is gso segmented ?
> >> >
> >> > Because igb_xmit_frame_ring() needs skb_shinfo(skb)->nr_frags + 4
> >> > descriptors, igb should stop its queue not at MAX_SKB_FRAGS + 4, but
> >> > MAX_SKB_FRAGS*4
>
> can you please help me understand the need for MAX_SKB_FRAGS * 4 as
> the requirement? Currently driver uses logic like
>
> in hard_start_tx: hey I just finished a tx, I should stop the qdisc if
> I don't have room (in tx descriptors) for a worst case transmit skb
> (MAX_SKB_FRAGS + 4) the next time I'm called.
> when cleaning from interrupt: My cleanup is done, do I have enough
> free tx descriptors (should be MAX_SKB_FRAGS + 4) for a worst case
> transmit? If yes, restart qdisc.
>
> I'm missing the jump from the above logic to using MAX_SKB_FRAGS * 4
> (== (18 * 4) == 72) as the minimum number of descriptors I need for a
> worst case TSO. Each descriptor can point to up to 16kB of contiguous
> memory, typically we use 1 for offload context setup, 1 for skb->data,
> and 1 for each page. I think we may be overestimating with
> MAX_SKB_FRAGS + 4, but that should be no big deal.
Did you read my second patch ?
Problem is you wakeup the queue too soon (16 available descriptors,
while a full TSO packet needs more than that)
How would you explain high 'requeues' number if it was not the problem ?
Also, its suboptimal to wakeup the queue if available space is very low,
since only _one_ packet may be dequeued from qdisc (you pay high cost in
cache line bouncing)
My first patch was about a very rare event : A full TSO packet is
segmented in gso_segment() [ say if you dynamically disable sg on eth
device and an old tcp buffer is retransmitted ] : You end with 16 skbs
delivered to NIC : In this case we can hit tx ring limit at 4th or 5th
skb, and Rick complains tcpdump outputs some packets several times ;)
Since igb needs 4 descriptors for linear skb, I said : 4 *
MAX_SKB_FRAGS, but real problem is addressed in my second patch, I
believe ?
^ permalink raw reply
* Re: twice past the taps, thence out to net?
From: Rick Jones @ 2011-12-16 19:35 UTC (permalink / raw)
To: Eric Dumazet
Cc: Stephen Hemminger, Vijay Subramanian, tcpdump-workers, netdev,
Matthew Vick, Jeff Kirsher
In-Reply-To: <1324009676.2562.9.camel@edumazet-laptop>
>> but is that getting a little close?
>>
>> rick jones
>
> Sure !
>
> I only pointed out a possible problem, and not gave a full patch, since
> we also need to change the opposite threshold (when we XON the queue at
> TX completion)
>
> You can see its not even consistent with the minimum for a single TSO
> frame ! Most probably your high requeue numbers come from this too low
> value given the real requirements of the hardware (4 + nr_frags
> descriptors per skb)
>
> /* How many Tx Descriptors do we need to call netif_wake_queue ? */
> #define IGB_TX_QUEUE_WAKE 16
>
>
> Maybe we should CC Intel guys
>
> Could you try following patch ?
I would *love* to. All my accessible igb-driven hardware is in an
environment locked to the kernels already there :( Not that it makes it
more possible for me to do it, but I suspect it does not require 30
receivers to reproduce the dups with netperf TCP_STREAM. Particularly
if the tx queue len is at 256 it may only take 6 or 8. In fact let me
try that now...
Yep, with just 8 destinations/concurrent TCP_STREAM tests from the one
system one can still see the duplicates in the packet trace taken on the
sender.
Perhaps we can trouble the Intel guys to try to reproduce what I've seen?
rick
>
> Thanks !
>
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index c69feeb..93ce118 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -51,8 +51,8 @@ struct igb_adapter;
> /* TX/RX descriptor defines */
> #define IGB_DEFAULT_TXD 256
> #define IGB_DEFAULT_TX_WORK 128
> -#define IGB_MIN_TXD 80
> -#define IGB_MAX_TXD 4096
> +#define IGB_MIN_TXD max_t(unsigned, 80U, IGB_TX_QUEUE_WAKE * 2)
> +#define IGB_MAX_TXD 4096
>
> #define IGB_DEFAULT_RXD 256
> #define IGB_MIN_RXD 80
> @@ -121,8 +121,11 @@ struct vf_data_storage {
> #define IGB_RXBUFFER_16384 16384
> #define IGB_RX_HDR_LEN IGB_RXBUFFER_512
>
> -/* How many Tx Descriptors do we need to call netif_wake_queue ? */
> -#define IGB_TX_QUEUE_WAKE 16
> +/* How many Tx Descriptors should be available
> + * before calling netif_wake_subqueue() ?
> + */
> +#define IGB_TX_QUEUE_WAKE (MAX_SKB_FRAGS * 4)
> +
> /* How many Rx Buffers do we bundle into one write to the hardware ? */
> #define IGB_RX_BUFFER_WRITE 16 /* Must be power of 2 */
>
>
>
> --
> 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 net-next v1 5/6] tg3: implementation of a non-NAPI mode
From: Eric Dumazet @ 2011-12-16 19:42 UTC (permalink / raw)
To: David Decotigny
Cc: Matt Carlson, Michael Chan, netdev, linux-kernel,
Javier Martinez Canillas, Robin Getz, Matt Mackall, Tom Herbert
In-Reply-To: <6d55ce25f5f237a4538f6a2fcd2609b877669297.1324059527.git.decot@googlers.com>
Le vendredi 16 décembre 2011 à 10:19 -0800, David Decotigny a écrit :
> From: Tom Herbert <therbert@google.com>
>
...
>
>
> Signed-off-by: David Decotigny <decot@googlers.com>
> ---
If patch author is Tom Herbert, you should use :
Signed-off-by: Tom Herbert <therbert@google.com>
Signed-off-by: David Decotigny <decot@googlers.com>
(Same problem on other patches)
^ permalink raw reply
* Re: [PATCH] tg3: Make the RSS indir tbl admin configurable
From: Ben Hutchings @ 2011-12-16 19:43 UTC (permalink / raw)
To: Matt Carlson; +Cc: davem@davemloft.net, netdev@vger.kernel.org, Michael Chan
In-Reply-To: <20111216191927.GA9054@mcarlson.broadcom.com>
On Fri, 2011-12-16 at 11:19 -0800, Matt Carlson wrote:
> On Thu, Dec 15, 2011 at 07:04:34PM -0800, Ben Hutchings wrote:
> > On Thu, 2011-12-15 at 16:47 -0800, Matt Carlson wrote:
> > > This patch adds the ethtool callbacks necessary to change the rss
> > > indirection table from userspace. When setting the indirection table
> > > through set_rxfh_indir, an indirection table size of zero is
> > > interpreted to mean that the admin wants to relinquish control of the
> > > table to the driver. Should the number of interrupts change (e.g.
> > > across a close / open call, or through a reset), any indirection table
> > > values that exceed the number of RSS queues or interrupt vectors will
> > > be automatically scaled back to values within range.
> > >
> > > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> > > Signed-off-by: Michael Chan <mchan@broadcom.com>
> > > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> >
> > No I didn't!
> >
> > Ben.
>
> Sorry. This part of procedure I'm unsure about. I didn't want to lose
> the ack of your contributions to this patch. The SOB does imply that
> you are O.K. with it as-is though, which you may or may not be aligned
> with. I apologize if I was putting words in your mouth.
>
> What should I have done instead? Would Dave put the SOB on your behalf
> as he accepts it?
[...]
If you really want to credit me for the tiny change I suggested, just
put that in free-form. But I'm afraid you'll have to respin this
completely, as David applied my changes to centralise validation and the
default policy. I think instead of adding the USER_INDIR_TBL flag you
could reasonably reset the table to default whenever the interface is
brought up and you fail to get the wanted number of IRQs.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
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