2008-02-01 16:47:47

by Johannes Berg

[permalink] [raw]
Subject: mac80211 QoS/aggregation questions, thoughts

Hi Ron,

I just noticed this:

/* try to get a Qdisc from the pool */
for (i = IEEE80211_TX_QUEUE_BEACON; i < local->hw.queues; i++)
if (!test_and_set_bit(i, &q->qdisc_pool)) {

shouldn't that be i = IEEE80211_TX_QUEUE_BEACON + 1?

Or, more likely, something like adding
IEEE80211_TX_QUEUE_FIRST_AMPDU = 8,

to enum ieee80211_tx_queue and using that above? This way it seems it'll
use the "beacon" queue which feels wrong, although that queue is only
"used" by mac80211 to configure IBSS beacon parameters...

To be honest, I'm totally unclear on how all this queue stuff is
supposed to work. Ivo pointed me to IEEE80211_TX_QUEUE_AFTER_BEACON
which is unused and duplicated with IEEE80211_TXCTL_SEND_AFTER_DTIM.

It seems to me that this queue stuff is mostly designed after atheros
hardware and we should be getting rid of it...

Ultimately, we only need four data queues as required by WMM and then
A-MPDU queues. Right now, you can only use 12 of the 16 queues iwl4965
hw offers (well actually 13 but I think using the BEACON queue ID is
wrong so 12 if that mistake is corrected) as far as I can tell, because
the qdisc_pool is only searched starting with IEEE80211_TX_QUEUE_BEACON
(but see above).

Here's a possible plan:

1. move queue configuration for data queues to bss-config
structure, it really is part of the bss configuration since it
is mandated by the AP
2. get rid of IEEE80211_TX_QUEUE_BEACON, it isn't used in mac80211
and it's not hardware-independent, not all hardware actually
uses a queue for beacons (b43 for example does beaconing
differently), those drivers that do use a queue will have to do
that internally. Also, the one place where it is used is the
queue configuration for IBSS beacons, but that somewhat bogus
anyway since we never reset that should we go back into AP mode
after being in IBSS! Hence, I think the driver should handle
that when an interface is brought up. Ivo, any comments?
3. get rid of IEEE80211_TX_QUEUE_AFTER_BEACON, we have
IEEE80211_TXCTL_SEND_AFTER_DTIM now and that is
hardware-independent
4. remove IEEE80211_TX_QUEUE_SVP, it's something strange and
atheros specific
5. remove IEEE80211_TX_QUEUE_DATA4, only rt61pci uses that and that
use is strange, Ivo? We never submit frames to that queue...

This would leave us with the regular WMM four data queues. When hardware
announces more than four queues we would assume that the remaining
queues are usable for aggregation so that drivers for hardware that
needs a beacon queue (for example) would have to subtract that from the
number of available queues announced to mac80211.

An alternative that allows us to utilize hardware with more queues fully
w/o aggregation would be to have the drivers declare 'regular' and
'ampdu' queues in separate fields. Then, we'd be able to write the
multiqueue netdev support in a way that allows registers a queue for
each 'regular' queue.

That would possibly allow something I've thought about earlier: letting
users configure the queues with tc(8), i.e. having some sort of fixed
qdisc that is always on top of the master netdev and that is a simple
one-to-one mapping of bands to the hw queues, but allows per-band
configuration (like pfifo's limit) that sets the queue parameters, i.e.
AIFS, CW_min/max and burst time.

The effect of this would be to sort of externalise all of WMM support
into the qdisc configuration, by default you'd have a filter attached
that does the WMM classification. Of course, when operating in STA mode,
the queue parameters cannot be changed, but that can be handled in the
qdisc.

That seems mostly sane to me, opinions? We might have to write a new tc
module to allow accessing these things (although in STA mode it's not
changeable and in AP mode hostapd wants full control), but we should be
able to dump the config anyway. Also, I'm not sure whether it is
possible to make a qdisc non-removable, but that shouldn't be too hard
to add even if it is not currently possible. The simple 'wireless fifo'
qdisc that controls the queue parameters should always be present.

Phew, enough thinking for now, I'll wait for comments.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-01 23:24:14

by Tomas Winkler

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

On Feb 1, 2008 11:43 PM, Johannes Berg <[email protected]> wrote:
> Any thoughts on this patch? The only thing it should really actually
> change is removing the IBSS beacon queue configuration, but as I've
> explained previously that really has to be done by the driver because we
> don't know whether the driver even uses queues for beaconing... Also we
> never reset that info.
>
> Also, I'm wondering, iwl4965 has 16 queues, can it use 4 for regular WMM
> and the other 12 for aggregation? And how does it configure the access
> parameters for aggregation? I don't see conf_tx() calls anywhere for
> that.
>

4965 has 4 queues which are used for regular WMM. Only 11 queues
might be used for aggregation, but in AP mode we might map one of the
queues to after beacon multicast traffic.
One queue is reserved for communicating with FW. The number my differ
on further NICs.

All the traffic queues 4 WMM + 11 AGG are served according regular WMM
policy, 4 AC - these are configured by conf_tx. The aggregation
queues are needed for handling ordering of frames correctly and not
for on media prioritizing

Sorry but I have to review your patch later
Thanks
Tomas



> johannes
>
> ---
> drivers/net/wireless/rt2x00/rt2400pci.c | 6 +++---
> drivers/net/wireless/rt2x00/rt2500pci.c | 8 ++++----
> drivers/net/wireless/rt2x00/rt2500usb.c | 10 ++++------
> drivers/net/wireless/rt2x00/rt2x00.h | 6 ++++++
> drivers/net/wireless/rt2x00/rt2x00dev.c | 12 ++++++------
> drivers/net/wireless/rt2x00/rt2x00pci.c | 7 +++----
> drivers/net/wireless/rt2x00/rt61pci.c | 19 ++++++++++---------
> drivers/net/wireless/rt2x00/rt73usb.c | 10 ++++------
> include/net/mac80211.h | 24 +++++-------------------
> net/mac80211/debugfs.c | 4 ++--
> net/mac80211/debugfs_sta.c | 4 ++--
> net/mac80211/ieee80211_i.h | 4 ++--
> net/mac80211/ieee80211_sta.c | 10 ++--------
> net/mac80211/wme.c | 4 ++--
> 14 files changed, 55 insertions(+), 73 deletions(-)
>
> --- everything.orig/include/net/mac80211.h 2008-02-01 21:49:26.168180664 +0100
> +++ everything/include/net/mac80211.h 2008-02-01 21:56:28.158178981 +0100
> @@ -127,34 +127,20 @@ struct ieee80211_tx_queue_stats_data {
> * @IEEE80211_TX_QUEUE_DATA1: data queue 1
> * @IEEE80211_TX_QUEUE_DATA2: data queue 2
> * @IEEE80211_TX_QUEUE_DATA3: data queue 3
> - * @IEEE80211_TX_QUEUE_DATA4: data queue 4
> - * @IEEE80211_TX_QUEUE_SVP: ??
> - * @NUM_TX_DATA_QUEUES: number of data queues
> - * @IEEE80211_TX_QUEUE_AFTER_BEACON: transmit queue for frames to be
> - * sent after a beacon
> - * @IEEE80211_TX_QUEUE_BEACON: transmit queue for beacon frames
> - * @NUM_TX_DATA_QUEUES_AMPDU: adding more queues for A-MPDU
> + * @IEEE80211_NUM_TX_DATA_QUEUES: number of data queues
> + * @IEEE80211_NUM_TX_DATA_QUEUES_AMPDU: adding more queues for A-MPDU
> */
> enum ieee80211_tx_queue {
> IEEE80211_TX_QUEUE_DATA0,
> IEEE80211_TX_QUEUE_DATA1,
> IEEE80211_TX_QUEUE_DATA2,
> IEEE80211_TX_QUEUE_DATA3,
> - IEEE80211_TX_QUEUE_DATA4,
> - IEEE80211_TX_QUEUE_SVP,
> -
> - NUM_TX_DATA_QUEUES,
> -
> -/* due to stupidity in the sub-ioctl userspace interface, the items in
> - * this struct need to have fixed values. As soon as it is removed, we can
> - * fix these entries. */
> - IEEE80211_TX_QUEUE_AFTER_BEACON = 6,
> - IEEE80211_TX_QUEUE_BEACON = 7,
> - NUM_TX_DATA_QUEUES_AMPDU = 16
> + IEEE80211_NUM_TX_DATA_QUEUES,
> + IEEE80211_NUM_TX_DATA_QUEUES_AMPDU = 16,
> };
>
> struct ieee80211_tx_queue_stats {
> - struct ieee80211_tx_queue_stats_data data[NUM_TX_DATA_QUEUES_AMPDU];
> + struct ieee80211_tx_queue_stats_data data[IEEE80211_NUM_TX_DATA_QUEUES_AMPDU];
> };
>
> struct ieee80211_low_level_stats {
> --- everything.orig/net/mac80211/ieee80211_i.h 2008-02-01 21:56:41.678182617 +0100
> +++ everything/net/mac80211/ieee80211_i.h 2008-02-01 21:57:13.008180501 +0100
> @@ -450,8 +450,8 @@ struct ieee80211_local {
> struct sta_info *sta_hash[STA_HASH_SIZE];
> struct timer_list sta_cleanup;
>
> - unsigned long state[NUM_TX_DATA_QUEUES_AMPDU];
> - struct ieee80211_tx_stored_packet pending_packet[NUM_TX_DATA_QUEUES_AMPDU];
> + unsigned long state[IEEE80211_NUM_TX_DATA_QUEUES_AMPDU];
> + struct ieee80211_tx_stored_packet pending_packet[IEEE80211_NUM_TX_DATA_QUEUES_AMPDU];
> struct tasklet_struct tx_pending_tasklet;
>
> /* number of interfaces with corresponding IFF_ flags */
> --- everything.orig/net/mac80211/ieee80211_sta.c 2008-02-01 21:52:05.988181315 +0100
> +++ everything/net/mac80211/ieee80211_sta.c 2008-02-01 21:58:02.408180935 +0100
> @@ -3231,17 +3231,11 @@ int ieee80211_sta_set_ssid(struct net_de
> qparam.cw_max = 1023;
> qparam.burst_time = 0;
>
> - for (i = IEEE80211_TX_QUEUE_DATA0; i < NUM_TX_DATA_QUEUES; i++)
> + for (i = IEEE80211_TX_QUEUE_DATA0;
> + i < IEEE80211_NUM_TX_DATA_QUEUES; i++)
> local->ops->conf_tx(local_to_hw(local),
> i + IEEE80211_TX_QUEUE_DATA0,
> &qparam);
> -
> - /* IBSS uses different parameters for Beacon sending */
> - qparam.cw_min++;
> - qparam.cw_min *= 2;
> - qparam.cw_min--;
> - local->ops->conf_tx(local_to_hw(local),
> - IEEE80211_TX_QUEUE_BEACON, &qparam);
> }
>
> ifsta = &sdata->u.sta;
> --- everything.orig/net/mac80211/wme.c 2008-02-01 21:54:35.928182237 +0100
> +++ everything/net/mac80211/wme.c 2008-02-01 21:57:42.178178982 +0100
> @@ -406,7 +406,7 @@ static int wme_qdiscop_init(struct Qdisc
> }
>
> /* reserve all legacy QoS queues */
> - for (i = 0; i < min(IEEE80211_TX_QUEUE_DATA4, queues); i++)
> + for (i = 0; i < min(IEEE80211_NUM_TX_DATA_QUEUES, queues); i++)
> set_bit(i, &q->qdisc_pool);
>
> return err;
> @@ -667,7 +667,7 @@ int ieee80211_ht_agg_queue_add(struct ie
> * matching the recieved HW queue */
>
> /* try to get a Qdisc from the pool */
> - for (i = IEEE80211_TX_QUEUE_BEACON; i < local->hw.queues; i++)
> + for (i = IEEE80211_NUM_TX_DATA_QUEUES; i < local->hw.queues; i++)
> if (!test_and_set_bit(i, &q->qdisc_pool)) {
> ieee80211_stop_queue(local_to_hw(local), i);
> sta->tid_to_tx_q[tid] = i;
> --- everything.orig/net/mac80211/debugfs.c 2008-02-01 21:58:36.578182400 +0100
> +++ everything/net/mac80211/debugfs.c 2008-02-01 22:00:02.748177137 +0100
> @@ -222,10 +222,10 @@ static ssize_t stats_wme_tx_queue_read(s
> size_t count, loff_t *ppos)
> {
> struct ieee80211_local *local = file->private_data;
> - char buf[NUM_TX_DATA_QUEUES*15], *p = buf;
> + char buf[IEEE80211_NUM_TX_DATA_QUEUES * 15], *p = buf;
> int i;
>
> - for (i = 0; i < NUM_TX_DATA_QUEUES; i++)
> + for (i = 0; i < IEEE80211_NUM_TX_DATA_QUEUES; i++)
> p += scnprintf(p, sizeof(buf)+buf-p,
> "%u\n", local->wme_tx_queue[i]);
>
> --- everything.orig/net/mac80211/debugfs_sta.c 2008-02-01 21:59:01.668182074 +0100
> +++ everything/net/mac80211/debugfs_sta.c 2008-02-01 21:59:11.768178819 +0100
> @@ -167,10 +167,10 @@ STA_OPS(wme_rx_queue);
> static ssize_t sta_wme_tx_queue_read(struct file *file, char __user *userbuf,
> size_t count, loff_t *ppos)
> {
> - char buf[15*NUM_TX_DATA_QUEUES], *p = buf;
> + char buf[15 * IEEE80211_NUM_TX_DATA_QUEUES], *p = buf;
> int i;
> struct sta_info *sta = file->private_data;
> - for (i = 0; i < NUM_TX_DATA_QUEUES; i++)
> + for (i = 0; i < IEEE80211_NUM_TX_DATA_QUEUES; i++)
> p += scnprintf(p, sizeof(buf)+buf-p, "%u ",
> sta->wme_tx_queue[i]);
> p += scnprintf(p, sizeof(buf)+buf-p, "\n");
> --- everything.orig/drivers/net/wireless/rt2x00/rt2400pci.c 2008-02-01 22:07:51.948181639 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2400pci.c 2008-02-01 22:08:23.088177517 +0100
> @@ -1040,7 +1040,7 @@ static void rt2400pci_kick_tx_queue(stru
> {
> u32 reg;
>
> - if (queue == IEEE80211_TX_QUEUE_BEACON) {
> + if (queue == RT2X00_QUEUE_BEACON) {
> rt2x00pci_register_read(rt2x00dev, CSR14, &reg);
> if (!rt2x00_get_field32(reg, CSR14_BEACON_GEN)) {
> rt2x00_set_field32(&reg, CSR14_BEACON_GEN, 1);
> @@ -1055,7 +1055,7 @@ static void rt2400pci_kick_tx_queue(stru
> rt2x00_set_field32(&reg, TXCSR0_KICK_TX,
> (queue == IEEE80211_TX_QUEUE_DATA1));
> rt2x00_set_field32(&reg, TXCSR0_KICK_ATIM,
> - (queue == IEEE80211_TX_QUEUE_AFTER_BEACON));
> + (queue == RT2X00_QUEUE_BCMC));
> rt2x00pci_register_write(rt2x00dev, TXCSR0, reg);
> }
>
> @@ -1160,7 +1160,7 @@ static irqreturn_t rt2400pci_interrupt(i
> * 3 - Atim ring transmit done interrupt.
> */
> if (rt2x00_get_field32(reg, CSR7_TXDONE_ATIMRING))
> - rt2400pci_txdone(rt2x00dev, IEEE80211_TX_QUEUE_AFTER_BEACON);
> + rt2400pci_txdone(rt2x00dev, RT2X00_QUEUE_BCMC);
>
> /*
> * 4 - Priority ring transmit done interrupt.
> --- everything.orig/drivers/net/wireless/rt2x00/rt2500pci.c 2008-02-01 22:08:36.438230143 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2500pci.c 2008-02-01 22:09:05.988178765 +0100
> @@ -275,7 +275,7 @@ static void rt2500pci_config_type(struct
> PREAMBLE + get_duration(IEEE80211_HEADER, 20));
> rt2x00_set_field32(&reg, BCNCSR1_BEACON_CWMIN,
> rt2x00lib_get_ring(rt2x00dev,
> - IEEE80211_TX_QUEUE_BEACON)
> + RT2X00_QUEUE_BEACON)
> ->tx_params.cw_min);
> rt2x00pci_register_write(rt2x00dev, BCNCSR1, reg);
>
> @@ -1192,7 +1192,7 @@ static void rt2500pci_kick_tx_queue(stru
> {
> u32 reg;
>
> - if (queue == IEEE80211_TX_QUEUE_BEACON) {
> + if (queue == RT2X00_QUEUE_BEACON) {
> rt2x00pci_register_read(rt2x00dev, CSR14, &reg);
> if (!rt2x00_get_field32(reg, CSR14_BEACON_GEN)) {
> rt2x00_set_field32(&reg, CSR14_BEACON_GEN, 1);
> @@ -1207,7 +1207,7 @@ static void rt2500pci_kick_tx_queue(stru
> rt2x00_set_field32(&reg, TXCSR0_KICK_TX,
> (queue == IEEE80211_TX_QUEUE_DATA1));
> rt2x00_set_field32(&reg, TXCSR0_KICK_ATIM,
> - (queue == IEEE80211_TX_QUEUE_AFTER_BEACON));
> + (queue == RT2X00_QUEUE_BCMC));
> rt2x00pci_register_write(rt2x00dev, TXCSR0, reg);
> }
>
> @@ -1309,7 +1309,7 @@ static irqreturn_t rt2500pci_interrupt(i
> * 3 - Atim ring transmit done interrupt.
> */
> if (rt2x00_get_field32(reg, CSR7_TXDONE_ATIMRING))
> - rt2500pci_txdone(rt2x00dev, IEEE80211_TX_QUEUE_AFTER_BEACON);
> + rt2500pci_txdone(rt2x00dev, RT2X00_QUEUE_BCMC);
>
> /*
> * 4 - Priority ring transmit done interrupt.
> --- everything.orig/drivers/net/wireless/rt2x00/rt2500usb.c 2008-02-01 22:12:41.858182942 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2500usb.c 2008-02-01 22:13:04.698180447 +0100
> @@ -1088,7 +1088,7 @@ static void rt2500usb_kick_tx_queue(stru
> {
> u16 reg;
>
> - if (queue != IEEE80211_TX_QUEUE_BEACON)
> + if (queue != RT2X00_QUEUE_BEACON)
> return;
>
> rt2500usb_register_read(rt2x00dev, TXRX_CSR19, &reg);
> @@ -1697,11 +1697,9 @@ static int rt2500usb_beacon_update(struc
> int length;
>
> /*
> - * Just in case the ieee80211 doesn't set this,
> - * but we need this queue set for the descriptor
> - * initialization.
> + * mac80211 doesn't set this but we use a queue for beacons
> */
> - control->queue = IEEE80211_TX_QUEUE_BEACON;
> + control->queue = RT2X00_QUEUE_BEACON;
> ring = rt2x00lib_get_ring(rt2x00dev, control->queue);
>
> /*
> @@ -1758,7 +1756,7 @@ static int rt2500usb_beacon_update(struc
> /*
> * Enable beacon generation.
> */
> - rt2500usb_kick_tx_queue(rt2x00dev, IEEE80211_TX_QUEUE_BEACON);
> + rt2500usb_kick_tx_queue(rt2x00dev, RT2X00_QUEUE_BEACON);
>
> return 0;
> }
> --- everything.orig/drivers/net/wireless/rt2x00/rt2x00.h 2008-02-01 22:05:11.038210937 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2x00.h 2008-02-01 22:09:22.358177843 +0100
> @@ -100,6 +100,12 @@
> #define MGMT_FRAME_SIZE 256
>
> /*
> + * Internal queue numbers for beacon and buffered broadcast/multicast traffic.
> + */
> +#define RT2X00_QUEUE_BEACON 100
> +#define RT2X00_QUEUE_BCMC 101
> +
> +/*
> * Number of entries in a packet ring.
> * PCI devices only need 1 Beacon entry,
> * but USB devices require a second because they
> --- everything.orig/drivers/net/wireless/rt2x00/rt2x00dev.c 2008-02-01 22:02:56.938182780 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2x00dev.c 2008-02-01 22:06:28.338180393 +0100
> @@ -49,9 +49,9 @@ struct data_ring *rt2x00lib_get_ring(str
> if (!rt2x00dev->bcn || !beacon)
> return NULL;
>
> - if (queue == IEEE80211_TX_QUEUE_BEACON)
> + if (queue == RT2X00_QUEUE_BEACON)
> return &rt2x00dev->bcn[0];
> - else if (queue == IEEE80211_TX_QUEUE_AFTER_BEACON)
> + else if (queue == RT2X00_QUEUE_BCMC)
> return &rt2x00dev->bcn[1];
>
> return NULL;
> @@ -502,7 +502,7 @@ static void rt2x00lib_beacondone_schedul
> struct rt2x00_dev *rt2x00dev =
> container_of(work, struct rt2x00_dev, beacon_work);
> struct data_ring *ring =
> - rt2x00lib_get_ring(rt2x00dev, IEEE80211_TX_QUEUE_BEACON);
> + rt2x00lib_get_ring(rt2x00dev, RT2X00_QUEUE_BEACON);
> struct data_entry *entry = rt2x00_get_data_entry(ring);
> struct sk_buff *skb;
>
> @@ -665,8 +665,8 @@ void rt2x00lib_write_tx_desc(struct rt2x
> */
> if (control->queue < rt2x00dev->hw->queues)
> desc.queue = control->queue;
> - else if (control->queue == IEEE80211_TX_QUEUE_BEACON ||
> - control->queue == IEEE80211_TX_QUEUE_AFTER_BEACON)
> + else if (control->queue == RT2X00_QUEUE_BEACON ||
> + control->queue == RT2X00_QUEUE_BCMC)
> desc.queue = QUEUE_MGMT;
> else
> desc.queue = QUEUE_OTHER;
> @@ -717,7 +717,7 @@ void rt2x00lib_write_tx_desc(struct rt2x
> * Beacons and probe responses require the tsf timestamp
> * to be inserted into the frame.
> */
> - if (control->queue == IEEE80211_TX_QUEUE_BEACON ||
> + if (control->queue == RT2X00_QUEUE_BEACON ||
> is_probe_resp(frame_control))
> __set_bit(ENTRY_TXD_REQ_TIMESTAMP, &desc.flags);
>
> --- everything.orig/drivers/net/wireless/rt2x00/rt2x00pci.c 2008-02-01 22:07:08.708181857 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2x00pci.c 2008-02-01 22:07:41.538180176 +0100
> @@ -43,11 +43,10 @@ int rt2x00pci_beacon_update(struct ieee8
> struct data_entry *entry;
>
> /*
> - * Just in case mac80211 doesn't set this correctly,
> - * but we need this queue set for the descriptor
> - * initialization.
> + * mac80211 doesn't set the queue but we use an internal
> + * number here that we later translate to the right thing.
> */
> - control->queue = IEEE80211_TX_QUEUE_BEACON;
> + control->queue = RT2X00_QUEUE_BEACON;
> ring = rt2x00lib_get_ring(rt2x00dev, control->queue);
> entry = rt2x00_get_data_entry(ring);
>
> --- everything.orig/drivers/net/wireless/rt2x00/rt61pci.c 2008-02-01 22:09:43.378180447 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt61pci.c 2008-02-01 22:12:26.208178873 +0100
> @@ -1049,8 +1049,9 @@ static int rt61pci_init_rings(struct rt2
> rt2x00pci_register_write(rt2x00dev, TX_RING_CSR0, reg);
>
> rt2x00pci_register_read(rt2x00dev, TX_RING_CSR1, &reg);
> + /* XXX: what is this fifth queue? */
> rt2x00_set_field32(&reg, TX_RING_CSR1_MGMT_RING_SIZE,
> - rt2x00dev->tx[IEEE80211_TX_QUEUE_DATA4].stats.limit);
> + rt2x00dev->tx[4].stats.limit);
> rt2x00_set_field32(&reg, TX_RING_CSR1_TXD_SIZE,
> rt2x00dev->tx[IEEE80211_TX_QUEUE_DATA0].desc_size /
> 4);
> @@ -1077,8 +1078,9 @@ static int rt61pci_init_rings(struct rt2
> rt2x00pci_register_write(rt2x00dev, AC3_BASE_CSR, reg);
>
> rt2x00pci_register_read(rt2x00dev, MGMT_BASE_CSR, &reg);
> + /* fifth queue again? */
> rt2x00_set_field32(&reg, MGMT_BASE_CSR_RING_REGISTER,
> - rt2x00dev->tx[IEEE80211_TX_QUEUE_DATA4].data_dma);
> + rt2x00dev->tx[4].data_dma);
> rt2x00pci_register_write(rt2x00dev, MGMT_BASE_CSR, reg);
>
> rt2x00pci_register_read(rt2x00dev, RX_RING_CSR, &reg);
> @@ -1572,7 +1574,7 @@ static void rt61pci_kick_tx_queue(struct
> {
> u32 reg;
>
> - if (queue == IEEE80211_TX_QUEUE_BEACON) {
> + if (queue == RT2X00_QUEUE_BEACON) {
> /*
> * For Wi-Fi faily generated beacons between participating
> * stations. Set TBTT phase adaptive adjustment step to 8us.
> @@ -1596,8 +1598,9 @@ static void rt61pci_kick_tx_queue(struct
> (queue == IEEE80211_TX_QUEUE_DATA2));
> rt2x00_set_field32(&reg, TX_CNTL_CSR_KICK_TX_AC3,
> (queue == IEEE80211_TX_QUEUE_DATA3));
> + /* fifth queue? never used in mac80211 right now... */
> rt2x00_set_field32(&reg, TX_CNTL_CSR_KICK_TX_MGMT,
> - (queue == IEEE80211_TX_QUEUE_DATA4));
> + (queue == 4));
> rt2x00pci_register_write(rt2x00dev, TX_CNTL_CSR, reg);
> }
>
> @@ -2383,11 +2386,9 @@ static int rt61pci_beacon_update(struct
> struct data_entry *entry;
>
> /*
> - * Just in case the ieee80211 doesn't set this,
> - * but we need this queue set for the descriptor
> - * initialization.
> + * mac80211 doesn't set this but we need it
> */
> - control->queue = IEEE80211_TX_QUEUE_BEACON;
> + control->queue = RT2X00_QUEUE_BEACON;
> ring = rt2x00lib_get_ring(rt2x00dev, control->queue);
> entry = rt2x00_get_data_entry(ring);
>
> @@ -2427,7 +2428,7 @@ static int rt61pci_beacon_update(struct
> */
> rt2x00pci_register_multiwrite(rt2x00dev, HW_BEACON_BASE0,
> skb->data, skb->len);
> - rt61pci_kick_tx_queue(rt2x00dev, IEEE80211_TX_QUEUE_BEACON);
> + rt61pci_kick_tx_queue(rt2x00dev, RT2X00_QUEUE_BEACON);
>
> return 0;
> }
> --- everything.orig/drivers/net/wireless/rt2x00/rt73usb.c 2008-02-01 22:13:24.188182129 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt73usb.c 2008-02-01 22:13:55.278180827 +0100
> @@ -1310,7 +1310,7 @@ static void rt73usb_kick_tx_queue(struct
> {
> u32 reg;
>
> - if (queue != IEEE80211_TX_QUEUE_BEACON)
> + if (queue != RT2X00_QUEUE_BEACON)
> return;
>
> /*
> @@ -1970,11 +1970,9 @@ static int rt73usb_beacon_update(struct
> int timeout;
>
> /*
> - * Just in case the ieee80211 doesn't set this,
> - * but we need this queue set for the descriptor
> - * initialization.
> + * mac80211 doesn't set this but we use a queue for beacons
> */
> - control->queue = IEEE80211_TX_QUEUE_BEACON;
> + control->queue = RT2X00_QUEUE_BEACON;
> ring = rt2x00lib_get_ring(rt2x00dev, control->queue);
> entry = rt2x00_get_data_entry(ring);
>
> @@ -2006,7 +2004,7 @@ static int rt73usb_beacon_update(struct
> USB_VENDOR_REQUEST_OUT,
> HW_BEACON_BASE0, 0x0000,
> skb->data, skb->len, timeout);
> - rt73usb_kick_tx_queue(rt2x00dev, IEEE80211_TX_QUEUE_BEACON);
> + rt73usb_kick_tx_queue(rt2x00dev, RT2X00_QUEUE_BEACON);
>
> return 0;
> }
>
>

2008-02-01 23:30:12

by Johannes Berg

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts


> 4965 has 4 queues which are used for regular WMM. Only 11 queues
> might be used for aggregation, but in AP mode we might map one of the
> queues to after beacon multicast traffic.
> One queue is reserved for communicating with FW. The number my differ
> on further NICs.

Ok, makes sense.

> All the traffic queues 4 WMM + 11 AGG are served according regular WMM
> policy, 4 AC - these are configured by conf_tx. The aggregation
> queues are needed for handling ordering of frames correctly and not
> for on media prioritizing

Yeah I realise those are just for the right ordering (which is
interesting btw, Broadcom gets away without extra queues, not sure yet
how), but I those have to have media access too and I wonder how that is
configured. But it's not too important right now.

I'm trying to grasp the qdisc/multiqueue netdev stuff right now, it's
proving harder than I expected :)

> Sorry but I have to review your patch later

No worries, I just whipped that up as proof-of-concept.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-03 15:55:22

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

On Sunday 03 February 2008, Guy Cohen wrote:
> On 2/2/08, Ivo van Doorn <[email protected]> wrote:
> > On Saturday 02 February 2008, Tomas Winkler wrote:
> > > 4965 has 4 queues which are used for regular WMM. Only 11 queues
> > > might be used for aggregation, but in AP mode we might map one of the
> > > queues to after beacon multicast traffic.
> >
> > Just out of interest, and I don't know about the iwl4965 driver specifics,
> > but how will you make sure those frames are send after the beacon?
> > Does the beacon trigger an interrupt after each interval which allows you to queue
> > the frames or is some other approach used?
>
> For iwl4965, mcast frames are queued into a separate HW TX FIFO (no
> need to buffer at the host). Beacon periodic transmission and
> controling the TX of mcast frames from the mcast FIFO are all timely
> done in real-time by the device.

Thanks for the explanation.

Ivo

2008-02-02 02:40:45

by Nick Kossifidis

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

>
> To be honest, I'm totally unclear on how all this queue stuff is
> supposed to work. Ivo pointed me to IEEE80211_TX_QUEUE_AFTER_BEACON
> which is unused and duplicated with IEEE80211_TXCTL_SEND_AFTER_DTIM.
>
> It seems to me that this queue stuff is mostly designed after atheros
> hardware and we should be getting rid of it...
>

Atheros hw has 12 hw queues since 5211, one is used already for
beacons inside ath5k (check out beacon_config/setup etc functions
inside hw.c), one called CAB ("crap after beacon" i think), and 10
data queues (newer hw might have more queues -madwifi btw only
includes 10 out of 12 queues on HAL's headers). I guess they can go
away since we have beacon_update callback from stack (we know what
frames to put in that queue anyway and cwmin/max parameters for
beacons are standard i guess).

> Ultimately, we only need four data queues as required by WMM and then
> A-MPDU queues. Right now, you can only use 12 of the 16 queues iwl4965
> hw offers (well actually 13 but I think using the BEACON queue ID is
> wrong so 12 if that mistake is corrected) as far as I can tell, because
> the qdisc_pool is only searched starting with IEEE80211_TX_QUEUE_BEACON
> (but see above).
>
> Here's a possible plan:
>
> 1. move queue configuration for data queues to bss-config
> structure, it really is part of the bss configuration since it
> is mandated by the AP

ACK, btw why do we have a separate ht_bss_conf which is handled by
bss_info_changed instead of bss_info_changed or conf_ht ?

> 2. get rid of IEEE80211_TX_QUEUE_BEACON, it isn't used in mac80211
> and it's not hardware-independent, not all hardware actually
> uses a queue for beacons (b43 for example does beaconing
> differently), those drivers that do use a queue will have to do
> that internally. Also, the one place where it is used is the
> queue configuration for IBSS beacons, but that somewhat bogus
> anyway since we never reset that should we go back into AP mode
> after being in IBSS! Hence, I think the driver should handle
> that when an interface is brought up. Ivo, any comments?

ACK

> 3. get rid of IEEE80211_TX_QUEUE_AFTER_BEACON, we have
> IEEE80211_TXCTL_SEND_AFTER_DTIM now and that is
> hardware-independent

ACK

> 4. remove IEEE80211_TX_QUEUE_SVP, it's something strange and
> atheros specific

ACK spectralink voice protocol, some voip related stuff i don't know
anything about, i just found it's definition on some old d80211 header
while porting openhal to dadwifi and wanted to make openhal's code
more portable across stacks so i added the comment on ath5k.h to note
the mapping.

> 5. remove IEEE80211_TX_QUEUE_DATA4, only rt61pci uses that and that
> use is strange, Ivo? We never submit frames to that queue...
>

ACK and btw let's also rename them, DATA* is not very informative,
something like IEEE80211_TX_QUEUE_BE would be better imho



--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

2008-02-01 22:25:50

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

On Friday 01 February 2008, Johannes Berg wrote:
> Any thoughts on this patch? The only thing it should really actually
> change is removing the IBSS beacon queue configuration, but as I've
> explained previously that really has to be done by the driver because we
> don't know whether the driver even uses queues for beaconing... Also we
> never reset that info.
>
> Also, I'm wondering, iwl4965 has 16 queues, can it use 4 for regular WMM
> and the other 12 for aggregation? And how does it configure the access
> parameters for aggregation? I don't see conf_tx() calls anywhere for
> that.

Like I mentioned earlier, ring/queue handling has received an overhaul,
so not much of the rt2x00 part of this patch will apply after the update.
However the change should be easier afterwards. ;)

> --- everything.orig/drivers/net/wireless/rt2x00/rt2x00.h 2008-02-01 22:05:11.038210937 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2x00.h 2008-02-01 22:09:22.358177843 +0100
> @@ -100,6 +100,12 @@
> #define MGMT_FRAME_SIZE 256
>
> /*
> + * Internal queue numbers for beacon and buffered broadcast/multicast traffic.
> + */
> +#define RT2X00_QUEUE_BEACON 100
> +#define RT2X00_QUEUE_BCMC 101

Personally I think these queue identifiers should be completely seperated from the ieee80211_queue
enumeration, and not be used inside a variable mixed.
But that will be implementation details for later, and something I should probably take care of. ;)


> +/*
> * Number of entries in a packet ring.
> * PCI devices only need 1 Beacon entry,
> * but USB devices require a second because they
> --- everything.orig/drivers/net/wireless/rt2x00/rt2x00dev.c 2008-02-01 22:02:56.938182780 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2x00dev.c 2008-02-01 22:06:28.338180393 +0100
> @@ -49,9 +49,9 @@ struct data_ring *rt2x00lib_get_ring(str
> if (!rt2x00dev->bcn || !beacon)
> return NULL;
>
> - if (queue == IEEE80211_TX_QUEUE_BEACON)
> + if (queue == RT2X00_QUEUE_BEACON)
> return &rt2x00dev->bcn[0];
> - else if (queue == IEEE80211_TX_QUEUE_AFTER_BEACON)
> + else if (queue == RT2X00_QUEUE_BCMC)
> return &rt2x00dev->bcn[1];
>
> return NULL;
> @@ -502,7 +502,7 @@ static void rt2x00lib_beacondone_schedul
> struct rt2x00_dev *rt2x00dev =
> container_of(work, struct rt2x00_dev, beacon_work);
> struct data_ring *ring =
> - rt2x00lib_get_ring(rt2x00dev, IEEE80211_TX_QUEUE_BEACON);
> + rt2x00lib_get_ring(rt2x00dev, RT2X00_QUEUE_BEACON);
> struct data_entry *entry = rt2x00_get_data_entry(ring);
> struct sk_buff *skb;
>
> @@ -665,8 +665,8 @@ void rt2x00lib_write_tx_desc(struct rt2x
> */
> if (control->queue < rt2x00dev->hw->queues)
> desc.queue = control->queue;
> - else if (control->queue == IEEE80211_TX_QUEUE_BEACON ||
> - control->queue == IEEE80211_TX_QUEUE_AFTER_BEACON)
> + else if (control->queue == RT2X00_QUEUE_BEACON ||
> + control->queue == RT2X00_QUEUE_BCMC)
> desc.queue = QUEUE_MGMT;
> else
> desc.queue = QUEUE_OTHER;
> @@ -717,7 +717,7 @@ void rt2x00lib_write_tx_desc(struct rt2x
> * Beacons and probe responses require the tsf timestamp
> * to be inserted into the frame.
> */
> - if (control->queue == IEEE80211_TX_QUEUE_BEACON ||
> + if (control->queue == RT2X00_QUEUE_BEACON ||
> is_probe_resp(frame_control))
> __set_bit(ENTRY_TXD_REQ_TIMESTAMP, &desc.flags);
>
> --- everything.orig/drivers/net/wireless/rt2x00/rt2x00pci.c 2008-02-01 22:07:08.708181857 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2x00pci.c 2008-02-01 22:07:41.538180176 +0100
> @@ -43,11 +43,10 @@ int rt2x00pci_beacon_update(struct ieee8
> struct data_entry *entry;
>
> /*
> - * Just in case mac80211 doesn't set this correctly,
> - * but we need this queue set for the descriptor
> - * initialization.
> + * mac80211 doesn't set the queue but we use an internal
> + * number here that we later translate to the right thing.
> */
> - control->queue = IEEE80211_TX_QUEUE_BEACON;
> + control->queue = RT2X00_QUEUE_BEACON;
> ring = rt2x00lib_get_ring(rt2x00dev, control->queue);
> entry = rt2x00_get_data_entry(ring);
>
> --- everything.orig/drivers/net/wireless/rt2x00/rt61pci.c 2008-02-01 22:09:43.378180447 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt61pci.c 2008-02-01 22:12:26.208178873 +0100
> @@ -1049,8 +1049,9 @@ static int rt61pci_init_rings(struct rt2
> rt2x00pci_register_write(rt2x00dev, TX_RING_CSR0, reg);
>
> rt2x00pci_register_read(rt2x00dev, TX_RING_CSR1, &reg);
> + /* XXX: what is this fifth queue? */
> rt2x00_set_field32(&reg, TX_RING_CSR1_MGMT_RING_SIZE,
> - rt2x00dev->tx[IEEE80211_TX_QUEUE_DATA4].stats.limit);
> + rt2x00dev->tx[4].stats.limit);

MGMT frames only, but since mac80211 doesn't handle seperate queues for management frames,
I never added a filter for routing management frames over the queue. Doing something like
that is a bad idea anyway, so this queue is just dummy unless there are uses for a
fifth TX queue.

Other then that, the patch is fine with me, but not this patch probably needs to be respinned after
my rt2x00 queue and virtual interfaces overhaul.

Ivo

2008-02-01 22:32:17

by Johannes Berg

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts


> Like I mentioned earlier, ring/queue handling has received an overhaul,
> so not much of the rt2x00 part of this patch will apply after the update.
> However the change should be easier afterwards. ;)

:)
Keep the patches coming. No need to test them in rt2x00.git first,
wireless-2.6 git is pretty broken anyway right now. Due to me,
unfortunately.

> Personally I think these queue identifiers should be completely seperated from the ieee80211_queue
> enumeration, and not be used inside a variable mixed.
> But that will be implementation details for later, and something I should probably take care of. ;)

Yeah, but this seemed easiest for now. We never use the beacon queue in
mac80211 and I believe that we shouldn't because not all hardware uses a
queue for beacons.

> Other then that, the patch is fine with me, but not this patch probably needs to be respinned after
> my rt2x00 queue and virtual interfaces overhaul.

Right, no worries, I'm not entirely sure about this patch anyway.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-01 23:00:47

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

On Friday 01 February 2008, Johannes Berg wrote:
>
> > Like I mentioned earlier, ring/queue handling has received an overhaul,
> > so not much of the rt2x00 part of this patch will apply after the update.
> > However the change should be easier afterwards. ;)
>
> :)
> Keep the patches coming. No need to test them in rt2x00.git first,
> wireless-2.6 git is pretty broken anyway right now. Due to me,
> unfortunately.

Ok. I'll try to implement LED classes this weekend. I already have the initial
patches for that ready, but those were not really correct. Ones those are in
I'll submit all patches as the 2.1.0 release.
If I release them all this weekend they will be compile tested only, and I would
like to have at least 1 run test to make sure no obvious NULL pointers or segmentation
faults are there during init.

> > Personally I think these queue identifiers should be completely seperated from the ieee80211_queue
> > enumeration, and not be used inside a variable mixed.
> > But that will be implementation details for later, and something I should probably take care of. ;)
>
> Yeah, but this seemed easiest for now. We never use the beacon queue in
> mac80211 and I believe that we shouldn't because not all hardware uses a
> queue for beacons.

I understand the change, and my comment was more a first idea for teh rt2x00 implementation. ;)
I think I'll try to remove the IEEE80211_TX_QUEUE_BEACON tomorrow already and setup the correct
implementation. That value is set as identifier by rt2x00 manually anyways (it never relies on
mac80211 passing that value, so moving it to a different identifier can easily be done seperately from
your patch. Saves you some time updating the patch anyway. :)


> > Other then that, the patch is fine with me, but not this patch probably needs to be respinned after
> > my rt2x00 queue and virtual interfaces overhaul.
>
> Right, no worries, I'm not entirely sure about this patch anyway.

Ivo

2008-02-02 07:21:29

by Johannes Berg

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts


> ACK, btw why do we have a separate ht_bss_conf which is handled by
> bss_info_changed instead of bss_info_changed or conf_ht ?

Historic reasons, it'll be gone soon I expect.

> ACK and btw let's also rename them, DATA* is not very informative,
> something like IEEE80211_TX_QUEUE_BE would be better imho

Well, I'm not entirely sure about that yet, hw with more queues might
allow people to configure them in a non-WMM way.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-01 21:43:48

by Johannes Berg

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

Any thoughts on this patch? The only thing it should really actually
change is removing the IBSS beacon queue configuration, but as I've
explained previously that really has to be done by the driver because we
don't know whether the driver even uses queues for beaconing... Also we
never reset that info.

Also, I'm wondering, iwl4965 has 16 queues, can it use 4 for regular WMM
and the other 12 for aggregation? And how does it configure the access
parameters for aggregation? I don't see conf_tx() calls anywhere for
that.

johannes

---
drivers/net/wireless/rt2x00/rt2400pci.c | 6 +++---
drivers/net/wireless/rt2x00/rt2500pci.c | 8 ++++----
drivers/net/wireless/rt2x00/rt2500usb.c | 10 ++++------
drivers/net/wireless/rt2x00/rt2x00.h | 6 ++++++
drivers/net/wireless/rt2x00/rt2x00dev.c | 12 ++++++------
drivers/net/wireless/rt2x00/rt2x00pci.c | 7 +++----
drivers/net/wireless/rt2x00/rt61pci.c | 19 ++++++++++---------
drivers/net/wireless/rt2x00/rt73usb.c | 10 ++++------
include/net/mac80211.h | 24 +++++-------------------
net/mac80211/debugfs.c | 4 ++--
net/mac80211/debugfs_sta.c | 4 ++--
net/mac80211/ieee80211_i.h | 4 ++--
net/mac80211/ieee80211_sta.c | 10 ++--------
net/mac80211/wme.c | 4 ++--
14 files changed, 55 insertions(+), 73 deletions(-)

--- everything.orig/include/net/mac80211.h 2008-02-01 21:49:26.168180664 +0100
+++ everything/include/net/mac80211.h 2008-02-01 21:56:28.158178981 +0100
@@ -127,34 +127,20 @@ struct ieee80211_tx_queue_stats_data {
* @IEEE80211_TX_QUEUE_DATA1: data queue 1
* @IEEE80211_TX_QUEUE_DATA2: data queue 2
* @IEEE80211_TX_QUEUE_DATA3: data queue 3
- * @IEEE80211_TX_QUEUE_DATA4: data queue 4
- * @IEEE80211_TX_QUEUE_SVP: ??
- * @NUM_TX_DATA_QUEUES: number of data queues
- * @IEEE80211_TX_QUEUE_AFTER_BEACON: transmit queue for frames to be
- * sent after a beacon
- * @IEEE80211_TX_QUEUE_BEACON: transmit queue for beacon frames
- * @NUM_TX_DATA_QUEUES_AMPDU: adding more queues for A-MPDU
+ * @IEEE80211_NUM_TX_DATA_QUEUES: number of data queues
+ * @IEEE80211_NUM_TX_DATA_QUEUES_AMPDU: adding more queues for A-MPDU
*/
enum ieee80211_tx_queue {
IEEE80211_TX_QUEUE_DATA0,
IEEE80211_TX_QUEUE_DATA1,
IEEE80211_TX_QUEUE_DATA2,
IEEE80211_TX_QUEUE_DATA3,
- IEEE80211_TX_QUEUE_DATA4,
- IEEE80211_TX_QUEUE_SVP,
-
- NUM_TX_DATA_QUEUES,
-
-/* due to stupidity in the sub-ioctl userspace interface, the items in
- * this struct need to have fixed values. As soon as it is removed, we can
- * fix these entries. */
- IEEE80211_TX_QUEUE_AFTER_BEACON = 6,
- IEEE80211_TX_QUEUE_BEACON = 7,
- NUM_TX_DATA_QUEUES_AMPDU = 16
+ IEEE80211_NUM_TX_DATA_QUEUES,
+ IEEE80211_NUM_TX_DATA_QUEUES_AMPDU = 16,
};

struct ieee80211_tx_queue_stats {
- struct ieee80211_tx_queue_stats_data data[NUM_TX_DATA_QUEUES_AMPDU];
+ struct ieee80211_tx_queue_stats_data data[IEEE80211_NUM_TX_DATA_QUEUES_AMPDU];
};

struct ieee80211_low_level_stats {
--- everything.orig/net/mac80211/ieee80211_i.h 2008-02-01 21:56:41.678182617 +0100
+++ everything/net/mac80211/ieee80211_i.h 2008-02-01 21:57:13.008180501 +0100
@@ -450,8 +450,8 @@ struct ieee80211_local {
struct sta_info *sta_hash[STA_HASH_SIZE];
struct timer_list sta_cleanup;

- unsigned long state[NUM_TX_DATA_QUEUES_AMPDU];
- struct ieee80211_tx_stored_packet pending_packet[NUM_TX_DATA_QUEUES_AMPDU];
+ unsigned long state[IEEE80211_NUM_TX_DATA_QUEUES_AMPDU];
+ struct ieee80211_tx_stored_packet pending_packet[IEEE80211_NUM_TX_DATA_QUEUES_AMPDU];
struct tasklet_struct tx_pending_tasklet;

/* number of interfaces with corresponding IFF_ flags */
--- everything.orig/net/mac80211/ieee80211_sta.c 2008-02-01 21:52:05.988181315 +0100
+++ everything/net/mac80211/ieee80211_sta.c 2008-02-01 21:58:02.408180935 +0100
@@ -3231,17 +3231,11 @@ int ieee80211_sta_set_ssid(struct net_de
qparam.cw_max = 1023;
qparam.burst_time = 0;

- for (i = IEEE80211_TX_QUEUE_DATA0; i < NUM_TX_DATA_QUEUES; i++)
+ for (i = IEEE80211_TX_QUEUE_DATA0;
+ i < IEEE80211_NUM_TX_DATA_QUEUES; i++)
local->ops->conf_tx(local_to_hw(local),
i + IEEE80211_TX_QUEUE_DATA0,
&qparam);
-
- /* IBSS uses different parameters for Beacon sending */
- qparam.cw_min++;
- qparam.cw_min *= 2;
- qparam.cw_min--;
- local->ops->conf_tx(local_to_hw(local),
- IEEE80211_TX_QUEUE_BEACON, &qparam);
}

ifsta = &sdata->u.sta;
--- everything.orig/net/mac80211/wme.c 2008-02-01 21:54:35.928182237 +0100
+++ everything/net/mac80211/wme.c 2008-02-01 21:57:42.178178982 +0100
@@ -406,7 +406,7 @@ static int wme_qdiscop_init(struct Qdisc
}

/* reserve all legacy QoS queues */
- for (i = 0; i < min(IEEE80211_TX_QUEUE_DATA4, queues); i++)
+ for (i = 0; i < min(IEEE80211_NUM_TX_DATA_QUEUES, queues); i++)
set_bit(i, &q->qdisc_pool);

return err;
@@ -667,7 +667,7 @@ int ieee80211_ht_agg_queue_add(struct ie
* matching the recieved HW queue */

/* try to get a Qdisc from the pool */
- for (i = IEEE80211_TX_QUEUE_BEACON; i < local->hw.queues; i++)
+ for (i = IEEE80211_NUM_TX_DATA_QUEUES; i < local->hw.queues; i++)
if (!test_and_set_bit(i, &q->qdisc_pool)) {
ieee80211_stop_queue(local_to_hw(local), i);
sta->tid_to_tx_q[tid] = i;
--- everything.orig/net/mac80211/debugfs.c 2008-02-01 21:58:36.578182400 +0100
+++ everything/net/mac80211/debugfs.c 2008-02-01 22:00:02.748177137 +0100
@@ -222,10 +222,10 @@ static ssize_t stats_wme_tx_queue_read(s
size_t count, loff_t *ppos)
{
struct ieee80211_local *local = file->private_data;
- char buf[NUM_TX_DATA_QUEUES*15], *p = buf;
+ char buf[IEEE80211_NUM_TX_DATA_QUEUES * 15], *p = buf;
int i;

- for (i = 0; i < NUM_TX_DATA_QUEUES; i++)
+ for (i = 0; i < IEEE80211_NUM_TX_DATA_QUEUES; i++)
p += scnprintf(p, sizeof(buf)+buf-p,
"%u\n", local->wme_tx_queue[i]);

--- everything.orig/net/mac80211/debugfs_sta.c 2008-02-01 21:59:01.668182074 +0100
+++ everything/net/mac80211/debugfs_sta.c 2008-02-01 21:59:11.768178819 +0100
@@ -167,10 +167,10 @@ STA_OPS(wme_rx_queue);
static ssize_t sta_wme_tx_queue_read(struct file *file, char __user *userbuf,
size_t count, loff_t *ppos)
{
- char buf[15*NUM_TX_DATA_QUEUES], *p = buf;
+ char buf[15 * IEEE80211_NUM_TX_DATA_QUEUES], *p = buf;
int i;
struct sta_info *sta = file->private_data;
- for (i = 0; i < NUM_TX_DATA_QUEUES; i++)
+ for (i = 0; i < IEEE80211_NUM_TX_DATA_QUEUES; i++)
p += scnprintf(p, sizeof(buf)+buf-p, "%u ",
sta->wme_tx_queue[i]);
p += scnprintf(p, sizeof(buf)+buf-p, "\n");
--- everything.orig/drivers/net/wireless/rt2x00/rt2400pci.c 2008-02-01 22:07:51.948181639 +0100
+++ everything/drivers/net/wireless/rt2x00/rt2400pci.c 2008-02-01 22:08:23.088177517 +0100
@@ -1040,7 +1040,7 @@ static void rt2400pci_kick_tx_queue(stru
{
u32 reg;

- if (queue == IEEE80211_TX_QUEUE_BEACON) {
+ if (queue == RT2X00_QUEUE_BEACON) {
rt2x00pci_register_read(rt2x00dev, CSR14, &reg);
if (!rt2x00_get_field32(reg, CSR14_BEACON_GEN)) {
rt2x00_set_field32(&reg, CSR14_BEACON_GEN, 1);
@@ -1055,7 +1055,7 @@ static void rt2400pci_kick_tx_queue(stru
rt2x00_set_field32(&reg, TXCSR0_KICK_TX,
(queue == IEEE80211_TX_QUEUE_DATA1));
rt2x00_set_field32(&reg, TXCSR0_KICK_ATIM,
- (queue == IEEE80211_TX_QUEUE_AFTER_BEACON));
+ (queue == RT2X00_QUEUE_BCMC));
rt2x00pci_register_write(rt2x00dev, TXCSR0, reg);
}

@@ -1160,7 +1160,7 @@ static irqreturn_t rt2400pci_interrupt(i
* 3 - Atim ring transmit done interrupt.
*/
if (rt2x00_get_field32(reg, CSR7_TXDONE_ATIMRING))
- rt2400pci_txdone(rt2x00dev, IEEE80211_TX_QUEUE_AFTER_BEACON);
+ rt2400pci_txdone(rt2x00dev, RT2X00_QUEUE_BCMC);

/*
* 4 - Priority ring transmit done interrupt.
--- everything.orig/drivers/net/wireless/rt2x00/rt2500pci.c 2008-02-01 22:08:36.438230143 +0100
+++ everything/drivers/net/wireless/rt2x00/rt2500pci.c 2008-02-01 22:09:05.988178765 +0100
@@ -275,7 +275,7 @@ static void rt2500pci_config_type(struct
PREAMBLE + get_duration(IEEE80211_HEADER, 20));
rt2x00_set_field32(&reg, BCNCSR1_BEACON_CWMIN,
rt2x00lib_get_ring(rt2x00dev,
- IEEE80211_TX_QUEUE_BEACON)
+ RT2X00_QUEUE_BEACON)
->tx_params.cw_min);
rt2x00pci_register_write(rt2x00dev, BCNCSR1, reg);

@@ -1192,7 +1192,7 @@ static void rt2500pci_kick_tx_queue(stru
{
u32 reg;

- if (queue == IEEE80211_TX_QUEUE_BEACON) {
+ if (queue == RT2X00_QUEUE_BEACON) {
rt2x00pci_register_read(rt2x00dev, CSR14, &reg);
if (!rt2x00_get_field32(reg, CSR14_BEACON_GEN)) {
rt2x00_set_field32(&reg, CSR14_BEACON_GEN, 1);
@@ -1207,7 +1207,7 @@ static void rt2500pci_kick_tx_queue(stru
rt2x00_set_field32(&reg, TXCSR0_KICK_TX,
(queue == IEEE80211_TX_QUEUE_DATA1));
rt2x00_set_field32(&reg, TXCSR0_KICK_ATIM,
- (queue == IEEE80211_TX_QUEUE_AFTER_BEACON));
+ (queue == RT2X00_QUEUE_BCMC));
rt2x00pci_register_write(rt2x00dev, TXCSR0, reg);
}

@@ -1309,7 +1309,7 @@ static irqreturn_t rt2500pci_interrupt(i
* 3 - Atim ring transmit done interrupt.
*/
if (rt2x00_get_field32(reg, CSR7_TXDONE_ATIMRING))
- rt2500pci_txdone(rt2x00dev, IEEE80211_TX_QUEUE_AFTER_BEACON);
+ rt2500pci_txdone(rt2x00dev, RT2X00_QUEUE_BCMC);

/*
* 4 - Priority ring transmit done interrupt.
--- everything.orig/drivers/net/wireless/rt2x00/rt2500usb.c 2008-02-01 22:12:41.858182942 +0100
+++ everything/drivers/net/wireless/rt2x00/rt2500usb.c 2008-02-01 22:13:04.698180447 +0100
@@ -1088,7 +1088,7 @@ static void rt2500usb_kick_tx_queue(stru
{
u16 reg;

- if (queue != IEEE80211_TX_QUEUE_BEACON)
+ if (queue != RT2X00_QUEUE_BEACON)
return;

rt2500usb_register_read(rt2x00dev, TXRX_CSR19, &reg);
@@ -1697,11 +1697,9 @@ static int rt2500usb_beacon_update(struc
int length;

/*
- * Just in case the ieee80211 doesn't set this,
- * but we need this queue set for the descriptor
- * initialization.
+ * mac80211 doesn't set this but we use a queue for beacons
*/
- control->queue = IEEE80211_TX_QUEUE_BEACON;
+ control->queue = RT2X00_QUEUE_BEACON;
ring = rt2x00lib_get_ring(rt2x00dev, control->queue);

/*
@@ -1758,7 +1756,7 @@ static int rt2500usb_beacon_update(struc
/*
* Enable beacon generation.
*/
- rt2500usb_kick_tx_queue(rt2x00dev, IEEE80211_TX_QUEUE_BEACON);
+ rt2500usb_kick_tx_queue(rt2x00dev, RT2X00_QUEUE_BEACON);

return 0;
}
--- everything.orig/drivers/net/wireless/rt2x00/rt2x00.h 2008-02-01 22:05:11.038210937 +0100
+++ everything/drivers/net/wireless/rt2x00/rt2x00.h 2008-02-01 22:09:22.358177843 +0100
@@ -100,6 +100,12 @@
#define MGMT_FRAME_SIZE 256

/*
+ * Internal queue numbers for beacon and buffered broadcast/multicast traffic.
+ */
+#define RT2X00_QUEUE_BEACON 100
+#define RT2X00_QUEUE_BCMC 101
+
+/*
* Number of entries in a packet ring.
* PCI devices only need 1 Beacon entry,
* but USB devices require a second because they
--- everything.orig/drivers/net/wireless/rt2x00/rt2x00dev.c 2008-02-01 22:02:56.938182780 +0100
+++ everything/drivers/net/wireless/rt2x00/rt2x00dev.c 2008-02-01 22:06:28.338180393 +0100
@@ -49,9 +49,9 @@ struct data_ring *rt2x00lib_get_ring(str
if (!rt2x00dev->bcn || !beacon)
return NULL;

- if (queue == IEEE80211_TX_QUEUE_BEACON)
+ if (queue == RT2X00_QUEUE_BEACON)
return &rt2x00dev->bcn[0];
- else if (queue == IEEE80211_TX_QUEUE_AFTER_BEACON)
+ else if (queue == RT2X00_QUEUE_BCMC)
return &rt2x00dev->bcn[1];

return NULL;
@@ -502,7 +502,7 @@ static void rt2x00lib_beacondone_schedul
struct rt2x00_dev *rt2x00dev =
container_of(work, struct rt2x00_dev, beacon_work);
struct data_ring *ring =
- rt2x00lib_get_ring(rt2x00dev, IEEE80211_TX_QUEUE_BEACON);
+ rt2x00lib_get_ring(rt2x00dev, RT2X00_QUEUE_BEACON);
struct data_entry *entry = rt2x00_get_data_entry(ring);
struct sk_buff *skb;

@@ -665,8 +665,8 @@ void rt2x00lib_write_tx_desc(struct rt2x
*/
if (control->queue < rt2x00dev->hw->queues)
desc.queue = control->queue;
- else if (control->queue == IEEE80211_TX_QUEUE_BEACON ||
- control->queue == IEEE80211_TX_QUEUE_AFTER_BEACON)
+ else if (control->queue == RT2X00_QUEUE_BEACON ||
+ control->queue == RT2X00_QUEUE_BCMC)
desc.queue = QUEUE_MGMT;
else
desc.queue = QUEUE_OTHER;
@@ -717,7 +717,7 @@ void rt2x00lib_write_tx_desc(struct rt2x
* Beacons and probe responses require the tsf timestamp
* to be inserted into the frame.
*/
- if (control->queue == IEEE80211_TX_QUEUE_BEACON ||
+ if (control->queue == RT2X00_QUEUE_BEACON ||
is_probe_resp(frame_control))
__set_bit(ENTRY_TXD_REQ_TIMESTAMP, &desc.flags);

--- everything.orig/drivers/net/wireless/rt2x00/rt2x00pci.c 2008-02-01 22:07:08.708181857 +0100
+++ everything/drivers/net/wireless/rt2x00/rt2x00pci.c 2008-02-01 22:07:41.538180176 +0100
@@ -43,11 +43,10 @@ int rt2x00pci_beacon_update(struct ieee8
struct data_entry *entry;

/*
- * Just in case mac80211 doesn't set this correctly,
- * but we need this queue set for the descriptor
- * initialization.
+ * mac80211 doesn't set the queue but we use an internal
+ * number here that we later translate to the right thing.
*/
- control->queue = IEEE80211_TX_QUEUE_BEACON;
+ control->queue = RT2X00_QUEUE_BEACON;
ring = rt2x00lib_get_ring(rt2x00dev, control->queue);
entry = rt2x00_get_data_entry(ring);

--- everything.orig/drivers/net/wireless/rt2x00/rt61pci.c 2008-02-01 22:09:43.378180447 +0100
+++ everything/drivers/net/wireless/rt2x00/rt61pci.c 2008-02-01 22:12:26.208178873 +0100
@@ -1049,8 +1049,9 @@ static int rt61pci_init_rings(struct rt2
rt2x00pci_register_write(rt2x00dev, TX_RING_CSR0, reg);

rt2x00pci_register_read(rt2x00dev, TX_RING_CSR1, &reg);
+ /* XXX: what is this fifth queue? */
rt2x00_set_field32(&reg, TX_RING_CSR1_MGMT_RING_SIZE,
- rt2x00dev->tx[IEEE80211_TX_QUEUE_DATA4].stats.limit);
+ rt2x00dev->tx[4].stats.limit);
rt2x00_set_field32(&reg, TX_RING_CSR1_TXD_SIZE,
rt2x00dev->tx[IEEE80211_TX_QUEUE_DATA0].desc_size /
4);
@@ -1077,8 +1078,9 @@ static int rt61pci_init_rings(struct rt2
rt2x00pci_register_write(rt2x00dev, AC3_BASE_CSR, reg);

rt2x00pci_register_read(rt2x00dev, MGMT_BASE_CSR, &reg);
+ /* fifth queue again? */
rt2x00_set_field32(&reg, MGMT_BASE_CSR_RING_REGISTER,
- rt2x00dev->tx[IEEE80211_TX_QUEUE_DATA4].data_dma);
+ rt2x00dev->tx[4].data_dma);
rt2x00pci_register_write(rt2x00dev, MGMT_BASE_CSR, reg);

rt2x00pci_register_read(rt2x00dev, RX_RING_CSR, &reg);
@@ -1572,7 +1574,7 @@ static void rt61pci_kick_tx_queue(struct
{
u32 reg;

- if (queue == IEEE80211_TX_QUEUE_BEACON) {
+ if (queue == RT2X00_QUEUE_BEACON) {
/*
* For Wi-Fi faily generated beacons between participating
* stations. Set TBTT phase adaptive adjustment step to 8us.
@@ -1596,8 +1598,9 @@ static void rt61pci_kick_tx_queue(struct
(queue == IEEE80211_TX_QUEUE_DATA2));
rt2x00_set_field32(&reg, TX_CNTL_CSR_KICK_TX_AC3,
(queue == IEEE80211_TX_QUEUE_DATA3));
+ /* fifth queue? never used in mac80211 right now... */
rt2x00_set_field32(&reg, TX_CNTL_CSR_KICK_TX_MGMT,
- (queue == IEEE80211_TX_QUEUE_DATA4));
+ (queue == 4));
rt2x00pci_register_write(rt2x00dev, TX_CNTL_CSR, reg);
}

@@ -2383,11 +2386,9 @@ static int rt61pci_beacon_update(struct
struct data_entry *entry;

/*
- * Just in case the ieee80211 doesn't set this,
- * but we need this queue set for the descriptor
- * initialization.
+ * mac80211 doesn't set this but we need it
*/
- control->queue = IEEE80211_TX_QUEUE_BEACON;
+ control->queue = RT2X00_QUEUE_BEACON;
ring = rt2x00lib_get_ring(rt2x00dev, control->queue);
entry = rt2x00_get_data_entry(ring);

@@ -2427,7 +2428,7 @@ static int rt61pci_beacon_update(struct
*/
rt2x00pci_register_multiwrite(rt2x00dev, HW_BEACON_BASE0,
skb->data, skb->len);
- rt61pci_kick_tx_queue(rt2x00dev, IEEE80211_TX_QUEUE_BEACON);
+ rt61pci_kick_tx_queue(rt2x00dev, RT2X00_QUEUE_BEACON);

return 0;
}
--- everything.orig/drivers/net/wireless/rt2x00/rt73usb.c 2008-02-01 22:13:24.188182129 +0100
+++ everything/drivers/net/wireless/rt2x00/rt73usb.c 2008-02-01 22:13:55.278180827 +0100
@@ -1310,7 +1310,7 @@ static void rt73usb_kick_tx_queue(struct
{
u32 reg;

- if (queue != IEEE80211_TX_QUEUE_BEACON)
+ if (queue != RT2X00_QUEUE_BEACON)
return;

/*
@@ -1970,11 +1970,9 @@ static int rt73usb_beacon_update(struct
int timeout;

/*
- * Just in case the ieee80211 doesn't set this,
- * but we need this queue set for the descriptor
- * initialization.
+ * mac80211 doesn't set this but we use a queue for beacons
*/
- control->queue = IEEE80211_TX_QUEUE_BEACON;
+ control->queue = RT2X00_QUEUE_BEACON;
ring = rt2x00lib_get_ring(rt2x00dev, control->queue);
entry = rt2x00_get_data_entry(ring);

@@ -2006,7 +2004,7 @@ static int rt73usb_beacon_update(struct
USB_VENDOR_REQUEST_OUT,
HW_BEACON_BASE0, 0x0000,
skb->data, skb->len, timeout);
- rt73usb_kick_tx_queue(rt2x00dev, IEEE80211_TX_QUEUE_BEACON);
+ rt73usb_kick_tx_queue(rt2x00dev, RT2X00_QUEUE_BEACON);

return 0;
}


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-01 22:29:43

by Johannes Berg

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

Hi Ivo,

> All patches are currently in testing, and I hope to send them to wireless-2.6
> next weekend.

Ah cool, I'll hold off on doing anything then until then.

> rt2400pci, rt2500pci and rt2500usb have a special dedicated queue which is called
> the ATIM queue. The implementation is a guess since it was never used in the
> legacy drivers. But it is clear it contains frames which are send directly after the beacon.

Well I'd guess this is for the ATIM window. Broadcom firmware actually
has an ATIM queue *and* a broadcast/multicast queue, I would expect that
you should actually put broadcast/multicast traffic onto the beacon
queue (unless the beacon queue only sends one packet per beacon
interval)

ATIM is actually one of the things I haven't fully understood yet, see
IEEE 802.11 11.2.2.

> > 4. remove IEEE80211_TX_QUEUE_SVP, it's something strange and
> > atheros specific
>
> I always wondered what the queue was. ;)

spectralink voice protocol. I have not even the slightest clue.

> HCCA queue is a big mysterie, there are registers to initialize it inside the device,
> but in implementation of the legacy driver the queue was unused, I haven't found a
> correct entry point yet to send frames over it, since that interface seems to be missing
> in the device... So either I am missing something or the queue was half-implemented
> in the device itself.. :S

No idea, that's HCF (802.11e) stuff. Maybe they just never got around to
finishing that implementation.

johannes


Attachments:
signature.asc (828.00 B)
This is a digitally signed message part

2008-02-01 22:54:25

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

Hi,

> > All patches are currently in testing, and I hope to send them to wireless-2.6
> > next weekend.
>
> Ah cool, I'll hold off on doing anything then until then.
>
> > rt2400pci, rt2500pci and rt2500usb have a special dedicated queue which is called
> > the ATIM queue. The implementation is a guess since it was never used in the
> > legacy drivers. But it is clear it contains frames which are send directly after the beacon.
>
> Well I'd guess this is for the ATIM window. Broadcom firmware actually
> has an ATIM queue *and* a broadcast/multicast queue, I would expect that
> you should actually put broadcast/multicast traffic onto the beacon
> queue (unless the beacon queue only sends one packet per beacon
> interval)

In all Ralink drivers the beacon queue will only send out the frame each interval,
where rt2400pci and rt2500pci a interrupt will be triggered to request a new
beacon frame.

> ATIM is actually one of the things I haven't fully understood yet, see
> IEEE 802.11 11.2.2.
>
> > > 4. remove IEEE80211_TX_QUEUE_SVP, it's something strange and
> > > atheros specific
> >
> > I always wondered what the queue was. ;)
>
> spectralink voice protocol. I have not even the slightest clue.
>
> > HCCA queue is a big mysterie, there are registers to initialize it inside the device,
> > but in implementation of the legacy driver the queue was unused, I haven't found a
> > correct entry point yet to send frames over it, since that interface seems to be missing
> > in the device... So either I am missing something or the queue was half-implemented
> > in the device itself.. :S
>
> No idea, that's HCF (802.11e) stuff. Maybe they just never got around to
> finishing that implementation.

Or the documenation is just missing. ;)
In any case it will remain unused for rt61pci and rt73usb. I have seen some other
queue handling in the legacy rt2800 drivers but that driver needs more investigation
before I can say anything usefull about it.

Ivo

2008-02-03 15:36:12

by Guy Cohen

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

On 2/2/08, Ivo van Doorn <[email protected]> wrote:
> On Saturday 02 February 2008, Tomas Winkler wrote:
> > 4965 has 4 queues which are used for regular WMM. Only 11 queues
> > might be used for aggregation, but in AP mode we might map one of the
> > queues to after beacon multicast traffic.
>
> Just out of interest, and I don't know about the iwl4965 driver specifics,
> but how will you make sure those frames are send after the beacon?
> Does the beacon trigger an interrupt after each interval which allows you to queue
> the frames or is some other approach used?

For iwl4965, mcast frames are queued into a separate HW TX FIFO (no
need to buffer at the host). Beacon periodic transmission and
controling the TX of mcast frames from the mcast FIFO are all timely
done in real-time by the device.

Guy.

2008-02-01 23:31:47

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

On Saturday 02 February 2008, Tomas Winkler wrote:
> On Feb 1, 2008 11:43 PM, Johannes Berg <[email protected]> wrote:
> > Any thoughts on this patch? The only thing it should really actually
> > change is removing the IBSS beacon queue configuration, but as I've
> > explained previously that really has to be done by the driver because we
> > don't know whether the driver even uses queues for beaconing... Also we
> > never reset that info.
> >
> > Also, I'm wondering, iwl4965 has 16 queues, can it use 4 for regular WMM
> > and the other 12 for aggregation? And how does it configure the access
> > parameters for aggregation? I don't see conf_tx() calls anywhere for
> > that.
> >
>
> 4965 has 4 queues which are used for regular WMM. Only 11 queues
> might be used for aggregation, but in AP mode we might map one of the
> queues to after beacon multicast traffic.

Just out of interest, and I don't know about the iwl4965 driver specifics,
but how will you make sure those frames are send after the beacon?
Does the beacon trigger an interrupt after each interval which allows you to queue
the frames or is some other approach used?

I am asking because I am not sure how I should implement such a thing for
the rt2x00 drivers that don't have a device ATIM queue (which automatically
sends the frames after the beacon was send).

> One queue is reserved for communicating with FW. The number my differ
> on further NICs.
>
> All the traffic queues 4 WMM + 11 AGG are served according regular WMM
> policy, 4 AC - these are configured by conf_tx. The aggregation
> queues are needed for handling ordering of frames correctly and not
> for on media prioritizing
>
> Sorry but I have to review your patch later
> Thanks
> Tomas


2008-02-03 17:42:54

by Jouni Malinen

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

On Fri, Feb 01, 2008 at 05:15:12PM +0100, Johannes Berg wrote:

> To be honest, I'm totally unclear on how all this queue stuff is
> supposed to work. Ivo pointed me to IEEE80211_TX_QUEUE_AFTER_BEACON
> which is unused and duplicated with IEEE80211_TXCTL_SEND_AFTER_DTIM.

I don't think IEEE80211_TX_QUEUE_AFTER_BEACON was originally used for
anything inside the d80211 code so it was just a queue identifier that
was shared between the hardware driver and more generic 802.11 code. As
such, it should be fine to do this inside the hardware drivers and
remove this from mac80211.

> 2. get rid of IEEE80211_TX_QUEUE_BEACON, it isn't used in mac80211
> and it's not hardware-independent, not all hardware actually
> uses a queue for beacons

This was originally used only for setting up queue parameters for Beacon
frames in IBSS and that can be done differently.

> 4. remove IEEE80211_TX_QUEUE_SVP, it's something strange and
> atheros specific

Sure, that should be removed.

--
Jouni Malinen PGP id EFC895FA

2008-02-01 22:17:09

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: mac80211 QoS/aggregation questions, thoughts

Hi,

Below are some comments regarding the rt2x00 implementations of
rings/queues. Note that all comments are for the actual hardware or
the current implementation inside rt2x00.git.
Queue handling has received a drastic overhaul in rt2x00, so the
implementation, especially for the beacon, is different then currently
in wireless-2.6. For the beacons I also have to add that rt2x00.git now
supports multiple virtual interfaces in master mode, which were made
possible due to the changes in the queue handling.
All patches are currently in testing, and I hope to send them to wireless-2.6
next weekend.

> 1. move queue configuration for data queues to bss-config
> structure, it really is part of the bss configuration since it
> is mandated by the AP
> 2. get rid of IEEE80211_TX_QUEUE_BEACON, it isn't used in mac80211
> and it's not hardware-independent, not all hardware actually
> uses a queue for beacons (b43 for example does beaconing
> differently), those drivers that do use a queue will have to do
> that internally. Also, the one place where it is used is the
> queue configuration for IBSS beacons, but that somewhat bogus
> anyway since we never reset that should we go back into AP mode
> after being in IBSS! Hence, I think the driver should handle
> that when an interface is brought up. Ivo, any comments?

rt2x00 uses the IEEE80211_TX_QUEUE_BEACON, but mostly for the internal
usage you described. Although only rt2400pci and rt2500pci have the actual
"ring" in the register, all other drivers use it as reference to

Per interface a queue entry is assigned in the ieee80211_vif private data,
this means that the queue structure itself is only used to find a free entry
for the interface.

> 3. get rid of IEEE80211_TX_QUEUE_AFTER_BEACON, we have
> IEEE80211_TXCTL_SEND_AFTER_DTIM now and that is
> hardware-independent

rt2400pci, rt2500pci and rt2500usb have a special dedicated queue which is called
the ATIM queue. The implementation is a guess since it was never used in the
legacy drivers. But it is clear it contains frames which are send directly after the beacon.
rt61pci and rt73usb don't contain the ATIM ring, and there doesn't seem to be a valid
replacement. Since Ralink never released the AP version of there driver under the GPL,
getting the driver to work in AP mode is mosting guessing.

> 4. remove IEEE80211_TX_QUEUE_SVP, it's something strange and
> atheros specific

I always wondered what the queue was. ;)

> 5. remove IEEE80211_TX_QUEUE_DATA4, only rt61pci uses that and that
> use is strange, Ivo? We never submit frames to that queue...

In rt2x00.git that queue has been removed.
A short overview of rings in rt2x00:
rt2400pci: PRIO, TX, ATIM, BEACON
rt2500pci: PRIO, TX, ATIM, BEACON
rt2500usb: PRIO, TX, ATIM, BEACON
rt61pci: AC0, AC1, AC2, AC3, MGMT, HCCA
rt73usb AC0, AC1, AC2, AC3, MGMT, HCCA

The legacy drivers uses the MGMT and PRIO rings to send management frames over.
ATIM and HCCA are unused and the AC* and TX rings are used for frames.

rt2x00 changed this and made the PRIO queue normal TX queue with higher priority
then the TX ring. I am not really sure what to make of the MGMT queue, up untill now
it was unused, but if there is some use for it...
HCCA queue is a big mysterie, there are registers to initialize it inside the device,
but in implementation of the legacy driver the queue was unused, I haven't found a
correct entry point yet to send frames over it, since that interface seems to be missing
in the device... So either I am missing something or the queue was half-implemented
in the device itself.. :S

> This would leave us with the regular WMM four data queues. When hardware
> announces more than four queues we would assume that the remaining
> queues are usable for aggregation so that drivers for hardware that
> needs a beacon queue (for example) would have to subtract that from the
> number of available queues announced to mac80211.

Which already happened I believe..

Ivo