On Fri, 2014-06-06 at 07:51 +0200, Michal Kazior wrote:
> On 5 June 2014 18:26, Emmanuel Grumbach <emmanuel.grumbach@xxxxxxxxx> wrote:
> > From: Luciano Coelho <luciano.coelho@xxxxxxxxx>
> >
> > Instead of stopping all the hardware queues during channel switch,
> > which is especially bad when we have large CSA counts, stop only the
> > queues that are assigned to the vif that is performing the channel
> > switch.
> >
> > Signed-off-by: Luciano Coelho <luciano.coelho@xxxxxxxxx>
> > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@xxxxxxxxx>
> > ---
> > net/mac80211/cfg.c | 10 ++++------
> > net/mac80211/iface.c | 5 ++---
> > net/mac80211/mlme.c | 20 ++++++++------------
> > 3 files changed, 14 insertions(+), 21 deletions(-)
> >
> > diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> > index c6fe358..5215abb 100644
> > --- a/net/mac80211/cfg.c
> > +++ b/net/mac80211/cfg.c
> > @@ -1142,9 +1142,8 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
> > mutex_lock(&local->mtx);
> > sdata->vif.csa_active = false;
> > if (!ieee80211_csa_needs_block_tx(local))
> > - ieee80211_wake_queues_by_reason(&local->hw,
> > - IEEE80211_MAX_QUEUE_MAP,
> > - IEEE80211_QUEUE_STOP_REASON_CSA);
> > + ieee80211_wake_vif_queues(local, sdata,
> > + IEEE80211_QUEUE_STOP_REASON_CSA);
> > mutex_unlock(&local->mtx);
> >
> > kfree(sdata->u.ap.next_beacon);
> > @@ -3146,9 +3145,8 @@ static int __ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
> > cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
> >
> > if (!ieee80211_csa_needs_block_tx(local))
> > - ieee80211_wake_queues_by_reason(&local->hw,
> > - IEEE80211_MAX_QUEUE_MAP,
> > - IEEE80211_QUEUE_STOP_REASON_CSA);
> > + ieee80211_wake_vif_queues(local, sdata,
> > + IEEE80211_QUEUE_STOP_REASON_CSA);
>
> I don't think this is going to work with the upcomming multi-vif
> channel switching.
>
> The ieee80211_csa_needs_block_tx() will become false upon *last*
> interface switch completion. This means preceeding interface csa
> finalizations won't call ieee80211_wake_vif_queues() so you actually
> end up not waking all of the queues when you finish csa.
I think the right way to do it would be to traverse the vifs that
switched and wake up their queues when the switch is finished.
> I think this can be solved with:
>
> if (!ieee80211_csa_needs_block_tx(local))
> list_for_each(sdata, &local->interfaces, list)
> ieee80211_wake_vif_queues(local, sdata, REASON_CSA);
>
> But then I guess it's just a convoluted way of saying:
>
> if (!ieee80211_csa_needs_block_tx(local))
> ieee80211_wake_queues_by_reason(hw, REASON_CSA);
>
> IOW waking should remain as it was while stopping can be done per-vif.
>
> This goes for all instances calling wake_vif_queues() in the patch.
Actually I had it like this in my first version, I had only added the
stop part, leaving the wake as it was. But then I thought that we would
be restarting queues from other vifs that we don't know about. If
another vif is switching separately, we would restart its queues at the
wrong time. I think we shouldn't assume that all vifs are part of the
same switch.
> Generally I don't think having wake_vif_queues() is a good idea. It's
> really tricky. Sure, you can have a stop_vif_queues() which will
> behave as you might expect (i.e. first call stops queues and
> overlapping vif-queue mappings are not a concern). But
> wake_vif_queues() seems to have little practical use to me without any
> kind of per-queue reference counting.
Yeah, this could be a problem for vifs that are sharing the same queues.
But that case would be really tricky with channel-switch anyway. because
if the channel of one vif is changed but not the other, the queue would
probably have to be split...
--
Luca.
��.n��������+%������w��{.n�����{���zW����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
