fm10k: Corrected an error in Tx statistics

The function collecting Tx statistics was actually using values from the RX
ring. Thus, Tx and Rx statistics values reported by "ifconfig" will return identical values. This change corrects this error and the Tx statistics is now reading from the Tx ring.

Signed-off-by Jeff Kirsher <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

The function collecting Tx statistics was actually using values from the RX ring. Thus, Tx and Rx statistics values reported by "ifconfig" will return identical values. This change corrects this error and the Tx statistics is now reading from the Tx ring.

Signed-off-by Jeff Kirsher <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Output of ethtool was reporting 2 rx_errors entries. This change removes one of the redundant entries.

Signed-off-by Jeff Kirsher <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k.h         | 3 +--
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 3 +--
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c     | 5 ++---
 3 files changed, 4 insertions(+), 7 deletions(-)

Output of ethtool was reporting 2 rx_errors entries. This change removes one of the redundant entries.

Signed-off-by Jeff Kirsher <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k.h         | 3 +--
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 3 +--
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c     | 5 ++---
 3 files changed, 4 insertions(+), 7 deletions(-)

Corrected a spelling mistake that was found over time.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Ngai-Mint Kwan <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_mbx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Corrected a spelling mistake that was found over time.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Ngai-Mint Kwan <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_mbx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Currently, the VFs do not read the default VLAN during initialization, so they will not be able to indicate untagged frames properly.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_vf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Currently, the VFs do not read the default VLAN during initialization, so they will not be able to indicate untagged frames properly.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_vf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

This change adds a function called "fm10k_netpoll" that's used to define
"ndo_poll_controller" in "fm10k_netdev_ops". This is required to enable
support for "netconsole" in fm10k.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Ngai-Mint Kwan <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k.h        |  1 +
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 21
+++++++++++++++++++++
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c    |  2 +-
 3 files changed, 23 insertions(+), 1 deletion(-)

This change adds a function called "fm10k_netpoll" that's used to define "ndo_poll_controller" in "fm10k_netdev_ops". This is required to enable support for "netconsole" in fm10k.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Ngai-Mint Kwan <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k.h        |  1 +
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 21
+++++++++++++++++++++
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c    |  2 +-
 3 files changed, 23 insertions(+), 1 deletion(-)

This change adds a function called "fm10k_netpoll" that's used to define "ndo_poll_controller" in "fm10k_netdev_ops". This is required to enable support for "netconsole" in fm10k.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Ngai-Mint Kwan <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
  drivers/net/ethernet/intel/fm10k/fm10k.h        |  1 +
  drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 21 +++++++++++++++++++++
  drivers/net/ethernet/intel/fm10k/fm10k_pci.c    |  2 +-
  3 files changed, 23 insertions(+), 1 deletion(-)

drivers/net/ethernet/intel/fm10k/fm10k.h +1 -0

index 65e7001..715d0a0 100644
@@ -457,6 +457,7 @@ void fm10k_down(struct fm10k_intfc *interface);
  void fm10k_update_stats(struct fm10k_intfc *interface);
  void fm10k_service_event_schedule(struct fm10k_intfc *interface);
  void fm10k_update_rx_drop_en(struct fm10k_intfc *interface);
+irqreturn_t fm10k_msix_clean_rings(int irq, void *data);
 
  /* Netdev */
  struct net_device *fm10k_alloc_netdev(void);

drivers/net/ethernet/intel/fm10k/fm10k_netdev.c +21 -0

index a7db5e2..2a61b83 100644
@@ -1350,6 +1350,26 @@ static void fm10k_dfwd_del_station(struct net_device *dev, void *priv)
  	}
  }
 
+/**
+ * fm10k_netpoll - A Polling 'interrupt' handler
+ * @netdev: network interface device structure
+ *
+ * This is used by netconsole to send skbs without having to re-enable
+ * interrupts. It's not called while the normal interrupt routine is executing.
+ **/
+static void fm10k_netpoll(struct net_device *netdev)
+{
+	struct fm10k_intfc *interface = netdev_priv(netdev);
+	int i;
+
+	/* if interface is down do nothing */
+	if (test_bit(__FM10K_DOWN, &interface->state))
+		return;
+
+	for (i = 0; i < interface->num_q_vectors; i++)
+		fm10k_msix_clean_rings(0, interface->q_vector[i]);
+}
+
  static netdev_features_t fm10k_features_check(struct sk_buff *skb,
  					      struct net_device *dev,
  					      netdev_features_t features)

@@ -1382,6 +1402,7 @@ static const struct net_device_ops fm10k_netdev_ops = {
  	.ndo_do_ioctl		= fm10k_ioctl,
  	.ndo_dfwd_add_station	= fm10k_dfwd_add_station,
  	.ndo_dfwd_del_station	= fm10k_dfwd_del_station,
+	.ndo_poll_controller	= fm10k_netpoll,
  	.ndo_features_check	= fm10k_features_check,
  };
 

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +0 -0

index 6fc9965..c11e2c9 100644

The were several functions which had parameters which were never or sometimes used in functions. To resolve possible compiler warnings, use __always_unused or __maybe_unused kernel macros to resolve.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_dcbnl.c   |  6 +++---
 drivers/net/ethernet/intel/fm10k/fm10k_debugfs.c |  8 +++++---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 12 ++++++------
 drivers/net/ethernet/intel/fm10k/fm10k_iov.c     |  6 +++---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c    | 18
++++++------------
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c     | 17
+++++++++--------
 drivers/net/ethernet/intel/fm10k/fm10k_ptp.c     |  5 +++--
 7 files changed, 35 insertions(+), 37 deletions(-)

The were several functions which had parameters which were never or sometimes used in functions. To resolve possible compiler warnings, use __always_unused or __maybe_unused kernel macros to resolve.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_dcbnl.c   |  6 +++---
 drivers/net/ethernet/intel/fm10k/fm10k_debugfs.c |  8 +++++---  
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 12 ++++++------
 drivers/net/ethernet/intel/fm10k/fm10k_iov.c     |  6 +++---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c    | 18
