[PATCH 1/4] mwl8k: Do not stop tx queues

[PATCH 1/4] mwl8k: Do not stop tx queues

[Nishant Sarmukadam]

From: Pradeep Nemavat <pnemavat@marvell.com>

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.

Since queues are not being stopped now, we need to be prepared for
a situation where packets hit the driver after the queues are full.
Drop all such packets in the driver itself.

Signed-off-by: Pradeep Nemavat <pnemavat@marvell.com>
Signed-off-by: Nishant Sarmukadam <nishants@marvell.com>
---
 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

[PATCH 2/4] mwl8k: Add timestamp information for tx packets

[Nishant Sarmukadam]

From: Pradeep Nemavat <pnemavat@marvell.com>

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 <pnemavat@marvell.com>
Signed-off-by: Nishant Sarmukadam <nishants@marvell.com>
---
 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

Re: [PATCH 2/4] mwl8k: Add timestamp information for tx packets

[Lennert Buytenhek]

On Tue, Apr 19, 2011 at 11:12:50AM +0530, Nishant Sarmukadam wrote:

> From: Pradeep Nemavat <pnemavat@marvell.com>
> 
> 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.

Why don't you do this entirely in the firmware?

When a packet is put into the TX ring by the host, give it a timestamp
of zero.

When the firmware gets a TX interrupt, have it walk the TX ring from
the last TX entry where a timestamp was written to (i.e. keep a separate
index for this), and timestamp all TX ring entries that are marked as
being FW OWNED but don't have a timestamp yet (and update your index).

Then when the time comes to process the TX packet (i.e. it has reached
the head of the TX queue), compare the timestamp with the current time.

As an added bonus, this'll also work over USB/SPI/whatever other bus
type where you cannot read some H/W microsecond timestamp register
directly like you can on PCI(e) -- which sticks out like a sore thumb
because it goes around the clean command / ring based host<->firmware
interface.

Re: [PATCH 2/4] mwl8k: Add timestamp information for tx packets

[Nishant Sarmukadam]

On Tue, 2011-04-19 at 00:54 -0700, Lennert Buytenhek wrote:
> On Tue, Apr 19, 2011 at 11:12:50AM +0530, Nishant Sarmukadam wrote:
> 
> > From: Pradeep Nemavat <pnemavat@marvell.com>
> > 
> > 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.
> 
> Why don't you do this entirely in the firmware?
> 
> When a packet is put into the TX ring by the host, give it a timestamp
> of zero.
> 
> When the firmware gets a TX interrupt, have it walk the TX ring from
> the last TX entry where a timestamp was written to (i.e. keep a separate
> index for this), and timestamp all TX ring entries that are marked as
> being FW OWNED but don't have a timestamp yet (and update your index).
> 
> Then when the time comes to process the TX packet (i.e. it has reached
> the head of the TX queue), compare the timestamp with the current time.

Since firmware cannot access host memory directly, it needs additional
dma operations to read all descriptors, then check the ownership bit and
update the timestamp for the descriptors. It can easily take about 1us
per descriptor for this and it can hit the peak throughput by about
2.5%. Also, when the firmware is processing the packets from the host tx
queues, it keeps the host interrupt for tx packets disabled to avoid
unnecessary interruptions. Hence, the timestamp in the firmware can
deviate from when the packet was queued in the host queue. 


> 
> As an added bonus, this'll also work over USB/SPI/whatever other bus
> type where you cannot read some H/W microsecond timestamp register
> directly like you can on PCI(e) -- which sticks out like a sore thumb
> because it goes around the clean command / ring based host<->firmware
> interface.

We agree that this approach will not work without additional changes for
non-PCI devices. However, we do not foresee such devices being supported
by mwl8k in the near future.  

Re: [PATCH 2/4] mwl8k: Add timestamp information for tx packets

[Lennert Buytenhek]

On Tue, Apr 19, 2011 at 03:38:20PM +0530, Nishant Sarmukadam wrote:

