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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [Make-wifi-fast] [PATCH v2 4/4] mac80211: Use Airtime-based Queue
 Limits (AQL) on packet dequeue
From:   Sebastian Moeller <moeller0@gmx.de>
In-Reply-To: <87o8yfg0zo.fsf@toke.dk>
Date:   Thu, 17 Oct 2019 11:57:36 +0200
Cc:     Kan Yan <kyan@google.com>,
        Rajkumar Manoharan <rmanohar@codeaurora.org>,
        Kevin Hayes <kevinhayes@google.com>,
        make-wifi-fast@lists.bufferbloat.net,
        linux-wireless@vger.kernel.org, ath10k@lists.infradead.org,
        John Crispin <john@phrozen.org>,
        Johannes Berg <johannes@sipsolutions.net>,
        Lorenzo Bianconi <lorenzo@kernel.org>,
        Felix Fietkau <nbd@nbd.name>
Content-Transfer-Encoding: quoted-printable
Message-Id: <751EA059-654B-4E06-A3D6-C727FE1FCE98@gmx.de>
References: <157115993755.2500430.12214017471129215800.stgit@toke.dk>
 <157115994190.2500430.14955682016008497593.stgit@toke.dk>
 <CA+iem5s=xbzZ5goaBO4tZWUKVQRaXb+SnB34NPCppej9mi8sAA@mail.gmail.com>
 <87o8yfg0zo.fsf@toke.dk>
To:     =?utf-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk


> On Oct 17, 2019, at 11:44, Toke H=C3=B8iland-J=C3=B8rgensen =
<toke@redhat.com> wrote:
>=20
> Kan Yan <kyan@google.com> writes:
>=20
>> Hi Toke,
>>=20
>> Thanks for getting this done! I will give it a try in the next few
>> days.  A few comments:
>>=20
>>> The estimated airtime for each skb is stored in the tx_info, so we =
can
>>> subtract the same amount from the running total when the skb is =
freed or
>>> recycled.
>>=20
>> Looks like ath10k driver zero out the info->status before calling
>> ieee80211_tx_status(...):
>> int ath10k_txrx_tx_unref(struct ath10k_htt *htt,
>>                         const struct htt_tx_done *tx_done)
>> {
>> ...
>>        info =3D IEEE80211_SKB_CB(msdu);
>>        memset(&info->status, 0, sizeof(info->status));
>> ...
>>        ieee80211_tx_status(htt->ar->hw, msdu);
>> }
>=20
> Ah, bugger; I was afraid we'd run into this. A quick grep indicates =
that
> it's only ath10k and iwl that do this, though, so it's probably
> manageable to just fix this. I think the simplest solution is just to
> restore the field after clearing, no?
>=20
>> We need either restore the info->status.tx_time_est or calling
>> ieee80211_sta_update_pending_airtime() in ath10k before tx_time_est
>> get erased.
>>=20
>>> +       if (local->airtime_flags & AIRTIME_USE_AQL) {
>>> +               airtime =3D ieee80211_calc_expected_tx_airtime(hw, =
vif, txq->sta,
>>> +                                                            =
skb->len + 38);
>>=20
>> I think it is better to put the "+  38" that takes care of the header
>> overhead inside ieee80211_calc_expected_tx_airtime().
>=20
> Hmm, no strong opinion about this; but yeah, since we have a dedicated
> function for this use I guess there's no harm in adding it there :)
>=20

Silly question, is this Overhead guaranteed to be 38 Bytes for all =
eternity? Otherwise a variable or a preprocessor constant might be more =
future proof?

Best Regards
	Sebastian


> -Toke
>=20
> _______________________________________________
> Make-wifi-fast mailing list
> Make-wifi-fast@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/make-wifi-fast