[Intel-wired-lan] [jkirsher/next-queue PATCH 2/2] ixgbe: Refactor queue disable logic to take completion time into account
Alexander Duyck
alexander.duyck at gmail.com
Mon Jul 23 17:10:57 UTC 2018
On Mon, Jul 23, 2018 at 9:34 AM, Shannon Nelson
<shannon.nelson at oracle.com> wrote:
> 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.
Yeah, that is a typo.
>
>> + * 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
Yeah, for now I will probably leave the code as is, but this is
something that could be updated int he future. Either that or you are
always free to submit a patch.. :-)
- Alex
More information about the Intel-wired-lan
mailing list