2011-04-19 17:12:34

by Nishant Sarmukadam

[permalink] [raw]
Subject: [PATCH V2 1/4] mwl8k: Do not stop tx queues

From: Pradeep Nemavat <[email protected]>

This is in preparation to support life time expiry of packets in the
hardware to avoid head-of-line blocking where a slow client can
hog a tx queue and affect the traffic to a faster client from the same
queue. Time stamp the packets in driver to allow dropping them in the
hardware if they are queued for more than 500ms.

If queues are stopped, packets will be queued up outside the driver.
Since we will be able to timestamp the packets only after they hit the
driver, the timestamp will be less accurate since we cannot consider
the time the packets spent in queues outside the driver. With this commit,
to achieve accurate timestamping, the tx queues will never be stopped.
Now, we need to be prepared for a situation where packets hit the driver
even after the tx queues are full. Drop all such packets in the driver itself.

Signed-off-by: Pradeep Nemavat <[email protected]>
Signed-off-by: Nishant Sarmukadam <[email protected]>
---
drivers/net/wireless/mwl8k.c | 20 +++++++-------------
1 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index ebaae35..7968301 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -1666,10 +1666,6 @@ mwl8k_txq_reclaim(struct ieee80211_hw *hw, int index, int limit, int force)
processed++;
}

- if (index < MWL8K_TX_WMM_QUEUES && processed && priv->radio_on &&
- !mutex_is_locked(&priv->fw_mutex))
- ieee80211_wake_queue(hw, index);
-
return processed;
}

@@ -1951,13 +1947,14 @@ mwl8k_txq_xmit(struct ieee80211_hw *hw, int index, struct sk_buff *skb)

txq = priv->txq + index;

- if (index >= MWL8K_TX_WMM_QUEUES && txq->len >= MWL8K_TX_DESCS) {
- /* This is the case in which the tx packet is destined for an
- * AMPDU queue and that AMPDU queue is full. Because we don't
- * start and stop the AMPDU queues, we must drop these packets.
- */
- dev_kfree_skb(skb);
+ if (txq->len >= MWL8K_TX_DESCS) {
+ if (start_ba_session) {
+ spin_lock(&priv->stream_lock);
+ mwl8k_remove_stream(hw, stream);
+ spin_unlock(&priv->stream_lock);
+ }
spin_unlock_bh(&priv->tx_lock);
+ dev_kfree_skb(skb);
return;
}

@@ -1985,9 +1982,6 @@ mwl8k_txq_xmit(struct ieee80211_hw *hw, int index, struct sk_buff *skb)
if (txq->tail == MWL8K_TX_DESCS)
txq->tail = 0;

- if (txq->head == txq->tail && index < MWL8K_TX_WMM_QUEUES)
- ieee80211_stop_queue(hw, index);
-
mwl8k_tx_start(priv);

spin_unlock_bh(&priv->tx_lock);
--
1.6.0.3



2011-04-21 07:39:17

by Lennert Buytenhek

[permalink] [raw]
Subject: Re: [PATCH V2 2/4] mwl8k: Add timestamp information for tx packets

On Tue, Apr 19, 2011 at 10:16:20PM +0530, Nishant Sarmukadam wrote:

> From: Pradeep Nemavat <[email protected]>
>
> Timestamp tx packets using a HW micro-second timer.
> This timestamp will be compared to the current timestamp
> in the hardware and if the difference is greater than 500ms,
> the packet will be dropped.
>
> Signed-off-by: Pradeep Nemavat <[email protected]>
> Signed-off-by: Nishant Sarmukadam <[email protected]>
> ---
> drivers/net/wireless/mwl8k.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
> index 7968301..aca0139 100644
> --- a/drivers/net/wireless/mwl8k.c
> +++ b/drivers/net/wireless/mwl8k.c
> @@ -1792,6 +1792,14 @@ static inline void mwl8k_tx_count_packet(struct ieee80211_sta *sta, u8 tid)
> tx_stats->pkts++;
> }
>
> +/* HW micro second timer register
> + * located at offset 0xA600. This
> + * will be used to timestamp tx
> + * packets.
> + */
> +
> +#define MWL8K_HW_TIMER_REGISTER (priv->regs + 0xA600)