++++++------------
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c     | 17
+++++++++--------
 drivers/net/ethernet/intel/fm10k/fm10k_ptp.c     |  5 +++--
 7 files changed, 35 insertions(+), 37 deletions(-)

Previously, the user was not allowed to create a VLAN interface on top of the switch default vid.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 4 ----
 1 file changed, 4 deletions(-)

Previously, the user was not allowed to create a VLAN interface on top of the switch default vid.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 4 ----
 1 file changed, 4 deletions(-)

Previously, the user was not allowed to create a VLAN interface on top of the switch default vid.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/net/ethernet/intel/fm10k/fm10k_netdev.c +0 -0

index 2a61b83..c9f87bb 100644

Currently, we show statistics for all 128 queues, even though we don't necessarily have that many queues available especially in the VF case. Instead, use the hw->mac.max_queues value, which tells us how many queues we actually have, rather than the space for the rings we allocated. In this way, we prevent dumping statistics that are useless on the VF.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Currently, we show statistics for all 128 queues, even though we don't necessarily have that many queues available especially in the VF case. Instead, use the hw->mac.max_queues value, which tells us how many queues we actually have, rather than the space for the rings we allocated. In this way, we prevent dumping statistics that are useless on the VF.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Even though it shouldn't strictly matter, don't count queue stats higher
than the max_queues value stored for this mac. This ensures that we don't attempt to check queues which don't belong to use in VFs. This shouldn't be a visible change, as the VFs should see zero for queues which don't belong to them.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 18
+++++++++++-------
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c     |  2 +-
 2 files changed, 12 insertions(+), 8 deletions(-)

Even though it shouldn't strictly matter, don't count queue stats higher than the max_queues value stored for this mac. This ensures that we don't attempt to check queues which don't belong to use in VFs. This shouldn't be a visible change, as the VFs should see zero for queues which don't belong to them.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 18
+++++++++++-------
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c     |  2 +-
 2 files changed, 12 insertions(+), 8 deletions(-)

This patch resolves an issue with ethtool stats displaying useless values on the VF, because some stats simply have no meaning to the VF. Resolve this by splitting these out into PF_STATS and only showing them
if we aren't the VF.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 51
+++++++++++++++++-------
 1 file changed, 37 insertions(+), 14 deletions(-)

This patch resolves an issue with ethtool stats displaying useless values on the VF, because some stats simply have no meaning to the VF. Resolve this by splitting these out into PF_STATS and only showing them if we aren't the VF.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 51
+++++++++++++++++-------
 1 file changed, 37 insertions(+), 14 deletions(-)

Since we already print this message when a reset is requested via the RESET_REQUESTED flag, we do not need to print it before setting the flag.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 1 -
 1 file changed, 1 deletion(-)

Since we already print this message when a reset is requested via the RESET_REQUESTED flag, we do not need to print it before setting the flag.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 1 -
 1 file changed, 1 deletion(-)

We were incrementing the tx_timeout_count for both the Tx hang and then for all reset flows. Instead, we should only increment tx_timeout_count in the Tx hang path, so that our Tx hang counter does not increment when it was not caused by a Tx hang.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 1 -
 1 file changed, 1 deletion(-)

We were incrementing the tx_timeout_count for both the Tx hang and then for all reset flows. Instead, we should only increment tx_timeout_count in the Tx hang path, so that our Tx hang counter does not increment when it was not caused by a Tx hang.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 1 -
 1 file changed, 1 deletion(-)

Named it tx_hang_count to differentiate it from tx_hwtstamp_timeout.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 2 ++
 1 file changed, 2 insertions(+)

Named it tx_hang_count to differentiate it from tx_hwtstamp_timeout.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 2 ++
 1 file changed, 2 insertions(+)

When returning virtualization queues from the VF back to the PF, do not
retain the VF rate limiter.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Todd Russell <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

When returning virtualization queues from the VF back to the PF, do not retain the VF rate limiter.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Todd Russell <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Since we run the watchdog periodically, which might take a while and potentially monopolize the system default workqueue, create our own separate work queue. This also helps reduce and stabilize latency between scheduling the work in our interrupt and actually performing the work. Still use a timer for the regular scheduled interval but queue the work onto its own work queue.

It seemed overkill to create a single workqueue per interface, so we just spawn a single work queue for all interfaces upon driver load. For
this reason, use a multi-threaded workqueue with one thread per processor, rather than single threaded queue.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k.h      |  3 +++
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 12 ++++++++++++
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c  |  2 +-
 3 files changed, 16 insertions(+), 1 deletion(-)

Since we run the watchdog periodically, which might take a while and potentially monopolize the system default workqueue, create our own separate work queue. This also helps reduce and stabilize latency between scheduling the work in our interrupt and actually performing the work. Still use a timer for the regular scheduled interval but queue the work onto its own work queue.

It seemed overkill to create a single workqueue per interface, so we just spawn a single work queue for all interfaces upon driver load. For
this reason, use a multi-threaded workqueue with one thread per processor, rather than single threaded queue.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k.h      |  3 +++
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 12 ++++++++++++  
drivers/net/ethernet/intel/fm10k/fm10k_pci.c  |  2 +-
 3 files changed, 16 insertions(+), 1 deletion(-)

Since we already schedule the service task, we can just wait for this task to handle the mailbox events from the VF. This reduces some complex
code flow, and makes it so we have a single path for handling the VF messages. There is a possibility that we have a slight delay in handling
VF messages, but it should be minimal.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_iov.c | 30
++--------------------------
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c |  3 +++
 2 files changed, 5 insertions(+), 28 deletions(-)

Since we already schedule the service task, we can just wait for this task to handle the mailbox events from the VF. This reduces some complex code flow, and makes it so we have a single path for handling the VF messages. There is a possibility that we have a slight delay in handling VF messages, but it should be minimal.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_iov.c | 30
++--------------------------
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c |  3 +++
 2 files changed, 5 insertions(+), 28 deletions(-)