> > > 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.
> > 
> > Why don't you do this entirely in the firmware?
> > 
> > When a packet is put into the TX ring by the host, give it a timestamp
> > of zero.
> > 
> > When the firmware gets a TX interrupt, have it walk the TX ring from
> > the last TX entry where a timestamp was written to (i.e. keep a separate
> > index for this), and timestamp all TX ring entries that are marked as
> > being FW OWNED but don't have a timestamp yet (and update your index).
> > 
> > Then when the time comes to process the TX packet (i.e. it has reached
> > the head of the TX queue), compare the timestamp with the current time.
> 
> Since firmware cannot access host memory directly, it needs additional
> dma operations to read all descriptors, then check the ownership bit and
> update the timestamp for the descriptors. It can easily take about 1us
> per descriptor for this and it can hit the peak throughput by about
> 2.5%.

That DMA needs to happen anyway, whether you have the host timestamp
the packets or not, so this doesn't seem to be an applicable argument.


> Also, when the firmware is processing the packets from the host tx
> queues, it keeps the host interrupt for tx packets disabled to avoid
> unnecessary interruptions. Hence, the timestamp in the firmware can
> deviate from when the packet was queued in the host queue. 

Right, but that is an (easily fixable?) implementation detail, and not
a reason for why this fundamentally cannot work.


> > As an added bonus, this'll also work over USB/SPI/whatever other bus
> > type where you cannot read some H/W microsecond timestamp register
> > directly like you can on PCI(e) -- which sticks out like a sore thumb
> > because it goes around the clean command / ring based host<->firmware
> > interface.
> 
> We agree that this approach will not work without additional changes
> for non-PCI devices. However, we do not foresee such devices being
> supported by mwl8k in the near future.  

It's still an ugly layering violation, IMHO.  But if MRVL is OK with it...

Re: [PATCH 2/4] mwl8k: Add timestamp information for tx packets

[Nishant Sarmukadam]

On Tue, 2011-04-19 at 03:31 -0700, Lennert Buytenhek wrote:
> On Tue, Apr 19, 2011 at 03:38:20PM +0530, Nishant Sarmukadam wrote:
> 
> > > > 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.
> > > 
> > > Why don't you do this entirely in the firmware?
> > > 
> > > When a packet is put into the TX ring by the host, give it a timestamp
> > > of zero.
> > > 
> > > When the firmware gets a TX interrupt, have it walk the TX ring from
> > > the last TX entry where a timestamp was written to (i.e. keep a separate
> > > index for this), and timestamp all TX ring entries that are marked as
> > > being FW OWNED but don't have a timestamp yet (and update your index).
> > > 
> > > Then when the time comes to process the TX packet (i.e. it has reached
> > > the head of the TX queue), compare the timestamp with the current time.
> > 
> > Since firmware cannot access host memory directly, it needs additional
> > dma operations to read all descriptors, then check the ownership bit and
> > update the timestamp for the descriptors. It can easily take about 1us
> > per descriptor for this and it can hit the peak throughput by about
> > 2.5%.
> 
> That DMA needs to happen anyway, whether you have the host timestamp
> the packets or not, so this doesn't seem to be an applicable argument.
> 

Currently, firmware downloads a small fixed number of descriptors at a
given time from any tx queue as it does not have memory to download all
the descriptors from a host queue.

Assume that the total tx buffers available in firmware is X and host tx
queue size is 2X and that the host queue is completely backlogged. In
this case, the firmware needs to download the descriptors from the head
of the host queue to transmit corresponding packets and it needs to
download the descriptors at the tail of the host queue to timestamp
them. But, firmware cannot transmit the packets associated with
descriptors from the tail since there are packets in host queue which
were queued earlier to these packets. This effectively results in each
descriptor being downloaded twice - once for timestamp and once for
transmission. This will significantly affect the peak throughput
performance.


> 
> > Also, when the firmware is processing the packets from the host tx
> > queues, it keeps the host interrupt for tx packets disabled to avoid
> > unnecessary interruptions. Hence, the timestamp in the firmware can
> > deviate from when the packet was queued in the host queue. 
> 
> Right, but that is an (easily fixable?) implementation detail, and not
> a reason for why this fundamentally cannot work.

Disabling the interrupts is a MIPS optimization. If we enable the
interrupts, it would result in lot of interrupts and context switches
which will add to processing overhead and reduce peak throughput. 

