Re: [PATCH 2.6.24 5/5]S2io: Optimize isr fast path
From: Jeff Garzik <hidden>
Date: 2007-08-31 13:05:42
Ramkrishna Vepa wrote:
- Optimized interrupt routine fast path. Signed-off-by: Sivakumar Subramani <redacted> Signed-off-by: Santosh Rastapur <redacted> Signed-off-by: Ramkrishna Vepa <redacted>
patch description is completely inadequate. how was it optimized? what are the gains / why is this patch worth having?
quoted hunk ↗ jump to hunk
diff -Nurp patch4/drivers/net/s2io.c patch5/drivers/net/s2io.c--- patch4/drivers/net/s2io.c 2007-08-15 08:42:14.000000000 -0700 +++ patch5/drivers/net/s2io.c 2007-08-15 08:42:51.000000000 -0700@@ -84,7 +84,7 @@ #include "s2io.h" #include "s2io-regs.h" -#define DRV_VERSION "2.0.26.1" +#define DRV_VERSION "2.0.26.2" /* S2io Driver name & version. */ static char s2io_driver_name[] = "Neterion";@@ -4917,71 +4917,76 @@ static irqreturn_t s2io_isr(int irq, voi * 1. Rx of packet. * 2. Tx complete. * 3. Link down. - * 4. Error in any functional blocks of the NIC. */ reason = readq(&bar0->general_int_status); - if (!reason) { - /* The interrupt was not raised by us. */ + if (unlikely(reason == S2IO_MINUS_ONE) ) { + /* Nothing much can be done. Get out */ atomic_dec(&sp->isr_cnt); - return IRQ_NONE; - } - else if (unlikely(reason == S2IO_MINUS_ONE) ) { - /* Disable device and get out */ - atomic_dec(&sp->isr_cnt); - return IRQ_NONE; + return IRQ_HANDLED; } - if (napi) { - if (reason & GEN_INTR_RXTRAFFIC) { - if ( likely ( netif_rx_schedule_prep(dev)) ) { - __netif_rx_schedule(dev); - writeq(S2IO_MINUS_ONE, &bar0->rx_traffic_mask); + if (reason & (GEN_INTR_RXTRAFFIC | + GEN_INTR_TXTRAFFIC | GEN_INTR_TXPIC)) + { + writeq(S2IO_MINUS_ONE, &bar0->general_int_mask); + + if (config->napi) { + if (reason & GEN_INTR_RXTRAFFIC) { + if ( likely (netif_rx_schedule_prep(dev)) ) { + __netif_rx_schedule(dev); + writeq(S2IO_MINUS_ONE, + &bar0->rx_traffic_mask); + } else + writeq(S2IO_MINUS_ONE, + &bar0->rx_traffic_int); } - else + } else { + /* + * rx_traffic_int reg is an R1 register, writing all 1's + * will ensure that the actual interrupt causing bit + * get's cleared and hence a read can be avoided. + */ + if (reason & GEN_INTR_RXTRAFFIC) writeq(S2IO_MINUS_ONE, &bar0->rx_traffic_int); + + for (i = 0; i < config->rx_ring_num; i++) + rx_intr_handler(&mac_control->rings[i]); } - } else { + /* - * Rx handler is called by default, without checking for the - * cause of interrupt. - * rx_traffic_int reg is an R1 register, writing all 1's + * tx_traffic_int reg is an R1 register, writing all 1's * will ensure that the actual interrupt causing bit get's * cleared and hence a read can be avoided. */ - if (reason & GEN_INTR_RXTRAFFIC) - writeq(S2IO_MINUS_ONE, &bar0->rx_traffic_int); + if (reason & GEN_INTR_TXTRAFFIC) + writeq(S2IO_MINUS_ONE, &bar0->tx_traffic_int); - for (i = 0; i < config->rx_ring_num; i++) { - rx_intr_handler(&mac_control->rings[i]); - } - } + for (i = 0; i < config->tx_fifo_num; i++) + tx_intr_handler(&mac_control->fifos[i]); - /* - * tx_traffic_int reg is an R1 register, writing all 1's - * will ensure that the actual interrupt causing bit get's - * cleared and hence a read can be avoided. - */ - if (reason & GEN_INTR_TXTRAFFIC) - writeq(S2IO_MINUS_ONE, &bar0->tx_traffic_int); + if (reason & GEN_INTR_TXPIC) + s2io_txpic_intr_handle(sp); - for (i = 0; i < config->tx_fifo_num; i++) - tx_intr_handler(&mac_control->fifos[i]); + /* + * Reallocate the buffers from the interrupt handler itself. + */ + if (!config->napi) { + for (i = 0; i < config->rx_ring_num; i++) + s2io_chk_rx_buffers(sp, i); + } + writeq(sp->general_int_mask, &bar0->general_int_mask); + readl(&bar0->general_int_status); - if (reason & GEN_INTR_TXPIC) - s2io_txpic_intr_handle(sp); - /* - * If the Rx buffer count is below the panic threshold then - * reallocate the buffers from the interrupt handler itself, - * else schedule a tasklet to reallocate the buffers. - */ - if (!napi) { - for (i = 0; i < config->rx_ring_num; i++) - s2io_chk_rx_buffers(sp, i); - } + atomic_dec(&sp->isr_cnt); + return IRQ_HANDLED; - writeq(0, &bar0->general_int_mask); - readl(&bar0->general_int_status); + } + else if (!reason) { + /* The interrupt was not raised by us */ + atomic_dec(&sp->isr_cnt); + return IRQ_NONE; + } atomic_dec(&sp->isr_cnt); return IRQ_HANDLED;@@ -7109,6 +7114,14 @@ static void s2io_rem_isr(struct s2io_nic struct net_device *dev = sp->dev; struct swStat *stats = &sp->mac_control.stats_info->sw_stat; + /* Waiting till all Interrupt handlers are complete */ + do { + if (!atomic_read(&sp->isr_cnt)) + break; + msleep(10); + cnt++; + } while(cnt < 5); + if (sp->config.intr_type == MSI_X) { int i; u16 msi_control;@@ -7138,14 +7151,6 @@ static void s2io_rem_isr(struct s2io_nic } else { free_irq(sp->pdev->irq, dev); } - /* Waiting till all Interrupt handlers are complete */ - cnt = 0; - do { - msleep(10); - if (!atomic_read(&sp->isr_cnt)) - break; - cnt++; - } while(cnt < 5); }
this is bogus code -- you should use synchronize_irq() and other standard kernel functions, rather than duplicating all that state in the driver-private structs