DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D57A72087A
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Wed, 10 Oct 2018 16:20:02 -0700
From:   Rajkumar Manoharan <rmanohar@codeaurora.org>
To:     =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>
Cc:     linux-wireless@vger.kernel.org,
        make-wifi-fast@lists.bufferbloat.net, Felix Fietkau <nbd@nbd.name>,
        Kan Yan <kyan@google.com>, linux-wireless-owner@vger.kernel.org
Subject: Re: [PATCH RFC v5 3/4] mac80211: Add airtime accounting and
 scheduling to TXQs
In-Reply-To: <87zhvm832s.fsf@toke.dk>
References: <153908805217.9471.9290979918041653328.stgit@alrua-kau>
 <153908837900.9471.5394468800857658136.stgit@alrua-kau>
 <a65909ee425585a49d9e17f6504cf0ba@codeaurora.org> <87zhvm832s.fsf@toke.dk>
Message-ID: <187bade306627912c70d800819ef0b87@codeaurora.org>
User-Agent: Roundcube Webmail/1.2.5
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 2018-10-10 04:15, Toke Høiland-Jørgensen wrote:
> Rajkumar Manoharan <rmanohar@codeaurora.org> writes:
> 
>> On 2018-10-09 05:32, Toke Høiland-Jørgensen wrote:
>>> This adds airtime accounting and scheduling to the mac80211 TXQ
>>> scheduler. A new callback, ieee80211_sta_register_airtime(), is added
>>> that drivers can call to report airtime usage for stations.
>>> 
>>> When airtime information is present, mac80211 will schedule TXQs
>>> (through ieee80211_next_txq()) in a way that enforces airtime 
>>> fairness
>>> between active stations. This scheduling works the same way as the
>>> ath9k
>>> in-driver airtime fairness scheduling. If no airtime usage is 
>>> reported
>>> by the driver, the scheduler will default to round-robin scheduling.
>>> 
>>> For drivers that don't control TXQ scheduling in software, a new API
>>> function, ieee80211_txq_may_transmit(), is added which the driver can
>>> use
>>> to check if the TXQ is eligible for transmission, or should be
>>> throttled to
>>> enforce fairness. Calls to this function must also be enclosed in
>>> ieee80211_txq_schedule_{start,end}() calls to ensure proper locking.
>>> TXQs
>>> that are throttled by ieee802111_txq_may_transmit() will be woken up
>>> again
>>> by a check added to the ieee80211_wake_txqs() tasklet.
>>> 
>> 
>> Toke,
>> 
>> I am observing soft lockup issues again with this new series while
>> running traffic with 50 clients. I am continuing testing with earlier
>> series along with snippet I shared.
> 
> Are these new lockups (that was not in your patched previous version),
> or did I just not get all your lock-related fixes incorporated?
> 
>> When driver operates in pull-mode, throttled txqs are marked and
>> refilled in airtime_tasklet. This is causing major throughput drops
>> and packet loss and I am suspecting the latency in replenishing
>> deficit. Whereas in push-mode or in ath9k model, refill happens
>> quicker at every packet indication as well as tx completion.
> 
> Yeah, the tasklet shouldn't be the main source of deficit replenishing.
> Can see why that would give bad performance :)
> 
>> I am planning to get rid of tasklet completely as it is only meant for
>> pull-mode. It would be better to refill in may_transmit() itself.
> 
> Hmm, right. So the way to do this correctly (from a fairness point of
> view) would be something like this (in max_tx()):
> 
> if (this_txq.stn.deficit > 0)
>   return true;
> 
> else if (any queued TXQ currently have positive deficit)
>   return false; /* other TXQ should try may_tx() later and get 
> permission */
> 
> else /* all deficits < 0 */
>   return replenish_deficits(this_txq);
> 
> And replenish_deficits() would be something like:
> 
> replenish_deficits(this_txq) {
> repeat:
>   for (txq in queued txqs) {
>     txq.stn.deficit += stn.weight;
>     if (txq.stn.deficit > 0 && !wake_txq)
>       wake_txq = txq;
>   }
>   if not wake_txq:
>     goto repeat;
> 
>   if (this_txq.stn.deficit > 0)
>     return true;
>   else
>     drv_wake_tx_queue(wake_txq);
> }
> 
> The wake_tx_queue call may have to be delegated to a tasklet still, to
> avoid the infinite recursion problem I mentioned earlier. But the
> tasklet could be made simpler and wouldn't have to be called so 
> often...
> 
> Does the above make sense?
> 
Hmm... mine is bit different. txqs are refilled only once for all txqs.
It will give more opportunity for non-served txqs. drv_wake_tx_queue 
won't be
called from may_tx as the driver anyway will not push packets in 
pull-mode.

bool ieee80211_txq_may_transmit(struct ieee80211_hw *hw,
                                 struct ieee80211_txq *txq)
{
         struct ieee80211_local *local = hw_to_local(hw);
         struct txq_info *txqi = to_txq_info(txq);
         struct sta_info *sta;
         u8 ac = txq->ac;

         lockdep_assert_held(&local->active_txq_lock[ac]);

         if (!txqi->txq.sta)
                 goto out;

         sta = container_of(txqi->txq.sta, struct sta_info, sta);
         if (sta->airtime[ac].deficit >= 0)
                 goto out;

         list_for_each_entry(txqi, &local->active_txqs[ac], 
schedule_order) {
                 if (!txqi->txq.sta)
                         continue;
                 sta = container_of(txqi->txq.sta, struct sta_info, sta);
                 sta->airtime[ac].deficit += sta->airtime_weight;
         }

         return false;

  out:
         list_del_init(&txqi->schedule_order);
         return true;
}

-Rajkumar