> 
> 
> > > As an added bonus, this'll also work over USB/SPI/whatever other bus
> > > type where you cannot read some H/W microsecond timestamp register
> > > directly like you can on PCI(e) -- which sticks out like a sore thumb
> > > because it goes around the clean command / ring based host<->firmware
> > > interface.
> > 
> > We agree that this approach will not work without additional changes
> > for non-PCI devices. However, we do not foresee such devices being
> > supported by mwl8k in the near future.  
> 
> It's still an ugly layering violation, IMHO.  But if MRVL is OK with it...

Re: [PATCH 2/4] mwl8k: Add timestamp information for tx packets

[Lennert Buytenhek]

On Tue, Apr 19, 2011 at 06:36:41PM +0530, Nishant Sarmukadam wrote:

> > > > > 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.
> > > > 
> > > > Why don't you do this entirely in the firmware?
> > > > 
> > > > When a packet is put into the TX ring by the host, give it a timestamp
> > > > of zero.
> > > > 
> > > > When the firmware gets a TX interrupt, have it walk the TX ring from
> > > > the last TX entry where a timestamp was written to (i.e. keep a separate
> > > > index for this), and timestamp all TX ring entries that are marked as
> > > > being FW OWNED but don't have a timestamp yet (and update your index).
> > > > 
> > > > Then when the time comes to process the TX packet (i.e. it has reached
> > > > the head of the TX queue), compare the timestamp with the current time.
> > > 
> > > Since firmware cannot access host memory directly, it needs additional
> > > dma operations to read all descriptors, then check the ownership bit and
> > > update the timestamp for the descriptors. It can easily take about 1us
> > > per descriptor for this and it can hit the peak throughput by about
> > > 2.5%.
> > 
> > That DMA needs to happen anyway, whether you have the host timestamp
> > the packets or not, so this doesn't seem to be an applicable argument.
> 
> Currently, firmware downloads a small fixed number of descriptors at a
> given time from any tx queue as it does not have memory to download all
> the descriptors from a host queue.
> 
> Assume that the total tx buffers available in firmware is X and host tx
> queue size is 2X and that the host queue is completely backlogged. In
> this case, the firmware needs to download the descriptors from the head
> of the host queue to transmit corresponding packets and it needs to
> download the descriptors at the tail of the host queue to timestamp
> them. But, firmware cannot transmit the packets associated with
> descriptors from the tail since there are packets in host queue which
> were queued earlier to these packets. This effectively results in each
> descriptor being downloaded twice - once for timestamp and once for
> transmission.

Point taken, but if the firmware has N buffers in its H/W TX ring,
does the host really need to have 2N buffers in its F/W TX ring?

There is another issue in that mwl8k H/W reports that a packet has been
transmitted OK even before the packet is attempted to be transmitted
(i.e. firmware marks the packet as transmitted as soon as the DMA for
the packet data completes), which sounds like a good candidate for
fixing at the same time.


> This will significantly affect the peak throughput performance.

Does that mean that you've implemented this and tried it out? :-)

Re: [PATCH 2/4] mwl8k: Add timestamp information for tx packets

[Nishant Sarmukadam]

