[RFT] iwlwifi: dvm: drop non VO frames when flushing

[RFT] iwlwifi: dvm: drop non VO frames when flushing

[Emmanuel Grumbach]

When mac80211 wants to ensure that a frame is sent, it calls
the flush() callback. Until now, iwldvm implemented this by
waiting that all the frames are sent (ACKed or timeout).
In case of weak signal, this can take a significant amount
of time, delaying the next connection (in case of roaming).
Many users have reported that the flush would take too long
leading to the following error messages to be printed:

iwlwifi 0000:03:00.0: fail to flush all tx fifo queues Q 2
iwlwifi 0000:03:00.0: Current SW read_ptr 161 write_ptr 201
iwl data: 00000000: 00 00 00 00 00 00 00 00 fe ff 01 00 00 00 00 00
[snip]
iwlwifi 0000:03:00.0: FH TRBs(0) = 0x00000000
[snip]
iwlwifi 0000:03:00.0: Q 0 is active and mapped to fifo 3 ra_tid 0x0000 [9,9]
[snip]

Instead of waiting for these packets, simply drop them. This
significantly improves the responsiveness of the network.
Note that all the queues are flushed, but the VO one. This
is not typically used by the applications and it likely
contains management frames that are useful for connection
or roaming.

This bug is tracked here:
https://bugzilla.kernel.org/show_bug.cgi?id=56581

But it is duplicated in distributions' trackers.
A simple search in Ubuntu's database led to these bugs:

https://bugs.launchpad.net/ubuntu/+source/linux-firmware/+bug/1270808
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1305406
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1356236
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1360597
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1361809

Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
---
I am trying here to fix an issues that has been affecting many users on dvm.
A few users in bugzilla already reported that this patch helped them a lot.
I am trying to get more testers :)
---
 drivers/net/wireless/iwlwifi/dvm/mac80211.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/dvm/mac80211.c b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
index d8ebc7e..b1a23dc 100644
--- a/drivers/net/wireless/iwlwifi/dvm/mac80211.c
+++ b/drivers/net/wireless/iwlwifi/dvm/mac80211.c
@@ -1095,6 +1095,7 @@ static void iwlagn_mac_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 			     u32 queues, bool drop)
 {
 	struct iwl_priv *priv = IWL_MAC80211_GET_DVM(hw);
+	u32 scd_queues;
 
 	mutex_lock(&priv->mutex);
 	IWL_DEBUG_MAC80211(priv, "enter\n");
@@ -1108,18 +1109,19 @@ static void iwlagn_mac_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 		goto done;
 	}
 
