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;
MIME-Version: 1.0
References: <20191112021136.42918-1-kyan@google.com> <20191112021136.42918-2-kyan@google.com>
 <871rudqpsg.fsf@toke.dk>
In-Reply-To: <871rudqpsg.fsf@toke.dk>
From:   Kan Yan <kyan@google.com>
Date:   Tue, 12 Nov 2019 18:27:02 -0800
Message-ID: <CA+iem5vrM0iF+yvS9m==UWnhp=DFV924ir-0nHcn6cwarEjZNA@mail.gmail.com>
Subject: Re: [PATCH v7 1/2] mac80211: Implement Airtime-based Queue Limit (AQL)
To:     =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= <toke@redhat.com>
Cc:     Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org,
        Make-Wifi-fast <make-wifi-fast@lists.bufferbloat.net>,
        Felix Fietkau <nbd@nbd.name>, Yibo Zhao <yiboz@codeaurora.org>,
        John Crispin <john@phrozen.org>,
        Lorenzo Bianconi <lorenzo@kernel.org>,
        Rajkumar Manoharan <rmanohar@codeaurora.org>,
        Kevin Hayes <kevinhayes@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Thanks for the review. I will pick up your new patches and give it a
try tomorrow.

> Why is this setting sta and device limits to the same value?

local->aql_txq_limit_low is not the per device limit, but the default
txq_limit for all STAs. Individual stations can be configured with
non-default value via debugfs entry
"netdev:interface_name_x/stations/mac_addr_x/airtime". "aql_threshold"
is the device limit for switching between the lower and higher per
station queue limit.

> Also, are you sure we won't risk write tearing when writing 32-bit
> values without locking on some architectures?

Does mac80211 ever runs in any 16-bit architectures? Even in an
architecture that write to 32-bit value is not atomic, I don't think
there is any side-effect for queue limit get wrong transiently in rare
occasions. Besides, the practical value of those queue limits should
always fit into 16 bits.

> I don't think this is right; another thread could do atomic_inc()
> between the atomic_read() and atomic_set() here, in which case this
> would clobber the other value.
> I think to get this right the logic would need to be something like
> this:
> retry:
>   old =3D atomic_read(&sta->airtime[ac].aql_tx_pending);
>   if (warn_once(tx_airtime > old))
>      new =3D 0;
>   else
>      new =3D old - tx_airtime;
>   if (atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending, old, new) !=3D old=
)
>      goto retry;
> (or use an equivalent do/while).

That's indeed not right. However, if a potential aql_tx_pending
underflow case is detected here (It should never happen), reset it to
0 maybe not the best remedy anyway. I think it is better  just
WARN_ONCE() and skip updating aql_tx_pending all together, so the
retry or loop can be avoided here. What do you think?


On Tue, Nov 12, 2019 at 3:48 AM Toke H=C3=B8iland-J=C3=B8rgensen <toke@redh=
at.com> wrote:
>
> Kan Yan <kyan@google.com> writes:
>
> > In order for the Fq_CoDel algorithm integrated in mac80211 layer to ope=
rate
> > effectively to control excessive queueing latency, the CoDel algorithm
> > requires an accurate measure of how long packets stays in the queue, AK=
A
> > sojourn time. The sojourn time measured at the mac80211 layer doesn't
> > include queueing latency in the lower layer (firmware/hardware) and CoD=
el
> > expects lower layer to have a short queue. However, most 802.11ac chips=
ets
> > offload tasks such TX aggregation to firmware or hardware, thus have a =
deep
> > lower layer queue.
> >
> > Without a mechanism to control the lower layer queue size, packets only
> > stay in mac80211 layer transiently before being sent to firmware queue.
> > As a result, the sojourn time measured by CoDel in the mac80211 layer i=
s
> > almost always lower than the CoDel latency target, hence CoDel does lit=
tle
> > to control the latency, even when the lower layer queue causes excessiv=
e
> > latency.
> >
> > The Byte Queue Limits (BQL) mechanism is commonly used to address the
> > similar issue with wired network interface. However, this method cannot=
 be
> > applied directly to the wireless network interface. "Bytes" is not a
> > suitable measure of queue depth in the wireless network, as the data ra=
te
> > can vary dramatically from station to station in the same network, from=
 a
