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: <87lftjfz51.fsf@toke.dk>
Date:   Thu, 17 Oct 2019 12:25:28 +0200
Cc:     Kan Yan <kyan@google.com>,
        Rajkumar Manoharan <rmanohar@codeaurora.org>,
        Kevin Hayes <kevinhayes@google.com>,
        Make-Wifi-fast <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: <18FC6F1D-084C-44BD-87E7-C9F394D6FCD1@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>
 <87lftjfz51.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

What about VLAN tags?

Best Regards
	Sebastian

> On Oct 17, 2019, at 12:24, Toke H=C3=B8iland-J=C3=B8rgensen =
<toke@redhat.com> wrote:
>=20
> Sebastian Moeller <moeller0@gmx.de> writes:
>=20
>>> 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
>>=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?
>=20
> Well, yeah, as long as we're sending Ethernet packets. Which is kinda
> baked into the WiFi standard :)
>=20
> -Toke