On Mon, Oct 21, 2019 at 9:44 AM Robert Beckett
<bob.beck...@collabora.com> wrote:
>
> If rx flow control has been enabled (via autoneg or forced), packets
> should not be dropped due to rx descriptor ring exhaustion. Instead
> pause frames should be used to apply back pressure.
>
> Move SRRCTL setup to its own function for easy reuse and only set drop
> enable bit if rx flow control is not enabled.
>
> Signed-off-by: Robert Beckett <bob.beck...@collabora.com>
> ---
> drivers/net/ethernet/intel/igb/igb.h | 1 +
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 8 ++++
> drivers/net/ethernet/intel/igb/igb_main.c | 46 ++++++++++++++------
> 3 files changed, 41 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb.h
> b/drivers/net/ethernet/intel/igb/igb.h
> index ca54e268d157..49b5fa9d4783 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -661,6 +661,7 @@ void igb_configure_tx_ring(struct igb_adapter *, struct
> igb_ring *);
> void igb_configure_rx_ring(struct igb_adapter *, struct igb_ring *);
> void igb_setup_tctl(struct igb_adapter *);
> void igb_setup_rctl(struct igb_adapter *);
> +void igb_setup_srrctl(struct igb_adapter *, struct igb_ring *);
> netdev_tx_t igb_xmit_frame_ring(struct sk_buff *, struct igb_ring *);
> void igb_alloc_rx_buffers(struct igb_ring *, u16);
> void igb_update_stats(struct igb_adapter *);
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 5acf3b743876..3c951f363d0e 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -396,6 +396,7 @@ static int igb_set_pauseparam(struct net_device *netdev,
> struct igb_adapter *adapter = netdev_priv(netdev);
> struct e1000_hw *hw = &adapter->hw;
> int retval = 0;
> + int i;
>
> /* 100basefx does not support setting link flow control */
> if (hw->dev_spec._82575.eth_flags.e100_base_fx)
> @@ -428,6 +429,13 @@ static int igb_set_pauseparam(struct net_device *netdev,
>
> retval = ((hw->phy.media_type == e1000_media_type_copper) ?
> igb_force_mac_fc(hw) : igb_setup_link(hw));
> +
> + /* Make sure SRRCTL considers new fc settings for each ring */
> + for (i = 0; i < adapter->num_rx_queues; i++) {
> + struct igb_ring *ring = adapter->rx_ring[i];
> +
> + igb_setup_srrctl(adapter, ring);
> + }
> }
So one issue here is that this is going through and toggling things in
the case that SR-IOV is enabled. We likely should not be doing that.
If SR-IOV is enabled we should always have the DROP_EN bit set.
Otherwise we run the risk of soft-locking the part since a single
stopped Rx ring can cause both Tx and Rx to fail due to internal
switching of the part.
>
> clear_bit(__IGB_RESETTING, &adapter->state);
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> b/drivers/net/ethernet/intel/igb/igb_main.c
> index ffaa6e031632..6b04c961c6e4 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -4488,6 +4488,36 @@ static inline void igb_set_vmolr(struct igb_adapter
> *adapter,
> wr32(E1000_VMOLR(vfn), vmolr);
> }
>
> +/**
> + * igb_setup_srrctl - configure the split and replication receive control
> + * registers
> + * @adapter: Board private structure
> + * @ring: receive ring to be configured
> + **/
> +void igb_setup_srrctl(struct igb_adapter *adapter, struct igb_ring *ring)
> +{
> + struct e1000_hw *hw = &adapter->hw;
> + int reg_idx = ring->reg_idx;
> + u32 srrctl;
> +
> + srrctl = IGB_RX_HDR_LEN << E1000_SRRCTL_BSIZEHDRSIZE_SHIFT;
> + if (ring_uses_large_buffer(ring))
> + srrctl |= IGB_RXBUFFER_3072 >> E1000_SRRCTL_BSIZEPKT_SHIFT;
> + else
> + srrctl |= IGB_RXBUFFER_2048 >> E1000_SRRCTL_BSIZEPKT_SHIFT;
> + srrctl |= E1000_SRRCTL_DESCTYPE_ADV_ONEBUF;
> + if (hw->mac.type >= e1000_82580)
> + srrctl |= E1000_SRRCTL_TIMESTAMP;
> + /* Only set Drop Enable if we are supporting multiple queues
> + * and rx flow control is disabled
> + */
> + if (!(hw->fc.current_mode & e1000_fc_rx_pause) &&
> + (adapter->vfs_allocated_count || adapter->num_rx_queues > 1))
> + srrctl |= E1000_SRRCTL_DROP_EN;
> +
> + wr32(E1000_SRRCTL(reg_idx), srrctl);
> +}
I would recommend making the criteria that either you have VFs
allocated or more than one queue and flow control enabled. In the
SR-IOV case I would never recommend letting any Rx queue not have the
DROP_EN bit set. The reason being that Tx can be stopped if it is
waiting on the Rx FIFO to become available for a frame that must be
switched from Tx to Rx.
Also, from everything I have seen this can negatively impact
performance as one overused queue can drag down the performance for
all other queues.
