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:     Sebastian Moeller <moeller0@gmx.de>
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>
Subject: Re: [Make-wifi-fast] [PATCH v2 4/4] mac80211: Use Airtime-based Queue Limits (AQL) on packet dequeue
In-Reply-To: <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> <751EA059-654B-4E06-A3D6-C727FE1FCE98@gmx.de>
Date:   Thu, 17 Oct 2019 12:24:26 +0200
Message-ID: <87lftjfz51.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

Sebastian Moeller <moeller0@gmx.de> writes:

>> 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?

Well, yeah, as long as we're sending Ethernet packets. Which is kinda
baked into the WiFi standard :)

-Toke