Add a header comment explaining why we have the somewhat crazy mailbox flow. This flow is necessary as it prevents the PF<->SM mailbox from being flooded by the VF messages, which normally trigger a message to the PF. This helps prevent the case where we see a PF mailbox timeout.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_iov.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Add a header comment explaining why we have the somewhat crazy mailbox flow. This flow is necessary as it prevents the PF<->SM mailbox from being flooded by the VF messages, which normally trigger a message to the PF. This helps prevent the case where we see a PF mailbox timeout.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_iov.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

The header comment included a miscopy of a C-code line, and also mis-used Rx FIFO when it clearly meant Tx FIFO

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_mbx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

The header comment included a miscopy of a C-code line, and also mis-used Rx FIFO when it clearly meant Tx FIFO

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_mbx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Since the service task handles varying work that doesn't all require the
interface to be up, launch the service timer immediately. This ensures that we continually check the mailbox, as well as handle other tasks while the device is down.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Since the service task handles varying work that doesn't all require the interface to be up, launch the service timer immediately. This ensures that we continually check the mailbox, as well as handle other tasks while the device is down.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Fm10k's interrupt throttle timers are based on the PCIe link speed. Because of this, the value being programmed into the ITR registers must be scaled.

For the PF, this is as simple as reading the PCIe link speed and storing
the result. However, in the case of SR-IOV, the VF's interrupt throttle
timers are based on the link speed of the PF. However, the VF is unable
to get the link speed information from its configuration space, so the PF must inform it of what scale to use.

Rather than pass this scale via mailbox message, take advantage of unused bits in the TDLEN register to pass the scale. It is the responsibility of the PF to program this for the VF while setting up the
VF queues and the responsibility of the VF to get the information accordingly. This is preferable because it allows the VF to set up the interrupts properly during initialization and matches how the MAC address is passed in the TDBAL/TDBAH registers.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Signed-off-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k.h      |  4 ++++
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 17 +++++++++++++----
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c  | 12 ++++++++++--
 drivers/net/ethernet/intel/fm10k/fm10k_pf.c   | 16 +++++++++++++++-
 drivers/net/ethernet/intel/fm10k/fm10k_type.h |  6 ++++++
 drivers/net/ethernet/intel/fm10k/fm10k_vf.c   | 11 +++++++++--
 6 files changed, 57 insertions(+), 9 deletions(-)

Fm10k's interrupt throttle timers are based on the PCIe link speed. Because of this, the value being programmed into the ITR registers must be scaled.

For the PF, this is as simple as reading the PCIe link speed and storing the result. However, in the case of SR-IOV, the VF's interrupt throttle timers are based on the link speed of the PF. However, the VF is unable to get the link speed information from its configuration space, so the PF must inform it of what scale to use.

Rather than pass this scale via mailbox message, take advantage of unused bits in the TDLEN register to pass the scale. It is the responsibility of the PF to program this for the VF while setting up the VF queues and the responsibility of the VF to get the information accordingly. This is preferable because it allows the VF to set up the interrupts properly during initialization and matches how the MAC address is passed in the TDBAL/TDBAH registers.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Signed-off-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k.h      |  4 ++++
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 17 +++++++++++++----
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c  | 12 ++++++++++--
 drivers/net/ethernet/intel/fm10k/fm10k_pf.c   | 16 +++++++++++++++-
 drivers/net/ethernet/intel/fm10k/fm10k_type.h |  6 ++++++
 drivers/net/ethernet/intel/fm10k/fm10k_vf.c   | 11 +++++++++--
 6 files changed, 57 insertions(+), 9 deletions(-)

drivers/net/ethernet/intel/fm10k/fm10k.h +0 -0

index cc7f442e..feb53c0 100644

drivers/net/ethernet/intel/fm10k/fm10k_main.c +8 -3

index 1e08832..cd2e86a 100644
@@ -1395,11 +1395,20 @@ static void fm10k_update_itr(struct fm10k_ring_container *ring_container)
 	if (avg_wire_size > 3000)
 		avg_wire_size = 3000;
 
-	/* Give a little boost to mid-size frames */
-	if ((avg_wire_size > 300) && (avg_wire_size < 1200))
-		avg_wire_size /= 3;
+	/* Simple throttle rate management based on average wire size,
+	 * providing boosts to small and medium packet loads. Divide the
+	 * average wire size by a constant factor to calculate the minimum time
+	 * until the next interrupt in microseconds.
+	 */
+	if (avg_wire_size < 300)
+		avg_wire_size /= FM10K_ITR_SCALE_SMALL;
+	else if ((avg_wire_size >= 300) && (avg_wire_size < 1200))

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +8 -1

index cc527dd..c7c9832 100644
@@ -532,6 +532,9 @@ static void fm10k_configure_tx_ring(struct fm10k_intfc *interface,
 	/* store tail pointer */
 	ring->tail = &interface->uc_addr[FM10K_TDT(reg_idx)];
 
+	/* store ITR scale */
+	ring->itr_scale = hw->mac.itr_scale;
+
 	/* reset ntu and ntc to place SW in sync with hardwdare */
 	ring->next_to_clean = 0;
 	ring->next_to_use = 0;

@@ -638,6 +641,9 @@ static void fm10k_configure_rx_ring(struct fm10k_intfc *interface,
 	/* store tail pointer */
 	ring->tail = &interface->uc_addr[FM10K_RDT(reg_idx)];
 
+	/* store ITR scale */
+	ring->itr_scale = hw->mac.itr_scale;
+
 	/* reset ntu and ntc to place SW in sync with hardwdare */
 	ring->next_to_clean = 0;
 	ring->next_to_use = 0;

@@ -824,7 +830,8 @@ static irqreturn_t fm10k_msix_mbx_vf(int __always_unused irq, void *data)
 
 	/* re-enable mailbox interrupt and indicate 20us delay */
 	fm10k_write_reg(hw, FM10K_VFITR(FM10K_MBX_VECTOR),
-			FM10K_ITR_ENABLE | FM10K_MBX_INT_DELAY);
+			FM10K_ITR_ENABLE | (FM10K_MBX_INT_DELAY /
+					    hw->mac.itr_scale));
 
 	/* service upstream mailbox */
 	if (fm10k_mbx_trylock(interface)) {

drivers/net/ethernet/intel/fm10k/fm10k_pf.c +15 -1

index a92b58d..be80024 100644
@@ -144,16 +144,21 @@ static s32 fm10k_init_hw_pf(struct fm10k_hw *hw)
 				FM10K_TPH_RXCTRL_HDR_WROEN);
 	}
 