1. Ugly implicit 'priv' argument here. :-(

Please just do something like:

#define MWL8K_A2H_INT_RX_READY (1 << 1)
#define MWL8K_A2H_INT_TX_DONE (1 << 0)
+
+/* Misc registers */
+#define MWL8K_HW_TIMER_REGISTER 0x0000a600

#define MWL8K_A2H_EVENTS (MWL8K_A2H_INT_DUMMY | \
MWL8K_A2H_INT_CHNL_SWITCHED | \

and then just use it as ioread32(priv->regs + MWL8K_HW_TIMER_REGISTER).


2. Does this register exist on 8687/superfly3, or is it SJ only?

2011-04-19 17:12:37

by Nishant Sarmukadam

[permalink] [raw]
Subject: [PATCH V2 2/4] mwl8k: Add timestamp information for tx packets

From: Pradeep Nemavat <[email protected]>

Timestamp tx packets using a HW micro-second timer.
This timestamp will be compared to the current timestamp
in the hardware and if the difference is greater than 500ms,
the packet will be dropped.

Signed-off-by: Pradeep Nemavat <[email protected]>
Signed-off-by: Nishant Sarmukadam <[email protected]>
---
drivers/net/wireless/mwl8k.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index 7968301..aca0139 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -1792,6 +1792,14 @@ static inline void mwl8k_tx_count_packet(struct ieee80211_sta *sta, u8 tid)
tx_stats->pkts++;
}