On Thu, 2011-04-21 at 00:34 -0700, Lennert Buytenhek wrote:
> On Tue, Apr 19, 2011 at 06:36:41PM +0530, Nishant Sarmukadam wrote:
> 
> > > > > > 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.
> > > > > 
> > > > > Why don't you do this entirely in the firmware?
> > > > > 
> > > > > When a packet is put into the TX ring by the host, give it a timestamp
> > > > > of zero.
> > > > > 
> > > > > When the firmware gets a TX interrupt, have it walk the TX ring from
> > > > > the last TX entry where a timestamp was written to (i.e. keep a separate
> > > > > index for this), and timestamp all TX ring entries that are marked as
> > > > > being FW OWNED but don't have a timestamp yet (and update your index).
> > > > > 
> > > > > Then when the time comes to process the TX packet (i.e. it has reached
> > > > > the head of the TX queue), compare the timestamp with the current time.
> > > > 
> > > > Since firmware cannot access host memory directly, it needs additional
> > > > dma operations to read all descriptors, then check the ownership bit and
> > > > update the timestamp for the descriptors. It can easily take about 1us
> > > > per descriptor for this and it can hit the peak throughput by about
> > > > 2.5%.
> > > 
> > > That DMA needs to happen anyway, whether you have the host timestamp
> > > the packets or not, so this doesn't seem to be an applicable argument.
> > 
> > Currently, firmware downloads a small fixed number of descriptors at a
> > given time from any tx queue as it does not have memory to download all
> > the descriptors from a host queue.
> > 
> > Assume that the total tx buffers available in firmware is X and host tx
> > queue size is 2X and that the host queue is completely backlogged. In
> > this case, the firmware needs to download the descriptors from the head
> > of the host queue to transmit corresponding packets and it needs to
> > download the descriptors at the tail of the host queue to timestamp
> > them. But, firmware cannot transmit the packets associated with
> > descriptors from the tail since there are packets in host queue which
> > were queued earlier to these packets. This effectively results in each
> > descriptor being downloaded twice - once for timestamp and once for
> > transmission.
> 
> Point taken, but if the firmware has N buffers in its H/W TX ring,
> does the host really need to have 2N buffers in its F/W TX ring?

Yes, we have confirmed this and 2N buffers are needed in the host for
peak throughput. 
> 
> There is another issue in that mwl8k H/W reports that a packet has been
> transmitted OK even before the packet is attempted to be transmitted
> (i.e. firmware marks the packet as transmitted as soon as the DMA for
> the packet data completes), which sounds like a good candidate for
> fixing at the same time.
> 
Since, this issue is generic and needs to be addressed irrespective of
the current patch set, we prefer handling this in a separate patch.

> 
> > This will significantly affect the peak throughput performance.
> 
> Does that mean that you've implemented this and tried it out? :-)

Yes, we have recently done some profiling experiments in the firmware to
confirm this. 

[PATCH 3/4] mwl8k: Reserve buffers for tx management frames

[Nishant Sarmukadam]

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 <nishants@marvell.com>
Signed-off-by: Pradeep Nemavat <pnemavat@marvell.com>
---
 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

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

[Nishant Sarmukadam]

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 <nishants@marvell.com>
Signed-off-by: Pradeep Nemavat <pnemavat@marvell.com>
---
 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

Re: [PATCH 1/4] mwl8k: Do not stop tx queues

[Lennert Buytenhek]

On Tue, Apr 19, 2011 at 11:12:49AM +0530, Nishant Sarmukadam wrote:

> From: Pradeep Nemavat <pnemavat@marvell.com>
> 
> 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.
> 
> Since queues are not being stopped now, we need to be prepared for
> a situation where packets hit the driver after the queues are full.
> Drop all such packets in the driver itself.

