From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 1DA233B29D for ; Fri, 11 Sep 2020 10:36:50 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1599835009; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=hfz2/7wuGD8Y38djXdRgoUk4t9vywrYM1kI/TWx1ITc=; b=MKhpwtQjHpk6np4CHedmFE7O5r2EkL7KLqFBHDcrZw49Z1MpVZbPSzyYa719x9/FmMQIVn 32jrRRJplWApxvc49PUhSXDANV1/UDjtRY2jD8KRBju7aJWBFJqtlefe3E3Vz1jdWRkUuc XiiVxDI8p/3aq7dXdwGALAIPnwnV5+Q= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-180-BvhC60VjNWu8HdHJomoo3A-1; Fri, 11 Sep 2020 10:36:47 -0400 X-MC-Unique: BvhC60VjNWu8HdHJomoo3A-1 Received: by mail-wr1-f69.google.com with SMTP id l15so3574449wro.10 for ; Fri, 11 Sep 2020 07:36:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version; bh=hfz2/7wuGD8Y38djXdRgoUk4t9vywrYM1kI/TWx1ITc=; b=Xgp6s1jWl30hCD5x8lZGgOK4Auqdj6+S6Uzj2A1s8CaPf7Z6BNHKrJugtnQ9y/0IQ2 e0GiEzX9BtqlKa/Qx+yjyQL5B3lgJnr7lgF0BZ1DRjYZgKvUlXKIP0sr024pBPtVwC74 8aM0/NaKFHw+j+heSESuCtca2a3n9L1FTQV+phzHDlSVljdXpcXmz5jH4fenTtRrj3T1 0Gli//fln0t1VNxd5pVkLTlFzDY0fyJeGggnENo52qP4X15TseEAIsP4nTNgNj/dikVz zipaxYtSUL4pN4goZP2NJRgfXNHlfuHGuAnbjJlt6R7AqW3AZiN/K9LE0F0Ocv9jCYus rIAQ== X-Gm-Message-State: AOAM533zlgr3Kk0l3iFQxU6Wr2lX4N+ageko36crLso/A+Z0Ur4gQA2h wN8D7BCLBHYv0m0E4/1U5fqUyu7Mpc2y3eF5kK8Bc9VT+cOaAsKSwHaPvWbx4nQu98LOIZfsqT3 6WKRfucf2hLk18mqrlFE9zg88hCgoboA0o8M= X-Received: by 2002:a05:600c:230f:: with SMTP id 15mr2551311wmo.186.1599835006037; Fri, 11 Sep 2020 07:36:46 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxI6BrikLZy71rWmHNHzSrhZH9gf7fYU6Cn05cLB9G07nNv9LxRdHcIEjCuBRxPjElcafqyKg== X-Received: by 2002:a05:600c:230f:: with SMTP id 15mr2551273wmo.186.1599835005573; Fri, 11 Sep 2020 07:36:45 -0700 (PDT) Received: from alrua-x1.borgediget.toke.dk ([2a0c:4d80:42:443::2]) by smtp.gmail.com with ESMTPSA id v6sm4810082wrt.90.2020.09.11.07.36.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 11 Sep 2020 07:36:44 -0700 (PDT) Received: by alrua-x1.borgediget.toke.dk (Postfix, from userid 1000) id 396561829D4; Fri, 11 Sep 2020 16:36:44 +0200 (CEST) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= To: Miguel Catalan Cid Cc: make-wifi-fast@lists.bufferbloat.net, linux-wireless@vger.kernel.org In-Reply-To: References: <87zh8uruou.fsf@toke.dk> <87366ei2x7.fsf@toke.dk> <87lfk4ftnc.fsf@toke.dk> X-Clacks-Overhead: GNU Terry Pratchett Date: Fri, 11 Sep 2020 16:36:44 +0200 Message-ID: <87k0x09y83.fsf@toke.dk> MIME-Version: 1.0 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=toke@redhat.com X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain Subject: Re: [Make-wifi-fast] Support for airtime scheduling using ath10k X-BeenThere: make-wifi-fast@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Sep 2020 14:36:50 -0000 Miguel Catalan Cid writes: > Hi, > > after doing some tests with different ath10k Wi-Fi cards and clients, I > found the following behaviour when combining AQL and the airtime > scheduler: Thank you for taking a closer look at this! > - When using the default AQL limits (threshold 24000, limits per AC > 5000/12000), the airtime scheduler is not working at all, regardless of the > airtime weights of the STAs. Indeed, in some cases, slower stations were > able to use a higher amount of airtime, leading to unfairness. I was > thinking that maybe the default AQL limits are too high to these slow > stations, allowing them to obtain too much pending airtime. I already used > the last patches from Felix Fietkau with the same results. Hmm, I think I can see what's going on here: The AQL patches changed the behaviour of ieee80211_next_txq() so it will keep iterating as long as there's at least one station that's eligible for transmission from the AQL PoV. Which means all station deficits will keep increasing, erasing the fairness throttling... > - Indeed, I was able to activate the airtime scheduler by fixing lower AQL > limits (e.g. threshold of 5000, limits per AC 0/5000). This way, it seems > that the STAs start competing again for the airtime, and their behaviour > follows the airtime weights. However, slower STAs lose a bit of > performance due to these lower limits. ... and the effect of lowering the AQL parameters is that this situation will occur less often, making the fairness scheduler work better. > - The airtime weights have to be higher (e.g. 10000 vs 20000 to obtain a > 33% vs 66% relation); I found the same behaviour using ath9k and 11n cards, > so I guess this is due to the aggregation of packets. This is a bit more puzzling, but I guess it's related to the above. Also, for ath9k, the airtime check *always* returns true, which just exacerbates the behaviour. > Looking into the code, it seems that the key airtime check is the one > in ieee80211_tx_dequeue. To enable the airtime scheduling, the > "ieee80211_txq_airtime_check" function has to return false more usually; > maybe it is just a matter of adjusting the AQL limits according to the > airtime weights or to modify a bit the "ieee80211_txq_airtime_check" > function to consider the airtime weight or the deficit of the > stations. Yup, you're right on point here, it's a bad interaction between those two throttling mechanisms. The fairness mechanism relies on not reshuffling the RR rotation. I'll think a bit more about what we can do about this. May need to set up a box for testing a few ideas, but I have been meaning to do that anyway. In the meantime, I wonder what happens if we just don't do that? I.e., something like the below (completely untested)? -Toke diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index d2136007e2eb..3a2898cbb111 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -3743,8 +3743,7 @@ 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, *head = NULL; - bool found_eligible_txq = false; + struct txq_info *txqi = NULL; spin_lock_bh(&local->active_txq_lock[ac]); @@ -3755,34 +3754,22 @@ struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, u8 ac) if (!txqi) goto out; - if (txqi == head) { - if (!found_eligible_txq) - goto out; - else - found_eligible_txq = false; - } - - if (!head) - head = txqi; - if (txqi->txq.sta) { struct sta_info *sta = container_of(txqi->txq.sta, struct sta_info, sta); - bool aql_check = ieee80211_txq_airtime_check(hw, &txqi->txq); s64 deficit = sta->airtime[txqi->txq.ac].deficit; - if (aql_check) - found_eligible_txq = true; - - if (deficit < 0) + if (deficit < 0) { sta->airtime[txqi->txq.ac].deficit += sta->airtime_weight; - if (deficit < 0 || !aql_check) { list_move_tail(&txqi->schedule_order, &local->active_txqs[txqi->txq.ac]); goto begin; } + + if (ieee80211_txq_airtime_check(hw, &txqi->txq)) + goto out; }