| Message ID | 1412517432-16309-1-git-send-email-emmanuel.grumbach@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 :) -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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 :) -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)