What this patch seems to do is: don't propagate queue start/stop
indications to mac80211, and if a packet is handed to mwl8k for
transmission while a TX queue is full, just drop it in the driver.
(This doesn't seem to correspond with the commit message.)

But since the driver still has an accurate idea of TX queue fullness
(i.e. it's not as if at transmit time, you go and transparently reclaim
TX queue entries), why do you need to start/stop mac80211 TX queues at
all?  Won't it still work fine without deleting that logic?

Re: [PATCH 1/4] mwl8k: Do not stop tx queues

[Nishant Sarmukadam]

On Tue, 2011-04-19 at 00:48 -0700, Lennert Buytenhek wrote:
> On Tue, Apr 19, 2011 at 11:12:49AM +0530, Nishant Sarmukadam wrote:
> 
> > From: Pradeep Nemavat <pnemavat@marvell.com>
> > 
> > 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.
> > 
> > Since queues are not being stopped now, we need to be prepared for
> > a situation where packets hit the driver after the queues are full.
> > Drop all such packets in the driver itself.
> 
> What this patch seems to do is: don't propagate queue start/stop
> indications to mac80211, and if a packet is handed to mwl8k for
> transmission while a TX queue is full, just drop it in the driver.
> (This doesn't seem to correspond with the commit message.)

We were invoking mac80211 routines to start/stop tx queues. Since we do
not call these routines now, the queues will not be stopped anymore.
Hence, packets that enter the driver after the corresponding queue is
full, need to be dropped. This is mentioned in the commit description.
Please let me know if I am missing something. 

> 
> But since the driver still has an accurate idea of TX queue fullness
> (i.e. it's not as if at transmit time, you go and transparently reclaim
> TX queue entries), why do you need to start/stop mac80211 TX queues at
> all?  Won't it still work fine without deleting that logic?

We need to timestamp all tx packets. If we stop the queues, packets will
be queued up outside the driver (in queue full conditions). 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 the queues outside the driver. Such packets add up
to the overall latency considerably. Hence we are not stopping the
queues anymore. 

Re: [PATCH 1/4] mwl8k: Do not stop tx queues

[Lennert Buytenhek]

On Tue, Apr 19, 2011 at 03:38:17PM +0530, Nishant Sarmukadam wrote:

> > > 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.
> > > 
> > > Since queues are not being stopped now, we need to be prepared for
> > > a situation where packets hit the driver after the queues are full.
> > > Drop all such packets in the driver itself.
> > 
> > What this patch seems to do is: don't propagate queue start/stop
> > indications to mac80211, and if a packet is handed to mwl8k for
> > transmission while a TX queue is full, just drop it in the driver.
> > (This doesn't seem to correspond with the commit message.)
> 
> We were invoking mac80211 routines to start/stop tx queues. Since we do
> not call these routines now, the queues will not be stopped anymore.
> Hence, packets that enter the driver after the corresponding queue is
> full, need to be dropped. This is mentioned in the commit description.
> Please let me know if I am missing something. 

Maybe it's me, but to me, the commit message seems to say that queue
starting/stopping is already not being done anymore by mwl8k, and
because of that, we need to drop packets that are being transmitted
in the driver when the TX queue is full, which this patch does.


> > But since the driver still has an accurate idea of TX queue fullness
> > (i.e. it's not as if at transmit time, you go and transparently reclaim
> > TX queue entries), why do you need to start/stop mac80211 TX queues at
> > all?  Won't it still work fine without deleting that logic?
> 
> We need to timestamp all tx packets. If we stop the queues, packets will
> be queued up outside the driver (in queue full conditions). 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 the queues outside the driver.

This is a fair argument, and this really should be in the commit message.

Re: [PATCH 1/4] mwl8k: Do not stop tx queues

[Nishant Sarmukadam]

On Tue, 2011-04-19 at 03:26 -0700, Lennert Buytenhek wrote:
> On Tue, Apr 19, 2011 at 03:38:17PM +0530, Nishant Sarmukadam wrote:
> 
> > > > 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.
> > > > 
> > > > Since queues are not being stopped now, we need to be prepared for
> > > > a situation where packets hit the driver after the queues are full.
> > > > Drop all such packets in the driver itself.
> > > 
> > > What this patch seems to do is: don't propagate queue start/stop
> > > indications to mac80211, and if a packet is handed to mwl8k for
> > > transmission while a TX queue is full, just drop it in the driver.
> > > (This doesn't seem to correspond with the commit message.)
> > 
> > We were invoking mac80211 routines to start/stop tx queues. Since we do
> > not call these routines now, the queues will not be stopped anymore.
> > Hence, packets that enter the driver after the corresponding queue is
> > full, need to be dropped. This is mentioned in the commit description.
> > Please let me know if I am missing something. 
> 
> Maybe it's me, but to me, the commit message seems to say that queue
> starting/stopping is already not being done anymore by mwl8k, and
> because of that, we need to drop packets that are being transmitted
> in the driver when the TX queue is full, which this patch does.

Ok, I will try to address this in patch set V2.

> 
> 
> > > But since the driver still has an accurate idea of TX queue fullness
> > > (i.e. it's not as if at transmit time, you go and transparently reclaim
> > > TX queue entries), why do you need to start/stop mac80211 TX queues at
> > > all?  Won't it still work fine without deleting that logic?
> > 
> > We need to timestamp all tx packets. If we stop the queues, packets will
> > be queued up outside the driver (in queue full conditions). 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 the queues outside the driver.
> 
> This is a fair argument, and this really should be in the commit message.

I will incorporate this in patch set V2.