[Intel-wired-lan] [jkirsher/next-queue PATCH 2/2] ixgbe: Refactor queue disable logic to take completion time into account

Shannon Nelson shannon.nelson at oracle.com
Mon Jul 23 16:34:29 UTC 2018


On 7/20/2018 3:29 PM, Alexander Duyck wrote:
> This change is meant to allow us to take completion time into account when
> disabling queues. Previously we were just working with hard coded values
> for how long we should wait. This worked fine for the standard case where
> completion timeout was operating in the 50us to 50ms range, however on
> platforms that have higher completion timeout times this was resulting in
> Rx queues disable messages being displayed as we weren't waiting long
> enough for outstanding Rx DMA completions.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck at intel.com>
> ---

[...]

> +
> +void ixgbe_disable_tx(struct ixgbe_adapter *adapter)
> +{
> +	unsigned long wait_delay, delay_interval;
> +	struct ixgbe_hw *hw = &adapter->hw;
> +	int i, wait_loop;
> +	u32 txdctl;
> +
> +	if (ixgbe_removed(hw->hw_addr))
> +		return;
> +
> +	/* disable all enabled Tx queues */
> +	for (i = 0; i < adapter->num_tx_queues; i++) {
> +		struct ixgbe_ring *ring = adapter->tx_ring[i];
> +		u8 reg_idx = ring->reg_idx;
> +
> +		IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx), IXGBE_TXDCTL_SWFLSH);
> +	}
> +
> +	/* disable all enabled XDP Tx queues */
> +	for (i = 0; i < adapter->num_xdp_queues; i++) {
> +		struct ixgbe_ring *ring = adapter->xdp_ring[i];
> +		u8 reg_idx = ring->reg_idx;
> +
> +		IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx), IXGBE_TXDCTL_SWFLSH);
> +	}
> +
> +	/* If the link is now up there shouldn't be much in the way of

I think you mean "not" up rather than "now" up, which has the opposite 
meaning and could be a bit confusing.

> +	 * pending transactions. Those that are left will be flushed out
> +	 * when the reset logic goes through the flush sequence to clean out
> +	 * the pending Tx transactions.
> +	 */
> +	if (!(IXGBE_READ_REG(hw, IXGBE_LINKS) & IXGBE_LINKS_UP))
> +		goto dma_engine_disable;
> +
> +	/* Determine our minimum delay interval. We will increase this value
> +	 * with each subsequent test. This way if the device returns quickly
> +	 * we should spend as little time as possible waiting, however as
> +	 * the time increases we will wait for larger periods of time.
> +	 *
> +	 * The trick here is that we increase the interval using the
> +	 * following pattern: 1x 3x 5x 7x 9x 11x 13x 15x 17x 19x. The result
> +	 * of that wait is that it totals up to 100x whatever interval we
> +	 * choose. Since our minimum wait is 100us we can just divide the
> +	 * total timeout by 100 to get our minimum delay interval.
> +	 */
> +	delay_interval = ixgbe_get_completion_timeout(adapter) / 100;
> +
> +	wait_loop = IXGBE_MAX_RX_DESC_POLL;
> +	wait_delay = delay_interval;
> +
> +	while (wait_loop--) {
> +		usleep_range(wait_delay, wait_delay + 10);
> +		wait_delay += delay_interval * 2;
> +		txdctl = 0;
> +
> +		/* OR together the reading of all the active TXDCTL registers,
> +		 * and then test the result. We need the disable to complete
> +		 * before we start freeing the memory and invalidating the
> +		 * DMA mappings.
> +		 */
> +		for (i = 0; i < adapter->num_tx_queues; i++) {
> +			struct ixgbe_ring *ring = adapter->tx_ring[i];
> +			u8 reg_idx = ring->reg_idx;
> +
> +			txdctl |= IXGBE_READ_REG(hw, IXGBE_TXDCTL(reg_idx));
> +		}
> +		for (i = 0; i < adapter->num_xdp_queues; i++) {
> +			struct ixgbe_ring *ring = adapter->xdp_ring[i];
> +			u8 reg_idx = ring->reg_idx;
> +
> +			txdctl |= IXGBE_READ_REG(hw, IXGBE_TXDCTL(reg_idx));
> +		}
> +
> +		if (!(txdctl & IXGBE_TXDCTL_ENABLE))
> +			goto dma_engine_disable;
> +	}
> +
> +	e_err(drv,
> +	      "TXDCTL.ENABLE for one or more queues not cleared within the polling period\n");
> +
> +dma_engine_disable:
> +	/* Disable the Tx DMA engine on 82599 and later MAC */
> +	switch (hw->mac.type) {
> +	case ixgbe_mac_82599EB:
> +	case ixgbe_mac_X540:
> +	case ixgbe_mac_X550:
> +	case ixgbe_mac_X550EM_x:
> +	case ixgbe_mac_x550em_a:
> +		IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL,
> +				(IXGBE_READ_REG(hw, IXGBE_DMATXCTL) &
> +				 ~IXGBE_DMATXCTL_TE));
> +		/* fall through */
> +	default:
> +		break;
> +	}

Picky bike-shed thoughts: I realize you're just moving the previously 
existing code, but with the comment "... and later MAC" I would think 
the translation to code puts all the later MACs into the default and 
have the WRITE_REG() as the default action.  This would make it default 
to the correct action for any new MACs if someone forgot to fix up this 
part of the code.

	switch (hw->mac.type) {
	case ixgbe_mac_82598EB:
		break;

	case ixgbe_mac_82599EB:
	case ixgbe_mac_X540:
	case ixgbe_mac_X550:
	case ixgbe_mac_X550EM_x:
	case ixgbe_mac_x550em_a:
	default:
		IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL,
				(IXGBE_READ_REG(hw, IXGBE_DMATXCTL) &
				 ~IXGBE_DMATXCTL_TE));
		break;
	}

Or even simpler,

	if (hw->mac.type != ixgbe_mac_82598EB)
		IXGBE_WRITE_REG(hw, IXGBE_DMATXCTL,
				(IXGBE_READ_REG(hw, IXGBE_DMATXCTL) &
				 ~IXGBE_DMATXCTL_TE));

Okay, I'll stop now.

Cheers,
sln



More information about the Intel-wired-lan mailing list