Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191007043120.67567-1-kyan@google.com> <20191007043120.67567-2-kyan@google.com>
 <18630c07d0aa46d16cf660d013f96b3d@codeaurora.org>
In-Reply-To: <18630c07d0aa46d16cf660d013f96b3d@codeaurora.org>
From:   Kan Yan <kyan@google.com>
Date:   Wed, 9 Oct 2019 15:44:37 -0700
Message-ID: <CA+iem5tVZLoisVF=pRR7+Jx04wCGz-CJh0oLKYa6=eQbDkOTKw@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
To:     Yibo Zhao <yiboz@codeaurora.org>
Cc:     Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org,
        make-wifi-fast@lists.bufferbloat.net,
        =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= <toke@redhat.com>,
        Felix Fietkau <nbd@nbd.name>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Hi  Johannes,

Thanks for the review and will address all issues you pointed out in
the next version.

Hi Yibo,
>
> I assume here the only txq in the list that does not meet AQL check will
> not be dequeued. Right? Will it affect peak throughput once there is
> only one station.

Yes, the txq won't be picked for transmitting even if it is the only
active txq if the AQL check failed.  However, this won't affect peak
throughput. The reason why there are two queue limits is address this
kind of situation. The higher queue limit ensures the hardware get
enough frames.
>
> > @@ -3748,10 +3785,10 @@ bool ieee80211_txq_may_transmit(struct
> > ieee80211_hw *hw,
> >       struct sta_info *sta;
> >       u8 ac = txq->ac;
> >
> > -     spin_lock_bh(&local->active_txq_lock[ac]);
> > -
> >       if (!txqi->txq.sta)
> > -             goto out;
> > +             return true;
>
> why return here? I think even a txq without sta info should get removed
> from list and added it back later in return_txq() if needed. No?
Yes, it should be removed from the active list. I will fix that.

Thanks,
Kan


On Wed, Oct 9, 2019 at 1:18 AM Yibo Zhao <yiboz@codeaurora.org> wrote:
>
> > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> > index f13eb2f61ccf..dadb643a5498 100644
> > --- a/net/mac80211/tx.c
> > +++ b/net/mac80211/tx.c
> > @@ -3669,7 +3669,8 @@ struct ieee80211_txq *ieee80211_next_txq(struct
> > ieee80211_hw *hw, u8 ac)
> >  {
> >       struct ieee80211_local *local = hw_to_local(hw);
> >       struct ieee80211_txq *ret = NULL;
> > -     struct txq_info *txqi = NULL;
> > +     struct txq_info *txqi = NULL, *head = NULL;
> > +     bool found_eligible_txq = false;
> >
> >       spin_lock_bh(&local->active_txq_lock[ac]);
> >
> > @@ -3680,20 +3681,32 @@ struct ieee80211_txq
> > *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac)
> >       if (!txqi)
> >               goto out;
> >
> > +     if (txqi == head && !found_eligible_txq)
> > +             goto out;
>
> I assume here the only txq in the list that does not meet AQL check will
> not be dequeued. Right? Will it affect peak throughput once there is
> only one station.
>
> How about dequeuing it anyway regardless AQL because it is the only one
> active now so it is fine to occupy the rest bandwidth. Otherwise, I am
> afraid next_txq() will return NULL in the test only one station is
> present.
>
> > @@ -3748,10 +3785,10 @@ bool ieee80211_txq_may_transmit(struct
> > ieee80211_hw *hw,
> >       struct sta_info *sta;
> >       u8 ac = txq->ac;
> >
> > -     spin_lock_bh(&local->active_txq_lock[ac]);
> > -
> >       if (!txqi->txq.sta)
> > -             goto out;
> > +             return true;
>
> why return here? I think even a txq without sta info should get removed
> from list and added it back later in return_txq() if needed. No?
>
> > +
> > +     spin_lock_bh(&local->active_txq_lock[ac]);
> >
> >       if (list_empty(&txqi->schedule_order))
> >               goto out;
>
>
> --
> Yibo