2015-11-09 23:51:17

by Benjamin Poirier

[permalink] [raw]
Subject: [PATCH v3 0/4] e1000e msi-x fixes

Hi,

For this series:


Benjamin Poirier (4):
e1000e: Remove unreachable code
e1000e: Do not read icr in Other interrupt
e1000e: Do not write lsc to ics in msi-x mode
e1000e: Fix msi-x interrupt automask

drivers/net/ethernet/intel/e1000e/defines.h | 3 +-
drivers/net/ethernet/intel/e1000e/netdev.c | 66 ++++++++++++-----------------
2 files changed, 30 insertions(+), 39 deletions(-)

Changes in v3:
Preserve LSC in IMS, LSC events are not delivered otherwise.
Disable CTRL_EXT.IAME to prevent IMC write on ICR read from external
program.

Changes in v2:
Address review comments from Alexander Duyck: extend cleanup of Other
interrupt handler and use tx_ring->ims_val.


The first three patches cleanup handling of Other interrupts and the
last patch fixes tx and rx interrupts. Please consider reading the
description for that patch before proceeding. I believe that the
following simple tracing statements are helpful in detecting the problem
fixed by the last patch.

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index a09d1e4..29b8c6e 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -1942,6 +1942,9 @@ static irqreturn_t e1000_intr_msix_rx(int __always_unused irq, void *data)
struct net_device *netdev = data;
struct e1000_adapter *adapter = netdev_priv(netdev);
struct e1000_ring *rx_ring = adapter->rx_ring;
+ struct e1000_hw *hw = &adapter->hw;
+
+ trace_printk("%s: rxq0 irq ims 0x%08x\n", netdev->name, er32(IMS));