-	/*
-	 * mac80211 will not push any more frames for transmit
-	 * until the flush is completed
-	 */
-	if (drop) {
-		IWL_DEBUG_MAC80211(priv, "send flush command\n");
-		if (iwlagn_txfifo_flush(priv, 0)) {
-			IWL_ERR(priv, "flush request fail\n");
-			goto done;
-		}
+	scd_queues = BIT(priv->cfg->base_params->num_of_queues) - 1;
+	scd_queues &= ~(BIT(IWL_IPAN_CMD_QUEUE_NUM) |
+			BIT(IWL_DEFAULT_CMD_QUEUE_NUM));
+
+	if (vif)
+		scd_queues &= ~BIT(vif->hw_queue[IEEE80211_AC_VO]);
+
+	IWL_DEBUG_TX_QUEUES(priv, "Flushing SCD queues: 0x%x\n", scd_queues);
+	if (iwlagn_txfifo_flush(priv, scd_queues)) {
+		IWL_ERR(priv, "flush request fail\n");
+		goto done;
 	}
-	IWL_DEBUG_MAC80211(priv, "wait transmit/flush all frames\n");
+	IWL_DEBUG_TX_QUEUES(priv, "wait transmit/flush all frames\n");
 	iwl_trans_wait_tx_queue_empty(priv->trans, 0xffffffff);
 done:
 	mutex_unlock(&priv->mutex);
-- 
1.9.1

Re: [RFT] iwlwifi: dvm: drop non VO frames when flushing

[Sujith Manoharan]

Emmanuel Grumbach wrote:
> When mac80211 wants to ensure that a frame is sent, it calls
> the flush() callback. Until now, iwldvm implemented this by
> waiting that all the frames are sent (ACKed or timeout).
> In case of weak signal, this can take a significant amount
> of time, delaying the next connection (in case of roaming).

ath9k does pretty much the same - just wait for pending
frames to be completed.

mac80211 doesn't seem to set 'drop' anywhere when calling flush(),
so ath9k ends up waiting for frame completion in all cases too.
Maybe this might cause inordinate delays in low-signal connections
with ath9k too...

Sujith

RE: [RFT] iwlwifi: dvm: drop non VO frames when flushing

[Grumbach, Emmanuel]

> 
> Emmanuel Grumbach wrote:
> > When mac80211 wants to ensure that a frame is sent, it calls the
> > flush() callback. Until now, iwldvm implemented this by waiting that
> > all the frames are sent (ACKed or timeout).
> > In case of weak signal, this can take a significant amount of time,
> > delaying the next connection (in case of roaming).
> 
> ath9k does pretty much the same - just wait for pending frames to be
> completed.
> 
> mac80211 doesn't seem to set 'drop' anywhere when calling flush(), so ath9k
> ends up waiting for frame completion in all cases too.
> Maybe this might cause inordinate delays in low-signal connections with
> ath9k too...
> 

I can't really tell... I guess it depends on the firmware / hardware. On these devices (dvm),
the firmware would take a very long time to send / drop the packets. I guess it is a bug, and
users have been suffering from this for a very long time. Thing is that I was educated by the
"Thee shall not drop packets" mantra. But it appears that sometimes, it is very healthy to
drop packets to solve the traffic jam. Here, I just drop the packets besides the VO queue
to allow the roaming mgmt frames to reach their goal (I rely on the fact that the VO queue
will not be too large...). TCP will retry these packets when the link will be much better after
roaming.
Users in bugzilla said that it improved dramatically the overall behavior.

RE: [RFT] iwlwifi: dvm: drop non VO frames when flushing

[Sujith Manoharan]

Grumbach, Emmanuel wrote:
> I can't really tell... I guess it depends on the firmware / hardware. On these
> devices (dvm), the firmware would take a very long time to send / drop the
> packets. I guess it is a bug, and users have been suffering from this for a
> very long time. Thing is that I was educated by the "Thee shall not drop
> packets" mantra. But it appears that sometimes, it is very healthy to drop
> packets to solve the traffic jam. Here, I just drop the packets besides the VO
> queue to allow the roaming mgmt frames to reach their goal (I rely on the fact
> that the VO queue will not be too large...).

Ok.

ath9k does have a timeout, so we drain (drop) all pending frames if flushing
doesn't complete within a reasonable time.

Sujith

Re: [RFT] iwlwifi: dvm: drop non VO frames when flushing

[Seth Forshee]

On Sun, Oct 05, 2014 at 04:57:12PM +0300, Emmanuel Grumbach wrote:
> When mac80211 wants to ensure that a frame is sent, it calls
> the flush() callback. Until now, iwldvm implemented this by
> waiting that all the frames are sent (ACKed or timeout).
> In case of weak signal, this can take a significant amount
> of time, delaying the next connection (in case of roaming).
> Many users have reported that the flush would take too long
> leading to the following error messages to be printed:
> 
> iwlwifi 0000:03:00.0: fail to flush all tx fifo queues Q 2
> iwlwifi 0000:03:00.0: Current SW read_ptr 161 write_ptr 201
> iwl data: 00000000: 00 00 00 00 00 00 00 00 fe ff 01 00 00 00 00 00
> [snip]
> iwlwifi 0000:03:00.0: FH TRBs(0) = 0x00000000
> [snip]
> iwlwifi 0000:03:00.0: Q 0 is active and mapped to fifo 3 ra_tid 0x0000 [9,9]
> [snip]
> 
> Instead of waiting for these packets, simply drop them. This
> significantly improves the responsiveness of the network.
> Note that all the queues are flushed, but the VO one. This
> is not typically used by the applications and it likely
> contains management frames that are useful for connection
> or roaming.
> 
> This bug is tracked here:
> https://bugzilla.kernel.org/show_bug.cgi?id=56581
> 
> But it is duplicated in distributions' trackers.
> A simple search in Ubuntu's database led to these bugs:
> 
> https://bugs.launchpad.net/ubuntu/+source/linux-firmware/+bug/1270808
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1305406
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1356236
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1360597
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1361809
> 
> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> ---
> I am trying here to fix an issues that has been affecting many users on dvm.
> A few users in bugzilla already reported that this patch helped them a lot.
> I am trying to get more testers :)

