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>,
        Jens Axboe <axboe@kernel.dk>,
        Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
        Luca Coelho <luciano.coelho@intel.com>
Cc:     "linux-wireless\@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        Networking <netdev@vger.kernel.org>
Subject: Re: iwlwifi warnings in 5.5-rc1
In-Reply-To: <3ca2be96898e9d30c27b2411148d201318e413f2.camel@sipsolutions.net>
References: <ceb74ea2-6a1b-4cef-8749-db21a2ee4311@kernel.dk> <9727368004ceef03f72d259b0779c2cf401432e1.camel@sipsolutions.net> <878snjgs5l.fsf@toke.dk> <3420d73e667b01ec64bf0cc9da6232b41e862860.camel@sipsolutions.net> <875zingnzt.fsf@toke.dk> <bfab4987668990ea8d86a98f3e87c3fa31403745.camel@sipsolutions.net> <87tv67ez9p.fsf@toke.dk> <3ca2be96898e9d30c27b2411148d201318e413f2.camel@sipsolutions.net>
Date:   Thu, 12 Dec 2019 11:55:19 +0100
Message-ID: <87v9qleru0.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-12-11 at 15:02 +0100, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>
>> > 2) GSO/TSO like what we have - it's not really clear how to handle it.
>> >    The airtime estimate will shoot *way* up (64kB frame) once that fra=
me
>> >    enters, and then ... should it "trickle back down" as the generated=
=20
>> >    parts are transmitted? But then the driver needs to split up the
>> >    airtime estimate? Or should it just go back down entirely? On the
>> >    first frame? That might overshoot. On the last frame? Opposite
>> >    problem ...
>>=20
>> Well, ideally it would be divided out over the component packets; but
>> yeah, who is going to do that?
>
> I'm not even sure we *can* do this easily - do we know up-front how many
> packets this will expand to? We should know, but it might not be so easy
> given the abstraction layers. We could guess and if it's wrong just set
> it to 0 on any remaining ones.

I was thinking about a scheme where we re-defined the value in the cb to
be a "time per byte" value, that we could just multiply by the packet
length; that would make it trivial to do partial reporting. Not sure
it's quite workable in practice, though; it would be hard to avoid
rounding errors, and there's also the additional headers when splitting
a packet, so the lengths don't necessarily add up.

>> I think reporting it on the first packet
>> would be the safest if we had to choose.=20
>
> Agree.
>
>> Also, ideally we would want the GSO/TSO mechanism to lower the size
>> of the superpackets at lower rates (does it?). At higher rates this
>> matters less...
>
> Well TCP does limit (pacing shift) the amount of outstanding data, so if
> it's _really_ slow I guess it will also limit the size of the
> superpackets?

Yeah, I *think* it does... :)

> It's really just an artifact of our software implementation that we
> report the SKBs back as used with partial content. Maybe we shouldn't
> even do that, since they weren't generated by mac80211 in the first
> place, and only report the original skb or something.

Hmm, yeah, was wondering how that works, actually. I assumed you send
the whole thing to the hardware as one superpacket? But if so how do you
get the completion events back? Or are you splitting it in the driver
just before you send it to the hardware?

>> > 3) I'm not quite convinced that all drivers report the TX rate
>> >    completely correctly in the status, some don't even use this path
>> >    but the ieee80211_tx_status_ext() which doesn't update the rate.
>> >=20
>> > 4) Probably most importantly, this is completely broken with HE because
>> >    there's currently no way to report HE rates in the TX status at all!
>> >    I just worked around that in our driver for 'iw' reporting purposes
>> >    by implementing the rate reporting in the sta_statistics callback,
>> >    but that data won't be used by the airtime estimates.
>>=20
>> Hmm, yeah, both of those are good points. I guess I just kinda assumed
>> that the drivers were already doing the right thing there... :)
>
> I'm not really sure I want to rely on this - this was never really
> needed *functionally*, just from a *statistics* point of view (e.g. "iw
> link" or such).

Right, I see. Well I guess now that we're turning this on one driver at
a time, we can ensure that the driver provides sufficiently accurate
rate information as part of that.

BTW, since we're discussing this in the context of iwlwifi: do you have
any data as to how much benefit AQL would be for that? I.e., do the
Intel devices tend to buffer a lot of data in hardware/firmware?

>> > Now, (1) probably doesn't matter, the estimates don't need to be that
>> > accurate. (2) I'm not sure how to solve; (3) and (4) could both be
>> > solved by having some mechanism of the rate scaling to tell us what the
>> > current rate is whenever it updates, rather than relying on the
>> > last_rate. Really we should do that much more, and even phase out
>> > last_rate entirely, it's a stupid concept.
>>=20
>> Yes, that last bit would be good!
>
> We already partially have this, we have a 'get best rate' or so callback
> in the rate scaling, we'd just have to extend it to the driver ops for
> offloaded rate scaling.
>
> Ideally, it'd be a function call from the rate scaling to mac80211 so we
> don't have to call a function every time we need the value, but the rate
> scaling just calls us whenever it updates. This would even work with
> iwlwifi's offloaded algorithm - it notifies the host on all changes.

Yup, this makes sense, and would be easy to integrate with Minstrel as
well, I think. We already have ieee80211_sta_set_expected_throughput(),
so maybe expanding that? It just provides a single number now, but we
could change it to set the full rate info instead?

-Toke