netdev - Re: iwlwifi warnings in 5.5-rc1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Dec 2019 22:17:30 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Jens Axboe <axboe@...nel.dk>,
        Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
        Luca Coelho <luciano.coelho@...el.com>
Cc:     "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>
Subject: Re: iwlwifi warnings in 5.5-rc1

On Wed, 2019-12-11 at 15:02 +0100, Toke Høiland-Jørgensen 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 frame
> >    enters, and then ... should it "trickle back down" as the generated 
> >    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 ...
> 
> 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 think reporting it on the first packet
> would be the safest if we had to choose. 

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?

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.

> > 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.
> > 
> > 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.
> 
> 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).

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

> > There's an additional wrinkle here - what about HE scheduled mode, where
> > the AP decided when and at what rate you're allowed to send? We don't
> > report that at all, not even as part of rate scaling, since rate scaling
> > only affects *our* decision, not when we send as a response to a trigger
> > frame. This is _still_ relevant for AQL, but there we can only see what
> > the AP used last (**), but we don't track that now nor do we track the
> > proportion of packets that we sent triggered vs. normal medium
> > access...
> 
> Huh, wasn't aware that was a thing in HE; that's cool! And yeah, could
> have interesting interactions with AQL...

Yeah ...

johannes