On Fri, 2012-11-30 at 00:48 +0200, Arik Nemtsov wrote:

Start using the new hw_queue mechanism in mac80211 and give each AC in
each vif its own hw_queue number. This allows us to stop an AC in a vif
independently from other vifs.

Change the Tx watermark handling functions to count packets per AC in
vif. From now on fast links should not be able to hurt the throughput
of slow links on the same AC but on different vifs.

Change internal queue mgmt functions to operate per vif, to support the
new Tx watermark granularity. Make the global versions of the queue
stop/start functions to use the global mac80211 API for queue mgmt. This
helps in situations where the driver currently doesn't know all the vifs
that reside in mac80211. Recovery is a good example for such a case.

Signed-off-by: Arik Nemtsov <redacted>
---
v2: move to new vif iterator API

Thanks for respinning! :)

[...]

Can't you just use an int to pass the q_base that you use below? Am I
missing something or are you just using the bitmap to pass the vif
number?

+static int wlcore_allocate_hw_queue_base(struct wl1271 *wl,
+					 struct wl12xx_vif *wlvif)
+{
+	struct ieee80211_vif *vif = wl12xx_wlvif_to_vif(wlvif);
+	struct wlcore_hw_queue_iter_data iter_data = {};
+	int i, q_base;
+
+	iter_data.vif = vif;
+
+	/* mark all bits taken by active interfaces */
+	ieee80211_iterate_active_interfaces_atomic(wl->hw,
+					IEEE80211_IFACE_ITER_RESUME_ALL,
+					wlcore_hw_queue_iter, &iter_data);

The comment here is misleading.  You don't really mark all bits, you
mark bits one by one.  This iteration will only set a single bit.

/+	/* the current vif is already running in mac80211 (resume/recovery) */
+	if (iter_data.cur_running) {
+		wlvif->hw_queue_base = vif->hw_queue[0];
+		wl1271_debug(DEBUG_MAC80211,
+			     "using pre-allocated hw queue base %d",
+			     wlvif->hw_queue_base);
+
+		/* interface type might have changed type */
+		goto adjust_cab_queue;
+	}

This can probably be avoided with the new IEEE80211_IFACE_ITER_NORMAL,
can't it?

+	q_base = find_first_zero_bit(iter_data.hw_queue_map,
+				     WLCORE_NUM_MAC_ADDRESSES);
+	if (q_base >= WLCORE_NUM_MAC_ADDRESSES)
+		return -EBUSY;
+
+	wlvif->hw_queue_base = q_base * NUM_TX_QUEUES;
+	wl1271_debug(DEBUG_MAC80211, "allocating hw queue base: %d",
+		     wlvif->hw_queue_base);
+
+	for (i = 0; i < NUM_TX_QUEUES; i++) {
+		wl->queue_stop_reasons[wlvif->hw_queue_base + i] = 0;
+		/* register hw queues in mac80211 */
+		vif->hw_queue[i] = wlvif->hw_queue_base + i;
+	}
+
+adjust_cab_queue:
+	/* the last places are reserved for cab queues per interface */
+	if (wlvif->bss_type == BSS_TYPE_AP_BSS)
+		vif->cab_queue = NUM_TX_QUEUES * WLCORE_NUM_MAC_ADDRESSES +
+				 wlvif->hw_queue_base / NUM_TX_QUEUES;
+	else
+		vif->cab_queue = IEEE80211_INVAL_HW_QUEUE;

Why not keep the reservation per-vif in the same order (ie. the cab
queue comes together with the other queues for that vif)? It doesn't
matter that you don't use the cab_queue for the non AP vifs, since
you're allocating them anyway.

Keeping them together would make this code clearer.  Just make a macro
NUM_TX_QUEUES_PER_VIF that includes the space for cab_queue.

[...]

It would look nicer if we moved the "wlvif->hw_queue_base + " to the
wl1271_tx_get_mac80211_queue() function.  After all, the function call
without adding the hw_queue_base doesn't mean anything anymore.

I'll change this when applying.

-void wlcore_wake_queue(struct wl1271 *wl, u8 queue,
+void wlcore_wake_queue(struct wl1271 *wl, struct wl12xx_vif *wlvif, u8 queue,
 		       enum wlcore_queue_stop_reason reason)
 {
 	unsigned long flags;
+	int hwq = wlvif->hw_queue_base + wl1271_tx_get_mac80211_queue(queue);
 
 	spin_lock_irqsave(&wl->wl_lock, flags);
-

Stray line removal, I'll insert it back.

[...]

This looks a bit hacky.  You'll "stop queues" even for vifs that were
not added, won't you? Also, you can do with a single for-loop here ;)

I'm also not sure about the WARN_ON.  If one of the queues is already
stopped, should we really warn? I see that, currently, the reasons to
stop all queues are not used to stop a single queue, but still. :)

[...]

-void wlcore_reset_stopped_queues(struct wl1271 *wl)
+void wlcore_wake_queues(struct wl1271 *wl,
+			enum wlcore_queue_stop_reason reason)
 {
-	int i;
+	int i, q_base;
 	unsigned long flags;
 
 	spin_lock_irqsave(&wl->wl_lock, flags);
 
-	for (i = 0; i < NUM_TX_QUEUES; i++) {
-		if (!wl->queue_stop_reasons[i])
-			continue;
+	for (q_base = 0; q_base < WLCORE_NUM_MAC_ADDRESSES; q_base++)
+		for (i = 0; i < NUM_TX_QUEUES; i++) {
+			int h = q_base * NUM_TX_QUEUES + i;
+			WARN_ON(!test_and_clear_bit(reason,
+						&wl->queue_stop_reasons[h]));
+		}

Same thing here.  Actually, wouldn't it make more sense to have a
separate bitmask for the "full-stops"? In that way, at least it would be
very clear that the full-stop reasons (eg. flush, restart...) cannot be
used with single queues.

--
Luca.