Question on iwlwifi iwl_mvm_mac_itxq_xmit, possible busy-spin.

[Ben Greear <greearb@candelatech.com>]

Hello,

We see indication that the iwlwifi txpath can busy-spin,
causing soft-lockup (and, only indication at this point, possibly
issue is elsewhere somehow).

While looking through the xmit path for potential busy-spin bugs, I found
this method below.  I'm planning to add this 'sofar' logic to bail out after
1000 loops:  It is not in upstream code.

But, I also wanted to check on expected behaviour.  At the bottom is a double
loop.  The inner will break out if the queues are full and for some other reasons,
but the outside loop is spinning on a different atomic counter.  The question is:
If the inner loop breaks out, at least for queue full reasons, should it then
immediately break out of the outer while loop as well?

And, from what I can tell, it would be possible for other transmitters to hit this
path, repeatedly increasing the tx_request to 2, causing the original transmitter
to run for a very long time.  Especially under high load with a slow kernel
larded up with debugging...  Maybe something like the 'sofar' logic would even
be wanted upstream?

Based on the description of the 3 tx_request states, I am also not sure that
this would not hang the tx path in case where inner loop bails out due to
tx queue full, leaving packets queued.  If no other packets are ever transmitted,
is there anything that would re-kick the xmit path?

void iwl_mvm_mac_itxq_xmit(struct ieee80211_hw *hw, struct ieee80211_txq *txq)
{
	struct iwl_mvm *mvm = IWL_MAC80211_GET_MVM(hw);
	struct iwl_mvm_txq *mvmtxq = iwl_mvm_txq_from_mac80211(txq);
	struct sk_buff *skb = NULL;
	u32 sofar = 0;

	/*
	 * No need for threads to be pending here, they can leave the first
	 * taker all the work.
	 *
	 * mvmtxq->tx_request logic:
	 *
	 * If 0, no one is currently TXing, set to 1 to indicate current thread
	 * will now start TX and other threads should quit.
	 *
	 * If 1, another thread is currently TXing, set to 2 to indicate to
	 * that thread that there was another request. Since that request may
	 * have raced with the check whether the queue is empty, the TXing
	 * thread should check the queue's status one more time before leaving.
	 * This check is done in order to not leave any TX hanging in the queue
	 * until the next TX invocation (which may not even happen).
	 *
	 * If 2, another thread is currently TXing, and it will already double
	 * check the queue, so do nothing.
	 */
	if (atomic_fetch_add_unless(&mvmtxq->tx_request, 1, 2))
		return;

	rcu_read_lock();
	do {
		while (likely(!test_bit(IWL_MVM_TXQ_STATE_STOP_FULL,
					&mvmtxq->state) &&
			      !test_bit(IWL_MVM_TXQ_STATE_STOP_REDIRECT,
					&mvmtxq->state) &&
			      !test_bit(IWL_MVM_TXQ_STATE_STOP_AP_CSA,
					&mvmtxq->state) &&
			      (sofar <= 1000) &&
			      !test_bit(IWL_MVM_STATUS_IN_D3, &mvm->status))) {
			skb = ieee80211_tx_dequeue(hw, txq);

			if (!skb) {
				if (txq->sta)
					IWL_DEBUG_TX(mvm,
						     "TXQ of sta %pM tid %d is now empty\n",
						     txq->sta->addr,
						     txq->tid);
				break;
			}

			iwl_mvm_tx_skb(mvm, skb, txq->sta);
			if (++sofar > 1000)
				pr_info("WARNING: Wrote %d packets in iwl_mvm_mac_itxq_xmit, tx_request: %d returning.\n",
					sofar, atomic_read(&mvmtxq->tx_request));
		}
	} while (atomic_dec_return(&mvmtxq->tx_request));
	rcu_read_unlock();
}

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com