/* Write the ITR value calculated at the end of the
* previous interrupt.
@@ -1956,6 +1959,7 @@ static irqreturn_t e1000_intr_msix_rx(int __always_unused irq, void *data)
adapter->total_rx_bytes = 0;
adapter->total_rx_packets = 0;
__napi_schedule(&adapter->napi);
+ trace_printk("%s: scheduling napi\n", netdev->name);
}
return IRQ_HANDLED;
}
@@ -2663,6 +2667,8 @@ static int e1000e_poll(struct napi_struct *napi, int weight)
struct net_device *poll_dev = adapter->netdev;
int tx_cleaned = 1, work_done = 0;

+ trace_printk("%s: poll starting ims 0x%08x\n", poll_dev->name,
+ er32(IMS));
adapter = netdev_priv(poll_dev);

if (!adapter->msix_entries ||
@@ -2680,6 +2686,8 @@ static int e1000e_poll(struct napi_struct *napi, int weight)
e1000_set_itr(adapter);
napi_complete_done(napi, work_done);
if (!test_bit(__E1000_DOWN, &adapter->state)) {
+ trace_printk("%s: will enable rxq0 irq\n",
+ poll_dev->name);
if (adapter->msix_entries)
ew32(IMS, adapter->rx_ring->ims_val);
else

-------- 8< --------

With that patch but without the patches in this series we can see that rx irqs
occur at unexpected times:

<idle>-0 [000] .Ns. 1986.887517: e1000e_poll: eth1: will enable rxq0 irq
<idle>-0 [000] d.h. 1986.896654: e1000_intr_msix_rx: eth1: rxq0 irq ims 0x01500004
<idle>-0 [000] d.h. 1986.896657: e1000_intr_msix_rx: eth1: scheduling napi
<idle>-0 [000] d.H. 1986.896662: e1000_intr_msix_rx: eth1: rxq0 irq ims 0x01500004
<idle>-0 [000] ..s. 1986.896667: e1000e_poll: eth1: poll starting ims 0x01500004
Warning: many interrupts (2) before napi
<idle>-0 [000] ..s. 1986.896685: e1000e_poll: eth1: will enable rxq0 irq

<idle>-0 [000] d.h. 1990.688870: e1000_intr_msix_rx: eth1: scheduling napi
<idle>-0 [000] ..s. 1990.688875: e1000e_poll: eth1: poll starting ims 0x01500004
<idle>-0 [000] dNH. 1990.688913: e1000_intr_msix_rx: eth1: rxq0 irq ims 0x01500004
Warning: interrupt inside napi
<idle>-0 [000] .Ns. 1990.688916: e1000e_poll: eth1: will enable rxq0 irq
<idle>-0 [000] d.h. 1990.729688: e1000_intr_msix_rx: eth1: rxq0 irq ims 0x01500004

Here's a typical sequence after applying the patches in this series. Notice
that ims is changed. Another printk at the end of e1000e_poll would show it to
be 0x01500004.

<idle>-0 [000] d.h. 23547.977917: e1000_intr_msix_rx: eth1: rxq0 irq ims 0x01400004
<idle>-0 [000] d.h. 23547.977922: e1000_intr_msix_rx: eth1: scheduling napi
<idle>-0 [000] ..s. 23547.977928: e1000e_poll: eth1: poll starting ims 0x01400004
<idle>-0 [000] ..s. 23547.977961: e1000e_poll: eth1: will enable rxq0 irq

Finally, here's the script I used to generate the warnings above:

#!/usr/bin/python3

import sys
import re
import pprint


class NaE(Exception):
"Not an Event"
pass

class Event:
def __init__(self, line):
# sample events:
# <idle>-0 [000] d.h. 2025.256536: e1000_intr_msix_rx: eth1: rxq0 irq ims 0x01500004
# <idle>-0 [000] d.h. 2025.256539: e1000_intr_msix_rx: eth1: scheduling napi
# <idle>-0 [000] ..s. 2025.256544: e1000e_poll: eth1: poll starting ims 0x01500004
# <idle>-0 [000] ..s. 2025.256558: e1000e_poll: eth1: will enable rxq0 irq
retval = re.match(" +<?(?P<comm>.*)>?-(?P<pid>[0-9]+) +\[(?P<cpu>.*)\] (?P<flags>[^ ]+) +(?P<time>[0-9.]+): (?P<funcname>[^:]+): (?P<ifname>[^:]+): (?P<args>.*)", line)
if retval:
self.comm = retval.group("comm")
self.pid = retval.group("pid")
self.cpu = retval.group("cpu")
self.flags = retval.group("flags")
self.time = retval.group("time")
self.funcname = retval.group("funcname")
self.ifname = retval.group("ifname")
self.args = retval.group("args")
else:
raise NaE


class Machine:
pass

class State:
def __init__(self, machine):
self.machine = machine

def entry(self, event):
pass

def transition(self, event):
"self.machine should be considered read-only in transition"
return State(self.machine)

def run(self, event):
pass

def exit(self, event):
pass

def advance(self, event):
nextState = self.transition(event)
if (nextState != self):
self.exit(event)
nextState.entry(event)
nextState.run(event)
return nextState

# general receive processing machine
def enteringNapi(machine, event):
if event.args.startswith("poll starting"):
return True
else:
return False

def exitingNapi(machine, event):
if event.args.startswith("will enable"):
return True
else:
return False

class OutsideNapi(State):
def entry(self, event):
self.machine.intr = []

def transition(self, event):
if enteringNapi(self.machine, event):
return InsideNapi(self.machine)
else:
return self

def run(self, event):
if event.args.startswith("rxq0 irq"):
self.machine.intr.append(event)

def exit(self, event):
if len(self.machine.intr) > 1:
print("Warning: many interrupts (%d) before napi at %s" % (
len(self.machine.intr), event.time,))

class InsideNapi(State):
def transition(self, event):
if exitingNapi(self.machine, event):
return OutsideNapi(self.machine)
else:
return self

def run(self, event):
if event.args.startswith("rxq0 irq"):
print("Warning: interrupt inside napi")

class UnknownState(State):
def transition(self, event):
if enteringNapi(self.machine, event):
return InsideNapi(self.machine)
elif exitingNapi(self.machine, event):
return ExitingNapi(self.machine)
else:
return self


if __name__ == '__main__':
debug = False

state = UnknownState(Machine())

for line in sys.stdin:
print(line, end='')
try:
event = Event(line)
except NaE:
pass
else:
if debug:
pprinter.pprint(event)
state = state.advance(event)

--
2.6.2


2015-11-09 23:51:20

by Benjamin Poirier

[permalink] [raw]
Subject: [PATCH v3 1/4] e1000e: Remove unreachable code

msi-x interrupts are not shared so there's no need to check if the
interrupt was really from this adapter.

Signed-off-by: Benjamin Poirier <[email protected]>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 6 ------
1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 0a854a4..a228167 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -1907,12 +1907,6 @@ static irqreturn_t e1000_msix_other(int __always_unused irq, void *data)
struct e1000_hw *hw = &adapter->hw;
u32 icr = er32(ICR);

- if (!(icr & E1000_ICR_INT_ASSERTED)) {
- if (!test_bit(__E1000_DOWN, &adapter->state))
- ew32(IMS, E1000_IMS_OTHER);
- return IRQ_NONE;
- }
-
if (icr & adapter->eiac_mask)
ew32(ICS, (icr & adapter->eiac_mask));

--
2.6.2

2015-11-09 23:51:25

by Benjamin Poirier

[permalink] [raw]
Subject: [PATCH v3 2/4] e1000e: Do not read icr in Other interrupt

removes the icr read in the other interrupt handler, uses eiac to
autoclear the Other bit from icr and ims. This allows us to avoid
interference with rx and tx interrupts in the Other interrupt handler.

The information read from icr is not needed. IMS is configured such that
the only interrupt cause that can trigger the Other interrupt is Link
Status Change.

Signed-off-by: Benjamin Poirier <[email protected]>

---
I noticed a 8-16% improvement in netperf rr tests after applying this
patch. This is a little surprising since this patch touches the handling
of Other interrupts, which do not occur during such a test. Some
profiling was not very insightful but the improvement seems related to
writing Other to EIAC.
---
drivers/net/ethernet/intel/e1000e/netdev.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index a228167..a73e323 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -1905,24 +1905,15 @@ static irqreturn_t e1000_msix_other(int __always_unused irq, void *data)
struct net_device *netdev = data;
struct e1000_adapter *adapter = netdev_priv(netdev);
struct e1000_hw *hw = &adapter->hw;
- u32 icr = er32(ICR);

- if (icr & adapter->eiac_mask)
- ew32(ICS, (icr & adapter->eiac_mask));
+ hw->mac.get_link_status = true;

- if (icr & E1000_ICR_OTHER) {
- if (!(icr & E1000_ICR_LSC))
- goto no_link_interrupt;
- hw->mac.get_link_status = true;
- /* guard against interrupt when we're going down */
- if (!test_bit(__E1000_DOWN, &adapter->state))
- mod_timer(&adapter->watchdog_timer, jiffies + 1);
+ /* guard against interrupt when we're going down */
+ if (!test_bit(__E1000_DOWN, &adapter->state)) {
+ mod_timer(&adapter->watchdog_timer, jiffies + 1);
+ ew32(IMS, E1000_IMS_OTHER);
}

