netdev - Re: [RFC PATCH 1/1] mac80211: use AQL airtime for expected throughput.

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, 25 May 2022 14:50:48 +0200
From:   Felix Fietkau <nbd@....name>
To:     Baligh GASMI <gasmibal@...il.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Johannes Berg <johannes@...solutions.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "open list:MAC80211" <linux-wireless@...r.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/1] mac80211: use AQL airtime for expected
 throughput.


On 25.05.22 14:14, Baligh GASMI wrote:
>> > Since the integration of AQL, packet TX airtime estimation is
>> > calculated and counted to be used for the dequeue limit.
>> >
>> > Use this estimated airtime to compute expected throughput for
>> > each station.
>> >
>> > It will be a generic mac80211 implementation. If the driver has
>> > get_expected_throughput implementation, it will be used instead.
>> >
>> > Useful for L2 routing protocols, like B.A.T.M.A.N.
>> >
>> > Signed-off-by: Baligh Gasmi <gasmibal@...il.com>
>> > ---
>> >  net/mac80211/driver-ops.h |  2 ++
>> >  net/mac80211/sta_info.h   |  2 ++
>> >  net/mac80211/status.c     | 22 ++++++++++++++++++++++
>> >  net/mac80211/tx.c         |  3 ++-
>> >  4 files changed, 28 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
>> > index 4e2fc1a08681..4331b79647fa 100644
>> > --- a/net/mac80211/driver-ops.h
>> > +++ b/net/mac80211/driver-ops.h
>> > @@ -1142,6 +1142,8 @@ static inline u32 drv_get_expected_throughput(struct ieee80211_local *local,
>> >       trace_drv_get_expected_throughput(&sta->sta);
>> >       if (local->ops->get_expected_throughput && sta->uploaded)
>> >               ret = local->ops->get_expected_throughput(&local->hw, &sta->sta);
>> > +     else
>> > +             ret = ewma_avg_est_tp_read(&sta->status_stats.avg_est_tp);
>> >       trace_drv_return_u32(local, ret);
>> >
>> >       return ret;
>> > diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
>> > index 379fd367197f..fe60be4c671d 100644
>> > --- a/net/mac80211/sta_info.h
>> > +++ b/net/mac80211/sta_info.h
>> > @@ -123,6 +123,7 @@ enum ieee80211_sta_info_flags {
>> >  #define HT_AGG_STATE_STOP_CB         7
>> >  #define HT_AGG_STATE_SENT_ADDBA              8
>> >
>> > +DECLARE_EWMA(avg_est_tp, 8, 16)
>> >  DECLARE_EWMA(avg_signal, 10, 8)
>> >  enum ieee80211_agg_stop_reason {
>> >       AGG_STOP_DECLINED,
>> > @@ -641,6 +642,7 @@ struct sta_info {
>> >               s8 last_ack_signal;
>> >               bool ack_signal_filled;
>> >               struct ewma_avg_signal avg_ack_signal;
>> > +             struct ewma_avg_est_tp avg_est_tp;
>> >       } status_stats;
>> >
>> >       /* Updated from TX path only, no locking requirements */
>> > diff --git a/net/mac80211/status.c b/net/mac80211/status.c
>> > index e81e8a5bb774..647ade3719f5 100644
>> > --- a/net/mac80211/status.c
>> > +++ b/net/mac80211/status.c
>> > @@ -1145,6 +1145,28 @@ void ieee80211_tx_status_ext(struct ieee80211_hw *hw,
>> >                       sta->status_stats.retry_failed++;
>> >               sta->status_stats.retry_count += retry_count;
>> >
>> > +             if (skb && tx_time_est) {
>>
>> Shouldn't this be conditioned on actually being used (i.e., existence of
>> get_expected_throughput op? Also maybe pull it out into its own function
>> to make it clear what it's doing...
> 
> It's already the case I think, since the tx_time_est is not-zero only
> when actually an estimated time is set.
> A dedicated function seems good for me, for clarity, yes.
> 
>>
>> > +                     /* max average packet size */
>> > +                     size_t pkt_size = skb->len > 1024 ? 1024 : skb->len;
>> > +
>> > +                     if (acked) {
>> > +                             /* ACK packet size */
>> > +                             pkt_size += 14;
>> > +                             /* SIFS x 2 */
>> > +                             tx_time_est += 2 * 2;
>> > +                     }
>> > +
>> > +                     /* Backoff average x retries */
>> > +                     tx_time_est += retry_count ? retry_count * 2 : 2;
>> > +
>> > +                     /* failed tx */
>> > +                     if (!acked && !noack_success)
>> > +                             pkt_size = 0;
>> > +
>> > +                     ewma_avg_est_tp_add(&sta->status_stats.avg_est_tp,
>> > +                                         ((pkt_size * 8) * 1000) / tx_time_est);
>>
>> Could we avoid adding this division in the fast path?
> 
> Maybe we can use the do_div() macro for optimization, I don't see how
> we can avoid it.
Here's one idea on how to make it less expensive and improve the average 
of the throughput estimation at the same time:

In sta->status_stats add these fields:

u32 last_tp_update;
u32 pkt_size;
u32 tx_time_est;

And in this part of the code you do something like this:

u32 diff = ((u32)jiffies) - sta->status_stats.last_tp_update;
sta->status_stats.pkt_size += (pkt_size * 8) * 1000;
sta->status_stats.tx_time_est += tx_time_est;
if (diff > HZ / 10) {
	ewma_avg_est_tp_add(&sta->status_stats.avg_est_tp,
			    sta->status_stats.pkt_size /
			    sta->status_stats.tx_time_est);
	sta->status_stats.pkt_size = 0;
	sta->status_stats.tx_time_est = 0;
}

This ensures that you will only get max. 1 update per 100ms interval.

Does that make sense?

- Felix