I'll post test builds to the Ubuntu bugs and see if I can get any
testing.

Thanks,
Seth

Re: [RFT] iwlwifi: dvm: drop non VO frames when flushing

[Seth Forshee]

On Sun, Oct 05, 2014 at 04:57:12PM +0300, Emmanuel Grumbach wrote:
> +	if (vif)
> +		scd_queues &= ~BIT(vif->hw_queue[IEEE80211_AC_VO]);

I'm backporting this to 3.13, and this part doesn't work unless
77be2c54c5bd26279abc13807398771d80cda37a is also backported. Is this
critical, or can it be omitted in the backport?

Thanks,
Seth

RE: [RFT] iwlwifi: dvm: drop non VO frames when flushing

[Grumbach, Emmanuel]

> Subject: Re: [RFT] iwlwifi: dvm: drop non VO frames when flushing
> 
> On Sun, Oct 05, 2014 at 04:57:12PM +0300, Emmanuel Grumbach wrote:
> > +	if (vif)
> > +		scd_queues &= ~BIT(vif->hw_queue[IEEE80211_AC_VO]);
> 
> I'm backporting this to 3.13, and this part doesn't work unless
> 77be2c54c5bd26279abc13807398771d80cda37a is also backported. Is this
> critical, or can it be omitted in the backport?
> 

77be2c54c5bd26279abc13807398771d80cda37a isn't really critical, but it is a dependency, and it is safe IMO.
But I'd wait for a bit more testing :) The patch isn't even in my tree yet :)

Re: [RFT] iwlwifi: dvm: drop non VO frames when flushing

[Seth Forshee]

On Mon, Oct 06, 2014 at 12:35:46PM +0000, Grumbach, Emmanuel wrote:
> > Subject: Re: [RFT] iwlwifi: dvm: drop non VO frames when flushing
> > 
> > On Sun, Oct 05, 2014 at 04:57:12PM +0300, Emmanuel Grumbach wrote:
> > > +	if (vif)
> > > +		scd_queues &= ~BIT(vif->hw_queue[IEEE80211_AC_VO]);
> > 
> > I'm backporting this to 3.13, and this part doesn't work unless
> > 77be2c54c5bd26279abc13807398771d80cda37a is also backported. Is this
> > critical, or can it be omitted in the backport?
> > 
> 
> 77be2c54c5bd26279abc13807398771d80cda37a isn't really critical, but it is a dependency, and it is safe IMO.
> But I'd wait for a bit more testing :) The patch isn't even in my tree yet :)

My backport is for testing too. Most of our bugs are against Ubuntu
14.04, which uses 3.13. Seems better to have them test this change in
isolation rather than also testing everything which has changed up to
3.17.

I agree the patch is safe, but I'd also prefer to have it tested in the
form which would eventually get applied to the 3.13 extended stable
tree. So I take it for stable you would advocate applying both patches?

Re: [RFT] iwlwifi: dvm: drop non VO frames when flushing

[Seth Forshee]

On Mon, Oct 06, 2014 at 07:43:07AM -0500, Seth Forshee wrote:
> On Mon, Oct 06, 2014 at 12:35:46PM +0000, Grumbach, Emmanuel wrote:
> > > Subject: Re: [RFT] iwlwifi: dvm: drop non VO frames when flushing
> > > 
> > > On Sun, Oct 05, 2014 at 04:57:12PM +0300, Emmanuel Grumbach wrote:
> > > > +	if (vif)
> > > > +		scd_queues &= ~BIT(vif->hw_queue[IEEE80211_AC_VO]);
> > > 
> > > I'm backporting this to 3.13, and this part doesn't work unless
> > > 77be2c54c5bd26279abc13807398771d80cda37a is also backported. Is this
> > > critical, or can it be omitted in the backport?
> > > 
> > 
> > 77be2c54c5bd26279abc13807398771d80cda37a isn't really critical, but it is a dependency, and it is safe IMO.
> > But I'd wait for a bit more testing :) The patch isn't even in my tree yet :)
> 
> My backport is for testing too. Most of our bugs are against Ubuntu
> 14.04, which uses 3.13. Seems better to have them test this change in
> isolation rather than also testing everything which has changed up to
> 3.17.
> 
> I agree the patch is safe,