-no_link_interrupt:
- if (!test_bit(__E1000_DOWN, &adapter->state))
- ew32(IMS, E1000_IMS_LSC | E1000_IMS_OTHER);
-
return IRQ_HANDLED;
}

@@ -2019,6 +2010,7 @@ static void e1000_configure_msix(struct e1000_adapter *adapter)
hw->hw_addr + E1000_EITR_82574(vector));
else
writel(1, hw->hw_addr + E1000_EITR_82574(vector));
+ adapter->eiac_mask |= E1000_IMS_OTHER;

/* Cause Tx interrupts on every write back */
ivar |= (1 << 31);
@@ -2247,7 +2239,7 @@ static void e1000_irq_enable(struct e1000_adapter *adapter)

if (adapter->msix_entries) {
ew32(EIAC_82574, adapter->eiac_mask & E1000_EIAC_MASK_82574);
- ew32(IMS, adapter->eiac_mask | E1000_IMS_OTHER | E1000_IMS_LSC);
+ ew32(IMS, adapter->eiac_mask | E1000_IMS_LSC);
} else if ((hw->mac.type == e1000_pch_lpt) ||
(hw->mac.type == e1000_pch_spt)) {
ew32(IMS, IMS_ENABLE_MASK | E1000_IMS_ECCER);
--
2.6.2

2015-11-09 23:51:30

by Benjamin Poirier

[permalink] [raw]
Subject: [PATCH v3 3/4] e1000e: Do not write lsc to ics in msi-x mode

In msi-x mode, there is no handler for the lsc interrupt so there is no
point in writing that to ics now that we always assume Other interrupts
are caused by lsc.

Reviewed-by: Jasna Hodzic <[email protected]>
Signed-off-by: Benjamin Poirier <[email protected]>
---
drivers/net/ethernet/intel/e1000e/defines.h | 3 ++-
drivers/net/ethernet/intel/e1000e/netdev.c | 27 ++++++++++++++++-----------
2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h
index 133d407..f7c7804 100644
--- a/drivers/net/ethernet/intel/e1000e/defines.h
+++ b/drivers/net/ethernet/intel/e1000e/defines.h
@@ -441,12 +441,13 @@
#define E1000_IMS_RXQ1 E1000_ICR_RXQ1 /* Rx Queue 1 Interrupt */
#define E1000_IMS_TXQ0 E1000_ICR_TXQ0 /* Tx Queue 0 Interrupt */
#define E1000_IMS_TXQ1 E1000_ICR_TXQ1 /* Tx Queue 1 Interrupt */
-#define E1000_IMS_OTHER E1000_ICR_OTHER /* Other Interrupts */
+#define E1000_IMS_OTHER E1000_ICR_OTHER /* Other Interrupt */

/* Interrupt Cause Set */
#define E1000_ICS_LSC E1000_ICR_LSC /* Link Status Change */
#define E1000_ICS_RXSEQ E1000_ICR_RXSEQ /* Rx sequence error */
#define E1000_ICS_RXDMT0 E1000_ICR_RXDMT0 /* Rx desc min. threshold */
+#define E1000_ICS_OTHER E1000_ICR_OTHER /* Other Interrupt */

/* Transmit Descriptor Control */
#define E1000_TXDCTL_PTHRESH 0x0000003F /* TXDCTL Prefetch Threshold */
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index a73e323..ed7cc8e 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4130,10 +4130,23 @@ void e1000e_reset(struct e1000_adapter *adapter)

}

