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;
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org,
        make-wifi-fast@lists.bufferbloat.net, ath10k@lists.infradead.org,
        John Crispin <john@phrozen.org>,
        Lorenzo Bianconi <lorenzo@kernel.org>,
        Felix Fietkau <nbd@nbd.name>, Kan Yan <kyan@google.com>,
        Rajkumar Manoharan <rmanohar@codeaurora.org>,
        Kevin Hayes <kevinhayes@google.com>
Subject: Re: [PATCH v6 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
In-Reply-To: <0b43c4822ab83ea4d33a5a32d8ff6c7a56eff6c5.camel@sipsolutions.net>
References: <157182473951.150713.7978051149956899705.stgit@toke.dk> <157182474399.150713.16380222749144410045.stgit@toke.dk> <0b43c4822ab83ea4d33a5a32d8ff6c7a56eff6c5.camel@sipsolutions.net>
Date:   Fri, 08 Nov 2019 12:01:32 +0100
Message-ID: <877e4afx83.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Johannes Berg <johannes@sipsolutions.net> writes:

> On Wed, 2019-10-23 at 11:59 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote=
:
>>=20
>> +=09if (info->tx_time_est) {
>> +=09=09struct sta_info *sta =3D NULL, *s;
>> +=09=09struct rhlist_head *tmp;
>> +
>> +=09=09rcu_read_lock();
>> +
>> +=09=09for_each_sta_info(local, hdr->addr1, s, tmp) {
>> +=09=09=09/* skip wrong virtual interface */
>> +=09=09=09if (!ether_addr_equal(hdr->addr2, s->sdata->vif.addr))
>> +=09=09=09=09continue;
>> +
>> +=09=09=09sta =3D s;
>> +=09=09=09break;
>> +=09=09}
>
> I guess that is better than looking up the sdata and then using
> sta_info_get(), but I think I'd like to see this wrapped into a function
> (even if it's an inline) in sta_info.{c,h}.

OK, can do.

>> +=09=09airtime =3D ieee80211_calc_expected_tx_airtime(hw, vif, txq->sta,
>> +=09=09=09=09=09=09=09     skb->len);
>> +=09=09if (airtime) {
>> +=09=09=09/* We only have 10 bits in tx_time_est, so store airtime
>> +=09=09=09 * in increments of 4us and clamp the maximum to 2**12-1
>> +=09=09=09 */
>> +=09=09=09airtime =3D min_t(u32, airtime, 4095) & ~3U;
>> +=09=09=09info->tx_time_est =3D airtime >> 2;
>> +=09=09=09ieee80211_sta_update_pending_airtime(local, tx.sta,
>> +=09=09=09=09=09=09=09     txq->ac, airtime,
>> +=09=09=09=09=09=09=09     false);
>
> I wonder if it'd be better to pass the shifted value to
> ieee80211_sta_update_pending_airtime() to avoid all the shifting in all
> places?
>
> You could even store the shifted value in "aql_tx_pending" and
> "aql_total_pending_airtime" etc., it's completely equivalent, and only
> shift it out for people looking at debugfs.

My reasoning for doing it this way was that we have another set of APIs
dealing with airtime which doesn't do any shifting; so keeping the APIs
in the same unit (straight airtime) seemed less confusing.

We could add (inline) setter and getter functions for the tx_time_est
field instead to avoid sprinkling shifts all over the place? :)

-Toke