> > few Mbps to over Gbps.
> >
> > This patch implements an Airtime-based Queue Limit (AQL) to make CoDel =
work
> > effectively with wireless drivers that utilized firmware/hardware
> > offloading. AQL allows each txq to release just enough packets to the l=
ower
> > layer to form 1-2 large aggregations to keep hardware fully utilized an=
d
> > retains the rest of the frames in mac80211 layer to be controlled by th=
e
> > CoDel algorithm.
> >
> > Signed-off-by: Kan Yan <kyan@google.com>
> > [ Toke: Keep API to set pending airtime internal, fix nits in commit ms=
g ]
> > Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen <toke@redhat.com>
>
> Thanks for the update! A few comments below. I'll also send you and
> updated version of my patch following this one, with fixes for Johannes'
> comments. You can include that in your next version...
>
> > ---
> >  include/net/cfg80211.h     |  7 ++++
> >  include/net/mac80211.h     | 12 ++++++
> >  net/mac80211/debugfs.c     | 78 ++++++++++++++++++++++++++++++++++++++
> >  net/mac80211/debugfs_sta.c | 43 ++++++++++++++++-----
> >  net/mac80211/ieee80211_i.h |  4 ++
> >  net/mac80211/main.c        | 10 ++++-
> >  net/mac80211/sta_info.c    | 40 +++++++++++++++++++
> >  net/mac80211/sta_info.h    |  8 ++++
> >  net/mac80211/tx.c          | 47 +++++++++++++++++++++--
> >  9 files changed, 235 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> > index 4ab2c49423dc..15f9f04de149 100644
> > --- a/include/net/cfg80211.h
> > +++ b/include/net/cfg80211.h
> > @@ -2602,6 +2602,13 @@ enum wiphy_params_flags {
> >
> >  #define IEEE80211_DEFAULT_AIRTIME_WEIGHT     256
> >
> > +/* The per TXQ device queue limit in airtime */
> > +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_L    4000
> > +#define IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H    8000
> > +
> > +/* The per interface airtime threshold to switch to lower queue limit =
*/
> > +#define IEEE80211_AQL_THRESHOLD                      24000
> > +
> >  /**
> >   * struct cfg80211_pmksa - PMK Security Association
> >   *
> > diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> > index 4319596ef472..269be699e89c 100644
> > --- a/include/net/mac80211.h
> > +++ b/include/net/mac80211.h
> > @@ -5558,6 +5558,18 @@ void ieee80211_send_eosp_nullfunc(struct ieee802=
11_sta *pubsta, int tid);
> >  void ieee80211_sta_register_airtime(struct ieee80211_sta *pubsta, u8 t=
id,
> >                                   u32 tx_airtime, u32 rx_airtime);
> >
> > +/**
> > + * ieee80211_txq_airtime_check - check if a txq can send frame to devi=
ce
> > + *
> > + * @hw: pointer obtained from ieee80211_alloc_hw()
> > + * @txq: pointer obtained from station or virtual interface
> > + *
> > + * Return true if the AQL's airtime limit has not been reached and the=
 txq can
> > + * continue to send more packets to the device. Otherwise return false=
.
> > + */
> > +bool
> > +ieee80211_txq_airtime_check(struct ieee80211_hw *hw, struct ieee80211_=
txq *txq);
> > +
> >  /**
> >   * ieee80211_iter_keys - iterate keys programmed into the device
> >   * @hw: pointer obtained from ieee80211_alloc_hw()
> > diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
> > index 568b3b276931..d77ea0e51c1d 100644
> > --- a/net/mac80211/debugfs.c
> > +++ b/net/mac80211/debugfs.c
> > @@ -148,6 +148,80 @@ static const struct file_operations aqm_ops =3D {
> >       .llseek =3D default_llseek,
> >  };
> >
> > +static ssize_t aql_txq_limit_read(struct file *file,
> > +                               char __user *user_buf,
> > +                               size_t count,
> > +                               loff_t *ppos)
> > +{
> > +     struct ieee80211_local *local =3D file->private_data;
> > +     char buf[400];
> > +     int len =3D 0;
> > +
> > +     len =3D scnprintf(buf, sizeof(buf),
> > +                     "AC     AQL limit low   AQL limit high\n"
> > +                     "VO     %u              %u\n"
> > +                     "VI     %u              %u\n"
> > +                     "BE     %u              %u\n"
> > +                     "BK     %u              %u\n",
> > +                     local->aql_txq_limit_low[IEEE80211_AC_VO],
> > +                     local->aql_txq_limit_high[IEEE80211_AC_VO],
> > +                     local->aql_txq_limit_low[IEEE80211_AC_VI],
> > +                     local->aql_txq_limit_high[IEEE80211_AC_VI],
> > +                     local->aql_txq_limit_low[IEEE80211_AC_BE],
> > +                     local->aql_txq_limit_high[IEEE80211_AC_BE],
> > +                     local->aql_txq_limit_low[IEEE80211_AC_BK],
> > +                     local->aql_txq_limit_high[IEEE80211_AC_BK]);
> > +     return simple_read_from_buffer(user_buf, count, ppos,
> > +                                    buf, len);
> > +}
> > +
> > +static ssize_t aql_txq_limit_write(struct file *file,
> > +                                const char __user *user_buf,
> > +                                size_t count,
> > +                                loff_t *ppos)
> > +{
> > +     struct ieee80211_local *local =3D file->private_data;
> > +     char buf[100];
> > +     size_t len;
> > +     u32 ac, q_limit_low, q_limit_high;
> > +     struct sta_info *sta;
> > +
> > +     if (count > sizeof(buf))
> > +             return -EINVAL;
> > +
> > +     if (copy_from_user(buf, user_buf, count))
> > +             return -EFAULT;
> > +
> > +     buf[sizeof(buf) - 1] =3D 0;
> > +     len =3D strlen(buf);
> > +     if (len > 0 && buf[len - 1] =3D=3D '\n')
> > +             buf[len - 1] =3D 0;
> > +
> > +     if (sscanf(buf, "%u %u %u", &ac, &q_limit_low, &q_limit_high) !=
=3D 3)
> > +             return -EINVAL;
> > +
> > +     if (ac >=3D IEEE80211_NUM_ACS)
> > +             return -EINVAL;
> > +
> > +     local->aql_txq_limit_low[ac] =3D q_limit_low;
> > +     local->aql_txq_limit_high[ac] =3D q_limit_high;
> > +
> > +     mutex_lock(&local->sta_mtx);
> > +     list_for_each_entry(sta, &local->sta_list, list) {
> > +             sta->airtime[ac].aql_limit_low =3D q_limit_low;
> > +             sta->airtime[ac].aql_limit_high =3D q_limit_high;
> > +     }
>
> Why is this setting sta and device limits to the same value?
>
> Also, are you sure we won't risk write tearing when writing 32-bit
> values without locking on some architectures?
>
> > +     mutex_unlock(&local->sta_mtx);
> > +     return count;
> > +}
> > +
> > +static const struct file_operations aql_txq_limit_ops =3D {
> > +     .write =3D aql_txq_limit_write,
> > +     .read =3D aql_txq_limit_read,
> > +     .open =3D simple_open,
> > +     .llseek =3D default_llseek,
> > +};
> > +
> >  static ssize_t force_tx_status_read(struct file *file,
> >                                   char __user *user_buf,
> >                                   size_t count,
> > @@ -441,6 +515,10 @@ void debugfs_hw_add(struct ieee80211_local *local)
> >       debugfs_create_u16("airtime_flags", 0600,
> >                          phyd, &local->airtime_flags);
> >
> > +     DEBUGFS_ADD(aql_txq_limit);
> > +     debugfs_create_u32("aql_threshold", 0600,
> > +                        phyd, &local->aql_threshold);
> > +
> >       statsd =3D debugfs_create_dir("statistics", phyd);
> >
> >       /* if the dir failed, don't put all the other things into the roo=
t! */
> > diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
> > index c8ad20c28c43..0185e6e5e5d1 100644
> > --- a/net/mac80211/debugfs_sta.c
> > +++ b/net/mac80211/debugfs_sta.c
> > @@ -197,10 +197,12 @@ static ssize_t sta_airtime_read(struct file *file=
, char __user *userbuf,
> >  {
> >       struct sta_info *sta =3D file->private_data;
> >       struct ieee80211_local *local =3D sta->sdata->local;
> > -     size_t bufsz =3D 200;
> > +     size_t bufsz =3D 400;
> >       char *buf =3D kzalloc(bufsz, GFP_KERNEL), *p =3D buf;
> >       u64 rx_airtime =3D 0, tx_airtime =3D 0;
> >       s64 deficit[IEEE80211_NUM_ACS];
> > +     u32 q_depth[IEEE80211_NUM_ACS];
> > +     u32 q_limit_l[IEEE80211_NUM_ACS], q_limit_h[IEEE80211_NUM_ACS];
> >       ssize_t rv;
> >       int ac;
> >
> > @@ -212,19 +214,22 @@ static ssize_t sta_airtime_read(struct file *file=
, char __user *userbuf,
> >               rx_airtime +=3D sta->airtime[ac].rx_airtime;
> >               tx_airtime +=3D sta->airtime[ac].tx_airtime;
> >               deficit[ac] =3D sta->airtime[ac].deficit;
> > +             q_limit_l[ac] =3D sta->airtime[ac].aql_limit_low;
> > +             q_limit_h[ac] =3D sta->airtime[ac].aql_limit_high;
> >               spin_unlock_bh(&local->active_txq_lock[ac]);
> > +             q_depth[ac] =3D atomic_read(&sta->airtime[ac].aql_tx_pend=
ing);
> >       }
> >
> >       p +=3D scnprintf(p, bufsz + buf - p,
> >               "RX: %llu us\nTX: %llu us\nWeight: %u\n"
> > -             "Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us=
\n",
> > -             rx_airtime,
> > -             tx_airtime,
> > -             sta->airtime_weight,
> > -             deficit[0],
> > -             deficit[1],
> > -             deficit[2],
> > -             deficit[3]);
> > +             "Deficit: VO: %lld us VI: %lld us BE: %lld us BK: %lld us=
\n"
> > +             "Q depth: VO: %u us VI: %u us BE: %u us BK: %u us\n"
> > +             "Q limit[low/high]: VO: %u/%u VI: %u/%u BE: %u/%u BK: %u/=
%u\n",
> > +             rx_airtime, tx_airtime, sta->airtime_weight,
> > +             deficit[0], deficit[1], deficit[2], deficit[3],
> > +             q_depth[0], q_depth[1], q_depth[2], q_depth[3],
> > +             q_limit_l[0], q_limit_h[0], q_limit_l[1], q_limit_h[1],
> > +             q_limit_l[2], q_limit_h[2], q_limit_l[3], q_limit_h[3]),
> >
> >       rv =3D simple_read_from_buffer(userbuf, count, ppos, buf, p - buf=
);
> >       kfree(buf);
> > @@ -236,7 +241,25 @@ static ssize_t sta_airtime_write(struct file *file=
, const char __user *userbuf,
> >  {
> >       struct sta_info *sta =3D file->private_data;
> >       struct ieee80211_local *local =3D sta->sdata->local;
> > -     int ac;
> > +     u32 ac, q_limit_l, q_limit_h;
> > +     char _buf[100] =3D {}, *buf =3D _buf;
> > +
> > +     if (count > sizeof(_buf))
> > +             return -EINVAL;
> > +
> > +     if (copy_from_user(buf, userbuf, count))
> > +             return -EFAULT;
> > +
> > +     buf[sizeof(_buf) - 1] =3D '\0';
> > +     if (sscanf(buf, "queue limit %u %u %u", &ac, &q_limit_l, &q_limit=
_h)
> > +         !=3D 3)
> > +             return -EINVAL;
> > +
> > +     if (ac >=3D IEEE80211_NUM_ACS)
> > +             return -EINVAL;
> > +
> > +     sta->airtime[ac].aql_limit_low =3D q_limit_l;
> > +     sta->airtime[ac].aql_limit_high =3D q_limit_h;
>
> Same concern as above re: unprotected writes.
>
> >       for (ac =3D 0; ac < IEEE80211_NUM_ACS; ac++) {
> >               spin_lock_bh(&local->active_txq_lock[ac]);
> > diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> > index 225ea4e3cd76..ad15b3be8bb3 100644
> > --- a/net/mac80211/ieee80211_i.h
> > +++ b/net/mac80211/ieee80211_i.h
> > @@ -1142,6 +1142,10 @@ struct ieee80211_local {
> >       u16 schedule_round[IEEE80211_NUM_ACS];
> >
> >       u16 airtime_flags;
> > +     u32 aql_txq_limit_low[IEEE80211_NUM_ACS];
> > +     u32 aql_txq_limit_high[IEEE80211_NUM_ACS];
> > +     u32 aql_threshold;
> > +     atomic_t aql_total_pending_airtime;
> >
> >       const struct ieee80211_ops *ops;
> >
> > diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> > index aba094b4ccfc..071ea92a3748 100644
> > --- a/net/mac80211/main.c
> > +++ b/net/mac80211/main.c
> > @@ -667,8 +667,16 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t =
priv_data_len,
> >       for (i =3D 0; i < IEEE80211_NUM_ACS; i++) {
> >               INIT_LIST_HEAD(&local->active_txqs[i]);
> >               spin_lock_init(&local->active_txq_lock[i]);
> > +             local->aql_txq_limit_low[i] =3D IEEE80211_DEFAULT_AQL_TXQ=
_LIMIT_L;
> > +             local->aql_txq_limit_high[i] =3D
> > +                     IEEE80211_DEFAULT_AQL_TXQ_LIMIT_H;
> >       }
> > -     local->airtime_flags =3D AIRTIME_USE_TX | AIRTIME_USE_RX;
> > +
> > +     local->airtime_flags =3D AIRTIME_USE_TX |
> > +                            AIRTIME_USE_RX |
> > +                            AIRTIME_USE_AQL;
> > +     local->aql_threshold =3D IEEE80211_AQL_THRESHOLD;
> > +     atomic_set(&local->aql_total_pending_airtime, 0);
> >
> >       INIT_LIST_HEAD(&local->chanctx_list);
> >       mutex_init(&local->chanctx_mtx);
> > diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> > index bd11fef2139f..a76e935a543a 100644
> > --- a/net/mac80211/sta_info.c
> > +++ b/net/mac80211/sta_info.c
> > @@ -396,6 +396,9 @@ struct sta_info *sta_info_alloc(struct ieee80211_su=
b_if_data *sdata,
> >               skb_queue_head_init(&sta->ps_tx_buf[i]);
> >               skb_queue_head_init(&sta->tx_filtered[i]);
> >               sta->airtime[i].deficit =3D sta->airtime_weight;
> > +             atomic_set(&sta->airtime[i].aql_tx_pending, 0);
> > +             sta->airtime[i].aql_limit_low =3D local->aql_txq_limit_lo=
w[i];
> > +             sta->airtime[i].aql_limit_high =3D local->aql_txq_limit_h=
igh[i];
> >       }
> >
> >       for (i =3D 0; i < IEEE80211_NUM_TIDS; i++)
> > @@ -1893,6 +1896,43 @@ void ieee80211_sta_register_airtime(struct ieee8=
0211_sta *pubsta, u8 tid,
> >  }
> >  EXPORT_SYMBOL(ieee80211_sta_register_airtime);
> >
> > +void ieee80211_sta_update_pending_airtime(struct ieee80211_local *loca=
l,
> > +                                       struct sta_info *sta, u8 ac,
> > +                                       u16 tx_airtime, bool tx_complet=
ed)
> > +{
> > +     if (!tx_completed) {
> > +             if (sta)
> > +                     atomic_add(tx_airtime,
> > +                                &sta->airtime[ac].aql_tx_pending);
> > +
> > +             atomic_add(tx_airtime, &local->aql_total_pending_airtime)=
;
> > +             return;
> > +     }
> > +
> > +     if (sta) {
> > +             if (WARN_ONCE(atomic_read(&sta->airtime[ac].aql_tx_pendin=
g) <
> > +                           tx_airtime,
> > +                           "STA %pM AC %d txq pending airtime underflo=
w: %u, %u",
> > +                           sta->addr, ac,
> > +                           atomic_read(&sta->airtime[ac].aql_tx_pendin=
g),
> > +                           tx_airtime))
> > +                     atomic_set(&sta->airtime[ac].aql_tx_pending, 0);
>
> I don't think this is right; another thread could do atomic_inc()
> between the atomic_read() and atomic_set() here, in which case this
> would clobber the other value.
>
> I think to get this right the logic would need to be something like
> this:
>
> retry:
>   old =3D atomic_read(&sta->airtime[ac].aql_tx_pending);
>   if (warn_once(tx_airtime > old))
>      new =3D 0;
>   else
>      new =3D old - tx_airtime;
>
>   if (atomic_cmpxchg(&sta->airtime[ac].aql_tx_pending, old, new) !=3D old=
)
>      goto retry;
>
>
> (or use an equivalent do/while).
>
>
> -Toke
>