Sorry, I was ambiguous here. I mean that
77be2c54c5bd26279abc13807398771d80cda37a is safe to backport.

> but I'd also prefer to have it tested in the
> form which would eventually get applied to the 3.13 extended stable
> tree. So I take it for stable you would advocate applying both patches?

RE: [RFT] iwlwifi: dvm: drop non VO frames when flushing

[Grumbach, Emmanuel]

> 
> On Mon, Oct 06, 2014 at 07:43:07AM -0500, Seth Forshee wrote:
> > On Mon, Oct 06, 2014 at 12:35:46PM +0000, Grumbach, Emmanuel wrote:
> > > > Subject: Re: [RFT] iwlwifi: dvm: drop non VO frames when flushing
> > > >
> > > > On Sun, Oct 05, 2014 at 04:57:12PM +0300, Emmanuel Grumbach wrote:
> > > > > +	if (vif)
> > > > > +		scd_queues &= ~BIT(vif-
> >hw_queue[IEEE80211_AC_VO]);
> > > >
> > > > I'm backporting this to 3.13, and this part doesn't work unless
> > > > 77be2c54c5bd26279abc13807398771d80cda37a is also backported. Is
> > > > this critical, or can it be omitted in the backport?
> > > >
> > >
> > > 77be2c54c5bd26279abc13807398771d80cda37a isn't really critical, but it is
> a dependency, and it is safe IMO.
> > > But I'd wait for a bit more testing :) The patch isn't even in my
> > > tree yet :)
> >
> > My backport is for testing too. Most of our bugs are against Ubuntu
> > 14.04, which uses 3.13. Seems better to have them test this change in
> > isolation rather than also testing everything which has changed up to
> > 3.17.
> >
> > I agree the patch is safe,
> 
> Sorry, I was ambiguous here. I mean that
> 77be2c54c5bd26279abc13807398771d80cda37a is safe to backport.
> 
> > but I'd also prefer to have it tested in the form which would
> > eventually get applied to the 3.13 extended stable tree. So I take it
> > for stable you would advocate applying both patches?

I guess... I can rework the discussed patch (drop non VO ...) to work without
77be2c54c5bd26279abc13807398771d80cda37a, but is it really worth it?
I don't see any reason not to backport 77be2c54c5bd26279abc13807398771d80cda37a.
IMHO, the easiest is to backport 77be2c54c5bd26279abc13807398771d80cda37a and
apply the drop non VO on top of it.

What am I missing?

Re: [RFT] iwlwifi: dvm: drop non VO frames when flushing

[Seth Forshee]

On Mon, Oct 06, 2014 at 12:47:41PM +0000, Grumbach, Emmanuel wrote:
> > 
> > On Mon, Oct 06, 2014 at 07:43:07AM -0500, Seth Forshee wrote:
> > > On Mon, Oct 06, 2014 at 12:35:46PM +0000, Grumbach, Emmanuel wrote:
> > > > > Subject: Re: [RFT] iwlwifi: dvm: drop non VO frames when flushing
> > > > >
> > > > > On Sun, Oct 05, 2014 at 04:57:12PM +0300, Emmanuel Grumbach wrote:
> > > > > > +	if (vif)
> > > > > > +		scd_queues &= ~BIT(vif-
> > >hw_queue[IEEE80211_AC_VO]);
> > > > >
> > > > > I'm backporting this to 3.13, and this part doesn't work unless
> > > > > 77be2c54c5bd26279abc13807398771d80cda37a is also backported. Is
> > > > > this critical, or can it be omitted in the backport?
> > > > >
> > > >
> > > > 77be2c54c5bd26279abc13807398771d80cda37a isn't really critical, but it is
> > a dependency, and it is safe IMO.
> > > > But I'd wait for a bit more testing :) The patch isn't even in my
> > > > tree yet :)
> > >
> > > My backport is for testing too. Most of our bugs are against Ubuntu
> > > 14.04, which uses 3.13. Seems better to have them test this change in
> > > isolation rather than also testing everything which has changed up to
> > > 3.17.
> > >
> > > I agree the patch is safe,
> > 
> > Sorry, I was ambiguous here. I mean that
> > 77be2c54c5bd26279abc13807398771d80cda37a is safe to backport.
> > 
> > > but I'd also prefer to have it tested in the form which would
> > > eventually get applied to the 3.13 extended stable tree. So I take it
> > > for stable you would advocate applying both patches?
> 
> I guess... I can rework the discussed patch (drop non VO ...) to work without
> 77be2c54c5bd26279abc13807398771d80cda37a, but is it really worth it?
> I don't see any reason not to backport 77be2c54c5bd26279abc13807398771d80cda37a.
> IMHO, the easiest is to backport 77be2c54c5bd26279abc13807398771d80cda37a and
> apply the drop non VO on top of it.
> 
> What am I missing?