+/* HW micro second timer register
+ * located at offset 0xA600. This
+ * will be used to timestamp tx
+ * packets.
+ */
+
+#define MWL8K_HW_TIMER_REGISTER (priv->regs + 0xA600)
+
static void
mwl8k_txq_xmit(struct ieee80211_hw *hw, int index, struct sk_buff *skb)
{
@@ -1972,6 +1980,10 @@ mwl8k_txq_xmit(struct ieee80211_hw *hw, int index, struct sk_buff *skb)
tx->peer_id = MWL8K_STA(tx_info->control.sta)->peer_id;
else
tx->peer_id = 0;
+
+ if (priv->ap_fw)
+ tx->timestamp = cpu_to_le32(ioread32(MWL8K_HW_TIMER_REGISTER));
+
wmb();
tx->status = cpu_to_le32(MWL8K_TXD_STATUS_FW_OWNED | txstatus);

--
1.6.0.3


2011-04-21 08:24:19

by Nishant Sarmukadam

[permalink] [raw]
Subject: Re: [PATCH V2 3/4] mwl8k: Reserve buffers for tx management frames


On Thu, 2011-04-21 at 00:44 -0700, Lennert Buytenhek wrote:
> On Tue, Apr 19, 2011 at 10:16:21PM +0530, Nishant Sarmukadam wrote:
>
> > - if (txq->len >= MWL8K_TX_DESCS) {
> > - if (start_ba_session) {
> > - spin_lock(&priv->stream_lock);
> > - mwl8k_remove_stream(hw, stream);
> > - spin_unlock(&priv->stream_lock);
> > + /* Mgmt frames that go out frequently are probe responses.
> > + * Other mgmt frames got out relatively infrequently.
> > + * Hence reserve 2 buffers so that other mgmt frames do not get
> > + * dropped due to an already queued probe response in one of
> > + * the reserved buffers.
> > + */
>
> Comment formatting..
>
>
> > + if (txq->len >= (MWL8K_TX_DESCS - 2)) {
> > + if ((mgmtframe == false) ||
> > + (txq->len == MWL8K_TX_DESCS)) {
> > + if (start_ba_session) {
> > + spin_lock(&priv->stream_lock);
> > + mwl8k_remove_stream(hw, stream);
> > + spin_unlock(&priv->stream_lock);
> > + }
>
> ..and lots of superfluous parentheses here:
>
> txq->len >= MWL8K_TX_DESCS - 2
> mgmtframe == false
> txq->len == MWL8K_TX_DESCS

I will address both these comments in patch set V3.



2011-04-19 17:12:41

by Nishant Sarmukadam

[permalink] [raw]
Subject: [PATCH V2 3/4] mwl8k: Reserve buffers for tx management frames

Since queues are not stopped anymore, management frames would be
dropped if the corresponding tx queue is full.
This can cause issues say when we want to setup an ampdu stream and
action frames i.e addba requests keep getting dropped frequently.
Fix this by reserving some buffers to allow management frames to
go through in queue full conditions.

Signed-off-by: Nishant Sarmukadam <[email protected]>
Signed-off-by: Pradeep Nemavat <[email protected]>
---
drivers/net/wireless/mwl8k.c | 30 ++++++++++++++++++++++--------
1 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index aca0139..82cd725 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -1818,6 +1818,7 @@ mwl8k_txq_xmit(struct ieee80211_hw *hw, int index, struct sk_buff *skb)
u8 tid = 0;
struct mwl8k_ampdu_stream *stream = NULL;
bool start_ba_session = false;
+ bool mgmtframe = false;
struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)skb->data;

wh = (struct ieee80211_hdr *)skb->data;
@@ -1826,6 +1827,9 @@ mwl8k_txq_xmit(struct ieee80211_hw *hw, int index, struct sk_buff *skb)
else
qos = 0;

+ if (ieee80211_is_mgmt(wh->frame_control))
+ mgmtframe = true;
+
if (priv->ap_fw)
mwl8k_encapsulate_tx_frame(skb);
else
@@ -1955,15 +1959,25 @@ mwl8k_txq_xmit(struct ieee80211_hw *hw, int index, struct sk_buff *skb)

txq = priv->txq + index;

- if (txq->len >= MWL8K_TX_DESCS) {
- if (start_ba_session) {
- spin_lock(&priv->stream_lock);
- mwl8k_remove_stream(hw, stream);
- spin_unlock(&priv->stream_lock);
+ /* Mgmt frames that go out frequently are probe responses.
+ * Other mgmt frames got out relatively infrequently.
+ * Hence reserve 2 buffers so that other mgmt frames do not get
+ * dropped due to an already queued probe response in one of
+ * the reserved buffers.
+ */
+
+ if (txq->len >= (MWL8K_TX_DESCS - 2)) {
+ if ((mgmtframe == false) ||
+ (txq->len == MWL8K_TX_DESCS)) {
+ if (start_ba_session) {
+ spin_lock(&priv->stream_lock);
+ mwl8k_remove_stream(hw, stream);
+ spin_unlock(&priv->stream_lock);
+ }
+ spin_unlock_bh(&priv->tx_lock);
+ dev_kfree_skb(skb);
+ return;
}
- spin_unlock_bh(&priv->tx_lock);
- dev_kfree_skb(skb);
- return;
}

BUG_ON(txq->skb[txq->tail] != NULL);
--
1.6.0.3


2011-04-19 17:12:44

by Nishant Sarmukadam

[permalink] [raw]
Subject: [PATCH V2 4/4] mwl8k: Enable life time expiry for tx packets in the hardware

Tell the firmware to enable the life time expiry of tx packets
in the hardware. The hardware will now refer to the timestamp
in every tx packet and decide whether the packet needs to be
dropped or transmitted.

Signed-off-by: Nishant Sarmukadam <[email protected]>
Signed-off-by: Pradeep Nemavat <[email protected]>
---
drivers/net/wireless/mwl8k.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index 82cd725..7db4bb9 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -2502,7 +2502,8 @@ static int mwl8k_cmd_set_hw_spec(struct ieee80211_hw *hw)

cmd->flags = cpu_to_le32(MWL8K_SET_HW_SPEC_FLAG_HOST_DECR_MGMT |
MWL8K_SET_HW_SPEC_FLAG_HOSTFORM_PROBERESP |
- MWL8K_SET_HW_SPEC_FLAG_HOSTFORM_BEACON);
+ MWL8K_SET_HW_SPEC_FLAG_HOSTFORM_BEACON |
+ MWL8K_SET_HW_SPEC_FLAG_ENABLE_LIFE_TIME_EXPIRY);
cmd->num_tx_desc_per_queue = cpu_to_le32(MWL8K_TX_DESCS);
cmd->total_rxd = cpu_to_le32(MWL8K_RX_DESCS);

--
1.6.0.3


2011-04-21 08:23:33

by Nishant Sarmukadam

[permalink] [raw]
Subject: Re: [PATCH V2 2/4] mwl8k: Add timestamp information for tx packets


On Thu, 2011-04-21 at 00:42 -0700, Lennert Buytenhek wrote:
> On Tue, Apr 19, 2011 at 10:16:20PM +0530, Nishant Sarmukadam wrote:
>
> > From: Pradeep Nemavat <[email protected]>
> >
> > Timestamp tx packets using a HW micro-second timer.
> > This timestamp will be compared to the current timestamp
> > in the hardware and if the difference is greater than 500ms,
> > the packet will be dropped.
> >
> > Signed-off-by: Pradeep Nemavat <[email protected]>
> > Signed-off-by: Nishant Sarmukadam <[email protected]>
> > ---
> > drivers/net/wireless/mwl8k.c | 12 ++++++++++++
> > 1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
> > index 7968301..aca0139 100644
> > --- a/drivers/net/wireless/mwl8k.c
> > +++ b/drivers/net/wireless/mwl8k.c
> > @@ -1792,6 +1792,14 @@ static inline void mwl8k_tx_count_packet(struct ieee80211_sta *sta, u8 tid)
> > tx_stats->pkts++;
> > }
> >
> > +/* HW micro second timer register
> > + * located at offset 0xA600. This
> > + * will be used to timestamp tx
> > + * packets.
> > + */
> > +
> > +#define MWL8K_HW_TIMER_REGISTER (priv->regs + 0xA600)
>
> 1. Ugly implicit 'priv' argument here. :-(
>
> Please just do something like:
>
> #define MWL8K_A2H_INT_RX_READY (1 << 1)
> #define MWL8K_A2H_INT_TX_DONE (1 << 0)
> +
> +/* Misc registers */
> +#define MWL8K_HW_TIMER_REGISTER 0x0000a600
>
> #define MWL8K_A2H_EVENTS (MWL8K_A2H_INT_DUMMY | \
> MWL8K_A2H_INT_CHNL_SWITCHED | \
>
> and then just use it as ioread32(priv->regs + MWL8K_HW_TIMER_REGISTER).
>
>
Ok, will send out patch set V3 incorporating this change.

> 2. Does this register exist on 8687/superfly3, or is it SJ only?

Yes, this does exist on 8687 and superfly3 as well.



2011-04-21 07:41:43

by Lennert Buytenhek

[permalink] [raw]
Subject: Re: [PATCH V2 3/4] mwl8k: Reserve buffers for tx management frames

On Tue, Apr 19, 2011 at 10:16:21PM +0530, Nishant Sarmukadam wrote:

> - if (txq->len >= MWL8K_TX_DESCS) {
> - if (start_ba_session) {
> - spin_lock(&priv->stream_lock);
> - mwl8k_remove_stream(hw, stream);
> - spin_unlock(&priv->stream_lock);
> + /* Mgmt frames that go out frequently are probe responses.
> + * Other mgmt frames got out relatively infrequently.
> + * Hence reserve 2 buffers so that other mgmt frames do not get
> + * dropped due to an already queued probe response in one of
> + * the reserved buffers.
> + */

Comment formatting..


> + if (txq->len >= (MWL8K_TX_DESCS - 2)) {
> + if ((mgmtframe == false) ||
> + (txq->len == MWL8K_TX_DESCS)) {
> + if (start_ba_session) {
> + spin_lock(&priv->stream_lock);
> + mwl8k_remove_stream(hw, stream);
> + spin_unlock(&priv->stream_lock);
> + }

..and lots of superfluous parentheses here:

txq->len >= MWL8K_TX_DESCS - 2
mgmtframe == false
txq->len == MWL8K_TX_DESCS

2011-04-21 07:36:06

by Lennert Buytenhek

[permalink] [raw]
Subject: Re: [PATCH V2 1/4] mwl8k: Do not stop tx queues

On Tue, Apr 19, 2011 at 10:16:19PM +0530, Nishant Sarmukadam wrote:

> From: Pradeep Nemavat <[email protected]>
>
> This is in preparation to support life time expiry of packets in the
> hardware to avoid head-of-line blocking where a slow client can
> hog a tx queue and affect the traffic to a faster client from the same
> queue. Time stamp the packets in driver to allow dropping them in the
> hardware if they are queued for more than 500ms.
>
> If queues are stopped, packets will be queued up outside the driver.
> Since we will be able to timestamp the packets only after they hit the
> driver, the timestamp will be less accurate since we cannot consider
> the time the packets spent in queues outside the driver. With this commit,
> to achieve accurate timestamping, the tx queues will never be stopped.
> Now, we need to be prepared for a situation where packets hit the driver
> even after the tx queues are full. Drop all such packets in the driver itself.

It might be worth adding that after this patch, the mac80211 queues are
only stopped when firmware commands are executing (and when the interface
is down'ed).

Also, it's somewhat doubtful that this should really be called HOL
blocking.

Apart from those, OK with the explanation.