-	/* set max hold interval to align with 1.024 usec in all modes */
+	/* set max hold interval to align with 1.024 usec in all modes and
+	 * store ITR scale
+	 */
 	switch (hw->bus.speed) {
 	case fm10k_bus_speed_2500:
 		dma_ctrl = FM10K_DMA_CTRL_MAX_HOLD_1US_GEN1;
+		hw->mac.itr_scale = FM10K_TDLEN_ITR_SCALE_GEN1;
 		break;
 	case fm10k_bus_speed_5000:
 		dma_ctrl = FM10K_DMA_CTRL_MAX_HOLD_1US_GEN2;
+		hw->mac.itr_scale = FM10K_TDLEN_ITR_SCALE_GEN2;
 		break;
 	case fm10k_bus_speed_8000:
 		dma_ctrl = FM10K_DMA_CTRL_MAX_HOLD_1US_GEN3;
+		hw->mac.itr_scale = FM10K_TDLEN_ITR_SCALE_GEN3;
 		break;
 	default:
 		dma_ctrl = 0;

@@ -907,6 +912,12 @@ static s32 fm10k_iov_assign_default_mac_vlan_pf(struct fm10k_hw *hw,
 	fm10k_write_reg(hw, FM10K_TDBAL(vf_q_idx), tdbal);
 	fm10k_write_reg(hw, FM10K_TDBAH(vf_q_idx), tdbah);
 
+	/* Provide the VF the ITR scale, using software-defined fields in TDLEN
+	 * to pass the information during VF initialization
+	 */
+	fm10k_write_reg(hw, FM10K_TDLEN(vf_q_idx), hw->mac.itr_scale <<
+						   FM10K_TDLEN_ITR_SCALE_SHIFT);
+
 err_out:
 	/* configure Queue control register */
 	txqctl = ((u32)vf_vid << FM10K_TXQCTL_VID_SHIFT) &

@@ -1039,6 +1050,9 @@ static s32 fm10k_iov_reset_resources_pf(struct fm10k_hw *hw,
 	for (i = queues_per_pool; i--;) {
 		fm10k_write_reg(hw, FM10K_TDBAL(vf_q_idx + i), tdbal);
 		fm10k_write_reg(hw, FM10K_TDBAH(vf_q_idx + i), tdbah);
+		fm10k_write_reg(hw, FM10K_TDLEN(vf_q_idx + i),
+				hw->mac.itr_scale <<
+				FM10K_TDLEN_ITR_SCALE_SHIFT);
 		fm10k_write_reg(hw, FM10K_TQMAP(qmap_idx + i), vf_q_idx + i);
 		fm10k_write_reg(hw, FM10K_RQMAP(qmap_idx + i), vf_q_idx + i);
 	}

drivers/net/ethernet/intel/fm10k/fm10k_type.h +6 -0

index 4af9668..c9a646d 100644
@@ -271,6 +271,11 @@ struct fm10k_hw;
 #define FM10K_TDBAL(_n)		((0x40 * (_n)) + 0x8000)
 #define FM10K_TDBAH(_n)		((0x40 * (_n)) + 0x8001)
 #define FM10K_TDLEN(_n)		((0x40 * (_n)) + 0x8002)
+#define FM10K_TDLEN_ITR_SCALE_SHIFT		9
+#define FM10K_TDLEN_ITR_SCALE_MASK		0x00000E00
+#define FM10K_TDLEN_ITR_SCALE_GEN1		4
+#define FM10K_TDLEN_ITR_SCALE_GEN2		2
+#define FM10K_TDLEN_ITR_SCALE_GEN3		1
 #define FM10K_TPH_TXCTRL(_n)	((0x40 * (_n)) + 0x8003)
 #define FM10K_TPH_TXCTRL_DESC_TPHEN		0x00000020
 #define FM10K_TPH_TXCTRL_DESC_RROEN		0x00000200

@@ -560,6 +565,7 @@ struct fm10k_mac_info {
 	bool get_host_state;
 	bool tx_ready;
 	u32 dglort_map;
+	u8 itr_scale;
 };
 
 struct fm10k_swapi_table_info {

drivers/net/ethernet/intel/fm10k/fm10k_vf.c +5 -1

index 94f0f6a..099f190 100644
@@ -28,7 +28,7 @@
 static s32 fm10k_stop_hw_vf(struct fm10k_hw *hw)
 {
 	u8 *perm_addr = hw->mac.perm_addr;
-	u32 bal = 0, bah = 0;
+	u32 bal = 0, bah = 0, tdlen;
 	s32 err;
 	u16 i;
 

@@ -48,6 +48,9 @@ static s32 fm10k_stop_hw_vf(struct fm10k_hw *hw)
 		       ((u32)perm_addr[2]);
 	}
 
+	/* restore default itr_scale for next VF initialization */
+	tdlen = hw->mac.itr_scale << FM10K_TDLEN_ITR_SCALE_SHIFT;
+
 	/* The queues have already been disabled so we just need to
 	 * update their base address registers
 	 */

@@ -56,6 +59,7 @@ static s32 fm10k_stop_hw_vf(struct fm10k_hw *hw)
 		fm10k_write_reg(hw, FM10K_TDBAH(i), bah);
 		fm10k_write_reg(hw, FM10K_RDBAL(i), bal);
 		fm10k_write_reg(hw, FM10K_RDBAH(i), bah);
+		fm10k_write_reg(hw, FM10K_TDLEN(i), tdlen);
 	}
 
 	return 0;

@@ -124,9 +128,12 @@ static s32 fm10k_init_hw_vf(struct fm10k_hw *hw)
 	/* record maximum queue count */
 	hw->mac.max_queues = i;
 

On 04/03/2015 01:27 PM, Jeff Kirsher wrote:

Quoted 7 lines

drivers/net/ethernet/intel/fm10k/fm10k.h +0 -0

index cc7f442e..feb53c0 100644

drivers/net/ethernet/intel/fm10k/fm10k.h struct fm10k_ring { · line 137

Quoted 7 lines

drivers/net/ethernet/intel/fm10k/fm10k.h +1 -0

@@ -131,6 +131,7 @@ struct fm10k_ring {
 					 * different for DCB and RSS modes
 					 */
 	u8 qos_pc;			/* priority class of queue */
+	u8 itr_scale;			/* throttle scaler based on PCI speed */
 	u16 vid;			/* default vlan ID of queue */
 	u16 count;			/* amount of descriptors */
 

This shouldn't be a part of the ring. If it is related to the ITR it really belongs in the ring container or the q_vector.

Quoted 10 lines

drivers/net/ethernet/intel/fm10k/fm10k.h +0 -0

@@ -164,6 +165,9 @@ struct fm10k_ring_container {
 #define FM10K_ITR_10K		100	/* 100us */
 #define FM10K_ITR_20K		50	/* 50us */
 #define FM10K_ITR_ADAPTIVE	0x8000	/* adaptive interrupt moderation

flag */

drivers/net/ethernet/intel/fm10k/fm10k.h +2 -0

+#define FM10K_ITR_SCALE_SMALL	60	/* Constant factor for small frames */
+#define FM10K_ITR_SCALE_MEDIUM	50	/* Constant factor for medium frames

*/
+#define FM10K_ITR_SCALE_LARGE 40 /* Constant factor for large frames */

Quoted 2 lines

#define FM10K_ITR_ENABLE (FM10K_ITR_AUTOMASK | FM10K_ITR_MASK_CLEAR)

These values don't make any sense to me, where did they come from?

Quoted 22 lines

drivers/net/ethernet/intel/fm10k/fm10k_main.c +3 -3

index 1e08832..cd2e86a 100644
@@ -1395,11 +1395,20 @@ static void fm10k_update_itr(struct
 fm10k_ring_container *ring_container)
 	if (avg_wire_size > 3000)
 		avg_wire_size = 3000;
 
-	/* Give a little boost to mid-size frames */
-	if ((avg_wire_size > 300) && (avg_wire_size < 1200))
-		avg_wire_size /= 3;
+	/* Simple throttle rate management based on average wire size,
+	 * providing boosts to small and medium packet loads. Divide the
+	 * average wire size by a constant factor to calculate the minimum

time

drivers/net/ethernet/intel/fm10k/fm10k_main.c +5 -0

+	 * until the next interrupt in microseconds.
+	 */
+	if (avg_wire_size < 300)
+		avg_wire_size /= FM10K_ITR_SCALE_SMALL;
+	else if ((avg_wire_size >= 300) && (avg_wire_size < 1200))

drivers/net/ethernet/intel/fm10k/fm10k_main.c static void fm10k_update_itr(struct fm10k_ring_container *ring_container) · line 1414

Quoted 10 lines

drivers/net/ethernet/intel/fm10k/fm10k_main.c +5 -1

+		avg_wire_size /= FM10K_ITR_SCALE_MEDIUM;
 	else
-		avg_wire_size /= 2;
+		avg_wire_size /= FM10K_ITR_SCALE_LARGE;
+
+	/* Scale for various PCIe link speeds */
+	avg_wire_size /= ring_container->ring->itr_scale;
 
 	/* write back value and retain adaptive flag */
 	ring_container->itr = avg_wire_size | FM10K_ITR_ADAPTIVE;

This seems like all it is doing is maxing out the interrupt rate for all cases. For example, a value of 1514 is reduced to 37.8. When you use that as a usecs value that means the queue is capable of over 26K interrupts per second.

The division by itr_scale is a really bad idea. I would recommend replacing it with a shift and you should probably check for the value hitting 0.

I suspect you probably aren't seeing much of a penalty because the MSI-X interrupt call is pretty cheap, however that is still a pretty high interrupt rate compared to past parts.

Quoted 47 lines

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +3 -0

index cc527dd..c7c9832 100644
@@ -532,6 +532,9 @@ static void fm10k_configure_tx_ring(struct
 fm10k_intfc *interface,
 	/* store tail pointer */
 	ring->tail = &interface->uc_addr[FM10K_TDT(reg_idx)];
 
+	/* store ITR scale */
+	ring->itr_scale = hw->mac.itr_scale;
+
 	/* reset ntu and ntc to place SW in sync with hardwdare */
 	ring->next_to_clean = 0;

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +3 -0

 	ring->next_to_use = 0;

@@ -638,6 +641,9 @@ static void fm10k_configure_rx_ring(struct
 fm10k_intfc *interface,
 	/* store tail pointer */
 	ring->tail = &interface->uc_addr[FM10K_RDT(reg_idx)];
 
+	/* store ITR scale */
+	ring->itr_scale = hw->mac.itr_scale;
+
 	/* reset ntu and ntc to place SW in sync with hardwdare */
 	ring->next_to_clean = 0;

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +2 -1

 	ring->next_to_use = 0;

@@ -824,7 +830,8 @@ static irqreturn_t fm10k_msix_mbx_vf(int
 __always_unused irq, void *data)
 
 	/* re-enable mailbox interrupt and indicate 20us delay */
 	fm10k_write_reg(hw, FM10K_VFITR(FM10K_MBX_VECTOR),
-			FM10K_ITR_ENABLE | FM10K_MBX_INT_DELAY);
+			FM10K_ITR_ENABLE | (FM10K_MBX_INT_DELAY /
+					    hw->mac.itr_scale));
 
 	/* service upstream mailbox */

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +2 -1

 	if (fm10k_mbx_trylock(interface)) {

@@ -1007,7 +1014,8 @@ static irqreturn_t fm10k_msix_mbx_pf(int
 __always_unused irq, void *data)
 
 	/* re-enable mailbox interrupt and indicate 20us delay */
 	fm10k_write_reg(hw, FM10K_ITR(FM10K_MBX_VECTOR),
-			FM10K_ITR_ENABLE | FM10K_MBX_INT_DELAY);
+			FM10K_ITR_ENABLE | (FM10K_MBX_INT_DELAY /
+					    hw->mac.itr_scale));
 

Quoted 2 lines

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +0 -0

 	return IRQ_HANDLED;

}

You would be much better off here using the itr_scale as a multiple or a shift value rather than doing a divide in an interrupt handler as a divide can be quite expensive.

Quoted 20 lines

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +5 -1

@@ -56,6 +59,7 @@ static s32 fm10k_stop_hw_vf(struct fm10k_hw *hw)
 		fm10k_write_reg(hw, FM10K_TDBAH(i), bah);
 		fm10k_write_reg(hw, FM10K_RDBAL(i), bal);
 		fm10k_write_reg(hw, FM10K_RDBAH(i), bah);
+		fm10k_write_reg(hw, FM10K_TDLEN(i), tdlen);
 	}
 
 	return 0;

@@ -124,9 +128,12 @@ static s32 fm10k_init_hw_vf(struct fm10k_hw *hw)
 	/* record maximum queue count */
 	hw->mac.max_queues = i;
 
-	/* fetch default VLAN */
+	/* fetch default VLAN and ITR scale */
 	hw->mac.default_vid = (fm10k_read_reg(hw, FM10K_TXQCTL(0)) &
 			       FM10K_TXQCTL_VID_MASK) >> FM10K_TXQCTL_VID_SHIFT;
+	hw->mac.itr_scale = (fm10k_read_reg(hw, FM10K_TDLEN(0)) &
+			     FM10K_TDLEN_ITR_SCALE_MASK) >>
+			    FM10K_TDLEN_ITR_SCALE_SHIFT;
 

drivers/net/ethernet/intel/fm10k/fm10k_pci.c static s32 fm10k_init_hw_vf(struct fm10k_hw *hw) · line 139

Quoted 3 lines

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +0 -0

 	return 0;
 }

This is a good reason to get rid of the divide in favor of a shift. If a VF didn't restore the tdlen value using the stop_hw_vf function then you could potentially have one driver load trip up the next and cause a divide by 0.

Quoted 1 line · Alexander Duyck

On 04/03/2015 01:27 PM, Jeff Kirsher wrote:

[...]

Quoted 7 lines

Quoted 7 lines

drivers/net/ethernet/intel/fm10k/fm10k.h +0 -0

index cc7f442e..feb53c0 100644
@@ -131,6 +131,7 @@ struct fm10k_ring {
 					 * different for DCB and RSS modes

Quoted 8 lines

drivers/net/ethernet/intel/fm10k/fm10k.h struct fm10k_ring { · line 137

Quoted 6 lines

drivers/net/ethernet/intel/fm10k/fm10k.h +1 -0

 					 */
 	u8 qos_pc;			/* priority class of queue */
+	u8 itr_scale;			/* throttle scaler based on PCI speed */
 	u16 vid;			/* default vlan ID of queue */
 	u16 count;			/* amount of descriptors */
 

This shouldn't be a part of the ring. If it is related to the ITR it really belongs in the ring container or the q_vector.

Fair enough--I definitely went back and forth on where to put it. I think I'll put it in the ring container, since the ITR already gets updated there. It's just mildly annoying during the initialization routines is all.

Quoted 10 lines

Quoted 10 lines

drivers/net/ethernet/intel/fm10k/fm10k.h +0 -0

@@ -164,6 +165,9 @@ struct fm10k_ring_container {
 #define FM10K_ITR_10K		100	/* 100us */
 #define FM10K_ITR_20K		50	/* 50us */
 #define FM10K_ITR_ADAPTIVE	0x8000	/* adaptive interrupt moderation

flag */

drivers/net/ethernet/intel/fm10k/fm10k.h +2 -0

+#define FM10K_ITR_SCALE_SMALL	60	/* Constant factor for small frames */
+#define FM10K_ITR_SCALE_MEDIUM	50	/* Constant factor for medium frames

*/
+#define FM10K_ITR_SCALE_LARGE 40 /* Constant factor for large frames */

Quoted 3 lines

Quoted 2 lines

#define FM10K_ITR_ENABLE (FM10K_ITR_AUTOMASK | FM10K_ITR_MASK_CLEAR)

These values don't make any sense to me, where did they come from?

They were experimental. I'll be the first to admit they probably aren't perfect, but they're at least a significant improvement.

Do you have some recommendations for interrupt rates at 25G/50G? Depending on what the targets are, it'd be nice to get these scalars to something we can shift by instead of divide by.

Quoted 37 lines

Quoted 22 lines

drivers/net/ethernet/intel/fm10k/fm10k_main.c +3 -3

index 1e08832..cd2e86a 100644
@@ -1395,11 +1395,20 @@ static void fm10k_update_itr(struct
 fm10k_ring_container *ring_container)
 	if (avg_wire_size > 3000)
 		avg_wire_size = 3000;
 
-	/* Give a little boost to mid-size frames */
-	if ((avg_wire_size > 300) && (avg_wire_size < 1200))
-		avg_wire_size /= 3;
+	/* Simple throttle rate management based on average wire size,
+	 * providing boosts to small and medium packet loads. Divide the
+	 * average wire size by a constant factor to calculate the minimum

time

drivers/net/ethernet/intel/fm10k/fm10k_main.c +5 -0

+	 * until the next interrupt in microseconds.
+	 */
+	if (avg_wire_size < 300)
+		avg_wire_size /= FM10K_ITR_SCALE_SMALL;
+	else if ((avg_wire_size >= 300) && (avg_wire_size < 1200))

drivers/net/ethernet/intel/fm10k/fm10k_main.c static void fm10k_update_itr(struct fm10k_ring_container *ring_container) · line 1414

Quoted 10 lines

drivers/net/ethernet/intel/fm10k/fm10k_main.c +5 -1

+		avg_wire_size /= FM10K_ITR_SCALE_MEDIUM;
 	else
-		avg_wire_size /= 2;
+		avg_wire_size /= FM10K_ITR_SCALE_LARGE;
+
+	/* Scale for various PCIe link speeds */
+	avg_wire_size /= ring_container->ring->itr_scale;
 
 	/* write back value and retain adaptive flag */
 	ring_container->itr = avg_wire_size | FM10K_ITR_ADAPTIVE;

This seems like all it is doing is maxing out the interrupt rate for all cases. For example, a value of 1514 is reduced to 37.8. When you use that as a usecs value that means the queue is capable of over 26K interrupts per second.

Quoted 7 lines

The division by itr_scale is a really bad idea. I would recommend replacing it with a shift and you should probably check for the value hitting 0.

I suspect you probably aren't seeing much of a penalty because the MSI-X interrupt call is pretty cheap, however that is still a pretty high interrupt rate compared to past parts.

The interrupt rates are definitely open to suggestion. I tried to maintain the idea of the original algorithm, which did basically the same thing (avg_wire_size divided by some scalar), so I'm not seeing what's significantly different here from that angle.

A shift for the itr_scale is a fair suggestion. I'll incorporate that into a v2.

I definitely understand that I went with some high interrupt values, but given the architecture of the device I'd be skeptical about dropping all the way to 8000int/s.

Quoted 48 lines

Quoted 48 lines

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +3 -0

index cc527dd..c7c9832 100644
@@ -532,6 +532,9 @@ static void fm10k_configure_tx_ring(struct
 fm10k_intfc *interface,
 	/* store tail pointer */
 	ring->tail = &interface->uc_addr[FM10K_TDT(reg_idx)];
 
+	/* store ITR scale */
+	ring->itr_scale = hw->mac.itr_scale;
+
 	/* reset ntu and ntc to place SW in sync with hardwdare */
 	ring->next_to_clean = 0;

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +3 -0

 	ring->next_to_use = 0;

@@ -638,6 +641,9 @@ static void fm10k_configure_rx_ring(struct
 fm10k_intfc *interface,
 	/* store tail pointer */
 	ring->tail = &interface->uc_addr[FM10K_RDT(reg_idx)];
 
+	/* store ITR scale */
+	ring->itr_scale = hw->mac.itr_scale;
+
 	/* reset ntu and ntc to place SW in sync with hardwdare */
 	ring->next_to_clean = 0;

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +2 -1

 	ring->next_to_use = 0;

@@ -824,7 +830,8 @@ static irqreturn_t fm10k_msix_mbx_vf(int
 __always_unused irq, void *data)
 
 	/* re-enable mailbox interrupt and indicate 20us delay */
 	fm10k_write_reg(hw, FM10K_VFITR(FM10K_MBX_VECTOR),
-			FM10K_ITR_ENABLE | FM10K_MBX_INT_DELAY);
+			FM10K_ITR_ENABLE | (FM10K_MBX_INT_DELAY /
+					    hw->mac.itr_scale));
 
 	/* service upstream mailbox */

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +2 -1

 	if (fm10k_mbx_trylock(interface)) {

@@ -1007,7 +1014,8 @@ static irqreturn_t fm10k_msix_mbx_pf(int
 __always_unused irq, void *data)
 
 	/* re-enable mailbox interrupt and indicate 20us delay */
 	fm10k_write_reg(hw, FM10K_ITR(FM10K_MBX_VECTOR),
-			FM10K_ITR_ENABLE | FM10K_MBX_INT_DELAY);
+			FM10K_ITR_ENABLE | (FM10K_MBX_INT_DELAY /
+					    hw->mac.itr_scale));
 

Quoted 5 lines

Quoted 2 lines

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +0 -0

 	return IRQ_HANDLED;

}

You would be much better off here using the itr_scale as a multiple or a shift value rather than doing a divide in an interrupt handler as a divide can be quite expensive.

Will fix for v2.

Quoted 20 lines

Quoted 20 lines

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +5 -1

@@ -56,6 +59,7 @@ static s32 fm10k_stop_hw_vf(struct fm10k_hw *hw)
 		fm10k_write_reg(hw, FM10K_TDBAH(i), bah);
 		fm10k_write_reg(hw, FM10K_RDBAL(i), bal);
 		fm10k_write_reg(hw, FM10K_RDBAH(i), bah);
+		fm10k_write_reg(hw, FM10K_TDLEN(i), tdlen);
 	}
 
 	return 0;

@@ -124,9 +128,12 @@ static s32 fm10k_init_hw_vf(struct fm10k_hw *hw)
 	/* record maximum queue count */
 	hw->mac.max_queues = i;
 
-	/* fetch default VLAN */
+	/* fetch default VLAN and ITR scale */
 	hw->mac.default_vid = (fm10k_read_reg(hw, FM10K_TXQCTL(0)) &
 			       FM10K_TXQCTL_VID_MASK) >> FM10K_TXQCTL_VID_SHIFT;
+	hw->mac.itr_scale = (fm10k_read_reg(hw, FM10K_TDLEN(0)) &
+			     FM10K_TDLEN_ITR_SCALE_MASK) >>
+			    FM10K_TDLEN_ITR_SCALE_SHIFT;
 

Quoted 7 lines

drivers/net/ethernet/intel/fm10k/fm10k_pci.c static s32 fm10k_init_hw_vf(struct fm10k_hw *hw) · line 139

Quoted 3 lines

drivers/net/ethernet/intel/fm10k/fm10k_pci.c +0 -0

 	return 0;
 }

This is a good reason to get rid of the divide in favor of a shift. If a VF didn't restore the tdlen value using the stop_hw_vf function then you could potentially have one driver load trip up the next and cause a divide by 0.

Agreed, and sure enough we have a commit in the works that assumes a default value if we read zero. I originally went with the divide for readability's sake, but the shift is probably the better choice, so I'll send a v2 with that.

If you have some suggestions for an interrupt range, I can look into incorporating those as well.

Thank you for the review, Alex!

Cheers,
Matthew

Quoted 4 lines · Matthew

Quoted 1 line · Alexander Duyck

On 04/03/2015 01:27 PM, Jeff Kirsher wrote:

Quoted 16 lines

Quoted 13 lines

Quoted 13 lines

drivers/net/ethernet/intel/fm10k/fm10k.h +0 -0

@@ -164,6 +165,9 @@ struct fm10k_ring_container {
 #define FM10K_ITR_10K		100	/* 100us */
 #define FM10K_ITR_20K		50	/* 50us */
 #define FM10K_ITR_ADAPTIVE	0x8000	/* adaptive interrupt moderation

flag */
+#define FM10K_ITR_SCALE_SMALL 60 /* Constant factor for small frames
*/
+#define FM10K_ITR_SCALE_MEDIUM 50 /* Constant factor for medium
frames
*/
+#define FM10K_ITR_SCALE_LARGE 40 /* Constant factor for large frames
*/

Quoted 3 lines

Quoted 2 lines

#define FM10K_ITR_ENABLE (FM10K_ITR_AUTOMASK | FM10K_ITR_MASK_CLEAR)

These values don't make any sense to me, where did they come from?

Quoted 8 lines

They were experimental. I'll be the first to admit they probably aren't perfect, but they're at least a significant improvement.

Do you have some recommendations for interrupt rates at 25G/50G? Depending
on what the targets are, it'd be nice to get these scalars to something we
can shift by instead of divide by.

If nothing else these need to be documented along with the expected range of interrupts per second.

So for example if you expect the FM10K_ITR_SCALE_SMALL of 60 to be used with packets 300 bytes and smaller you should document somewhere that this is going to drive rates of 1 million to 200 thousand interrupts per second if that is what you actually want. I was assuming that was a bug since that value seems obscenely high, however at 50Gb/s I supposed it is possible that you could theoretically push 70Mpps so in theory that might be appropriate if we were actually processing small packets at line rate.. :-)

Quoted 52 lines

Quoted 38 lines

Quoted 22 lines

drivers/net/ethernet/intel/fm10k/fm10k_main.c +3 -3

index 1e08832..cd2e86a 100644
@@ -1395,11 +1395,20 @@ static void fm10k_update_itr(struct
 fm10k_ring_container *ring_container)
 	if (avg_wire_size > 3000)
 		avg_wire_size = 3000;
 
-	/* Give a little boost to mid-size frames */
-	if ((avg_wire_size > 300) && (avg_wire_size < 1200))
-		avg_wire_size /= 3;
+	/* Simple throttle rate management based on average wire size,
+	 * providing boosts to small and medium packet loads. Divide the
+	 * average wire size by a constant factor to calculate the minimum

time

drivers/net/ethernet/intel/fm10k/fm10k_main.c +5 -0

+	 * until the next interrupt in microseconds.
+	 */
+	if (avg_wire_size < 300)
+		avg_wire_size /= FM10K_ITR_SCALE_SMALL;
+	else if ((avg_wire_size >= 300) && (avg_wire_size < 1200))

drivers/net/ethernet/intel/fm10k/fm10k_main.c static void fm10k_update_itr(struct fm10k_ring_container *ring_container) · line 1414

Quoted 10 lines

drivers/net/ethernet/intel/fm10k/fm10k_main.c +5 -1

+		avg_wire_size /= FM10K_ITR_SCALE_MEDIUM;
 	else
-		avg_wire_size /= 2;
+		avg_wire_size /= FM10K_ITR_SCALE_LARGE;
+
+	/* Scale for various PCIe link speeds */
+	avg_wire_size /= ring_container->ring->itr_scale;
 
 	/* write back value and retain adaptive flag */
 	ring_container->itr = avg_wire_size | FM10K_ITR_ADAPTIVE;

This seems like all it is doing is maxing out the interrupt rate for all
cases. For example, a value of 1514 is reduced to 37.8. When you use that as a usecs value that means the queue is capable of over 26K interrupts per second.

Quoted 8 lines

The division by itr_scale is a really bad idea. I would recommend replacing it with a shift and you should probably check for the value hitting 0.

I suspect you probably aren't seeing much of a penalty because the MSI-X
interrupt call is pretty cheap, however that is still a pretty high interrupt rate compared to past parts.

The interrupt rates are definitely open to suggestion. I tried to maintain
the idea of the original algorithm, which did basically the same thing (avg_wire_size divided by some scalar), so I'm not seeing what's significantly different here from that angle.

Quoted 7 lines

A shift for the itr_scale is a fair suggestion. I'll incorporate that into
a v2.

I definitely understand that I went with some high interrupt values, but given the architecture of the device I'd be skeptical about dropping all the way to 8000int/s.

I think the big issue with this is that there is no bottom end. So if you have a setup where you are running small packets your adjustment brings it all the way down to 1us since 60/60 is 1. Then you also have to do a shift by 2 or divide by 4 for the PCIe gen1 which will bring the value down to 0 which isn't correct.

What you probably need to do is look at creating a limit on just how low a value you can have. Since you are having to do a divide by 4 after the computation you might make 4us the lowest value you can generate for this algorithm, and probably make that the limit, excluding ITR disabled, for user controllable interrupt moderation as well. Then that way things are always at least throttled to no more than 250K interrupts/second per queue. Otherwise there is s good chance that everything with packet size smaller than 240B will just end up with no interrupt moderation since that is currently being shifted off into oblivion.

When the PF receives a request to update a multicast address for the VF,
it checks the enabled multicast mode first. Fix a bug where the VF tried
to set a multicast address before requesting the required xcast mode. This ensures the multicast addresses are honored as long as the xcast mode was allowed.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 22
+++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

When the PF receives a request to update a multicast address for the VF, it checks the enabled multicast mode first. Fix a bug where the VF tried to set a multicast address before requesting the required xcast mode.
This ensures the multicast addresses are honored as long as the xcast mode was allowed.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 22
+++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

The use of dropped doesn't really mean dropped mailbox messages, but rather specifically messages which were too large to fit in the remote Rx FIFO. Rename the stat to more clearly indicate what it means.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The use of dropped doesn't really mean dropped mailbox messages, but rather specifically messages which were too large to fit in the remote Rx FIFO. Rename the stat to more clearly indicate what it means.

Signed-off-by Jeff Kirsher <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

Acked-by Matthew Vick <[email protected]>

---
 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)