Nothing, except that stable kernel rules dictate that only bug fixes
will be applied. 77be2c54c5 isn't a bug fix. If it's a necessary
prerequisite then I suspect they'd be accomodating, but if it's not
necessary then they might request a backport which doesn't require
77be2c54c5.

No sense it beating it to death now though. I'll backport both, and if
the stable trees end up with something different I can ask for
additional testing. Especially if it involves more than simply dropping
those two lines.

Thanks,
Seth

RE: [RFT] iwlwifi: dvm: drop non VO frames when flushing

[Grumbach, Emmanuel]

> 
> On Mon, Oct 06, 2014 at 12:47:41PM +0000, Grumbach, Emmanuel wrote:
> > >
> > > On Mon, Oct 06, 2014 at 07:43:07AM -0500, Seth Forshee wrote:
> > > > On Mon, Oct 06, 2014 at 12:35:46PM +0000, Grumbach, Emmanuel
> wrote:
> > > > > > Subject: Re: [RFT] iwlwifi: dvm: drop non VO frames when
> > > > > > flushing
> > > > > >
> > > > > > On Sun, Oct 05, 2014 at 04:57:12PM +0300, Emmanuel Grumbach
> wrote:
> > > > > > > +	if (vif)
> > > > > > > +		scd_queues &= ~BIT(vif-
> > > >hw_queue[IEEE80211_AC_VO]);
> > > > > >
> > > > > > I'm backporting this to 3.13, and this part doesn't work
> > > > > > unless 77be2c54c5bd26279abc13807398771d80cda37a is also
> > > > > > backported. Is this critical, or can it be omitted in the backport?
> > > > > >
> > > > >
> > > > > 77be2c54c5bd26279abc13807398771d80cda37a isn't really critical,
> > > > > but it is
> > > a dependency, and it is safe IMO.
> > > > > But I'd wait for a bit more testing :) The patch isn't even in
> > > > > my tree yet :)
> > > >
> > > > My backport is for testing too. Most of our bugs are against
> > > > Ubuntu 14.04, which uses 3.13. Seems better to have them test this
> > > > change in isolation rather than also testing everything which has
> > > > changed up to 3.17.
> > > >
> > > > I agree the patch is safe,
> > >
> > > Sorry, I was ambiguous here. I mean that
> > > 77be2c54c5bd26279abc13807398771d80cda37a is safe to backport.
> > >
> > > > but I'd also prefer to have it tested in the form which would
> > > > eventually get applied to the 3.13 extended stable tree. So I take
> > > > it for stable you would advocate applying both patches?
> >
> > I guess... I can rework the discussed patch (drop non VO ...) to work
> > without 77be2c54c5bd26279abc13807398771d80cda37a, but is it really
> worth it?
> > I don't see any reason not to backport
> 77be2c54c5bd26279abc13807398771d80cda37a.
> > IMHO, the easiest is to backport
> > 77be2c54c5bd26279abc13807398771d80cda37a and apply the drop non VO
> on top of it.
> >
> > What am I missing?
> 
> Nothing, except that stable kernel rules dictate that only bug fixes will be
> applied. 77be2c54c5 isn't a bug fix. If it's a necessary prerequisite then I
> suspect they'd be accomodating, but if it's not necessary then they might
> request a backport which doesn't require 77be2c54c5.
> 
> No sense it beating it to death now though. I'll backport both, and if the
> stable trees end up with something different I can ask for additional testing.
> Especially if it involves more than simply dropping those two lines.
> 

Yeah - I know the rules, but heh... You can even either:
1) a patch that is not a bug fix and then no conflicts
2) a conflict which can be tricky (bug prone) to solve

Sometimes rules need to be challenged :)