-int e1000e_up(struct e1000_adapter *adapter)
+/**
+ * e1000e_trigger_lsc - trigger an lsc interrupt
+ *
+ * Fire a link status change interrupt to start the watchdog.
+ **/
+static void e1000e_trigger_lsc(struct e1000_adapter *adapter)
{
struct e1000_hw *hw = &adapter->hw;

+ if (adapter->msix_entries)
+ ew32(ICS, E1000_ICS_OTHER);
+ else
+ ew32(ICS, E1000_ICS_LSC);
+}
+
+int e1000e_up(struct e1000_adapter *adapter)
+{
/* hardware has been reset, we need to reload some things */
e1000_configure(adapter);

@@ -4145,11 +4158,7 @@ int e1000e_up(struct e1000_adapter *adapter)

netif_start_queue(adapter->netdev);

- /* fire a link change interrupt to start the watchdog */
- if (adapter->msix_entries)
- ew32(ICS, E1000_ICS_LSC | E1000_ICR_OTHER);
- else
- ew32(ICS, E1000_ICS_LSC);
+ e1000e_trigger_lsc(adapter);

return 0;
}
@@ -4576,11 +4585,7 @@ static int e1000_open(struct net_device *netdev)
hw->mac.get_link_status = true;
pm_runtime_put(&pdev->dev);

- /* fire a link status change interrupt to start the watchdog */
- if (adapter->msix_entries)
- ew32(ICS, E1000_ICS_LSC | E1000_ICR_OTHER);
- else
- ew32(ICS, E1000_ICS_LSC);
+ e1000e_trigger_lsc(adapter);

return 0;

--
2.6.2

2015-11-09 23:51:34

by Benjamin Poirier

[permalink] [raw]
Subject: [PATCH v3 4/4] e1000e: Fix msi-x interrupt automask

Since the introduction of 82574 support in e1000e, the driver has worked
on the assumption that msi-x interrupt generation is automatically
disabled after each irq. As it turns out, this is not the case.
Currently, rx interrupts can fire multiple times before and during napi
processing. This can be a problem for users because frames that arrive
in a certain window (after adapter->clean_rx() but before
napi_complete_done() has cleared NAPI_STATE_SCHED) generate an interrupt
which does not lead to napi_schedule(). These frames sit in the rx queue
until another frame arrives (a tcp retransmit for example).

While the EIAC and CTRL_EXT registers are properly configured for irq
automask, the modification of IAM in e1000_configure_msix() is what
prevents automask from working as intended.

This patch removes that erroneous write and fixes interrupt rearming for
tx interrupts. It also clears IAME from CTRL_EXT. This is not strictly
necessary for operation of the driver but it is to avoid disruption from
potential programs that access the registers directly, like `ethregs -c`.

Reported-by: Frank Steiner <[email protected]>
Signed-off-by: Benjamin Poirier <[email protected]>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index ed7cc8e..2a22ed7 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -1931,6 +1931,9 @@ static irqreturn_t e1000_intr_msix_tx(int __always_unused irq, void *data)
/* Ring was not completely cleaned, so fire another interrupt */
ew32(ICS, tx_ring->ims_val);

+ if (!test_bit(__E1000_DOWN, &adapter->state))
+ ew32(IMS, adapter->tx_ring->ims_val);
+
return IRQ_HANDLED;
}

@@ -2018,12 +2021,8 @@ static void e1000_configure_msix(struct e1000_adapter *adapter)
ew32(IVAR, ivar);

/* enable MSI-X PBA support */
- ctrl_ext = er32(CTRL_EXT);
- ctrl_ext |= E1000_CTRL_EXT_PBA_CLR;
-
- /* Auto-Mask Other interrupts upon ICR read */
- ew32(IAM, ~E1000_EIAC_MASK_82574 | E1000_IMS_OTHER);
- ctrl_ext |= E1000_CTRL_EXT_EIAME;
+ ctrl_ext = er32(CTRL_EXT) & ~E1000_CTRL_EXT_IAME;
+ ctrl_ext |= E1000_CTRL_EXT_PBA_CLR | E1000_CTRL_EXT_EIAME;
ew32(CTRL_EXT, ctrl_ext);
e1e_flush();
}
--
2.6.2