Re: [PATCH v2 1/4] mac80211: Rearrange ieee80211_tx_info to make room for tx_time_est

[Kan Yan <kyan@google.com>]

The "tx_time_est" field, shared by control and status, is not able to
survive until the skb returns to the mac80211 layer in some
architectures. The same space is defined as driver_data and some
wireless drivers use it for other purposes, as the cb in the sk_buff
is free to be used by any layer.

In the case of ath10k, the tx_time_est get clobbered by
struct ath10k_skb_cb {
        dma_addr_t paddr;
        u8 flags;
        u8 eid;
        u16 msdu_id;
        u16 airtime_est;
        struct ieee80211_vif *vif;
        struct ieee80211_txq *txq;
} __packed;

Do you think shrink driver_data by 2 bytes and use that space for
tx_time_est to make it persistent across mac80211 and wireless driver
layer an acceptable solution?

Kan




On Tue, Oct 15, 2019 at 10:19 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> To implement airtime queue limiting, we need to keep a running account of
> the estimated airtime of all skbs queued into the device. Do to this
> correctly, we need to store the airtime estimate into the skb so we can
> decrease the outstanding balance when the skb is freed. This means that the
> time estimate must be stored somewhere that will survive for the lifetime
> of the skb.
>
> Fortunately, we had a couple of bytes left in the 'status' field in the
> ieee80211_tx_info; and since we only plan to calculate the airtime estimate
> after the skb is dequeued from the FQ structure, on the control side we can
> share the space with the codel enqueue time. And by rearranging the order
> of elements it is possible to have the position of the new tx_time_est line
> up between the control and status structs, so the value will survive from
> when mac80211 hands the packet to the driver, and until the driver either
> frees it, or hands it back through TX status.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  include/net/mac80211.h |   26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index d69081c38788..49f8ea0af5f8 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -975,20 +975,23 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate)
>   * @control.short_preamble: use short preamble (CCK only)
>   * @control.skip_table: skip externally configured rate table
>   * @control.jiffies: timestamp for expiry on powersave clients
> + * @control.enqueue_time: enqueue time (for iTXQs)
> + * @control.tx_time_est: estimated airtime usage (shared with @status)
> + * @control.reserved: unused field to ensure alignment of data structure
> + * @control.flags: control flags, see &enum mac80211_tx_control_flags
>   * @control.vif: virtual interface (may be NULL)
>   * @control.hw_key: key to encrypt with (may be NULL)
> - * @control.flags: control flags, see &enum mac80211_tx_control_flags
> - * @control.enqueue_time: enqueue time (for iTXQs)
>   * @driver_rates: alias to @control.rates to reserve space
>   * @pad: padding
>   * @rate_driver_data: driver use area if driver needs @control.rates
>   * @status: union part for status data
>   * @status.rates: attempted rates
>   * @status.ack_signal: ACK signal
> + * @status.tx_time_est: estimated airtime of skb (shared with @control)
> + * @status.tx_time: actual airtime consumed for transmission
>   * @status.ampdu_ack_len: AMPDU ack length
>   * @status.ampdu_len: AMPDU length
>   * @status.antenna: (legacy, kept only for iwlegacy)
> - * @status.tx_time: airtime consumed for transmission
>   * @status.is_valid_ack_signal: ACK signal is valid
>   * @status.status_driver_data: driver use area
>   * @ack: union part for pure ACK data
> @@ -1026,11 +1029,17 @@ struct ieee80211_tx_info {
>                                 /* only needed before rate control */
>                                 unsigned long jiffies;
>                         };
> +                       union {
> +                               codel_time_t enqueue_time;
> +                               struct {
> +                                       u16 tx_time_est; /* shared with status */
> +                                       u16 reserved; /* padding for alignment */
> +                               };
> +                       };
> +                       u32 flags;
>                         /* NB: vif can be NULL for injected frames */
>                         struct ieee80211_vif *vif;
>                         struct ieee80211_key_conf *hw_key;
> -                       u32 flags;
> -                       codel_time_t enqueue_time;
>                 } control;
>                 struct {
>                         u64 cookie;
> @@ -1038,12 +1047,13 @@ struct ieee80211_tx_info {
>                 struct {
>                         struct ieee80211_tx_rate rates[IEEE80211_TX_MAX_RATES];
>                         s32 ack_signal;
> +                       u16 tx_time_est; /* shared with control */
> +                       u16 tx_time;
>                         u8 ampdu_ack_len;
>                         u8 ampdu_len;
>                         u8 antenna;
> -                       u16 tx_time;
>                         bool is_valid_ack_signal;
> -                       void *status_driver_data[19 / sizeof(void *)];
> +                       void *status_driver_data[16 / sizeof(void *)];
>                 } status;
>                 struct {
>                         struct ieee80211_tx_rate driver_rates[
> @@ -1126,6 +1136,8 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
>                      offsetof(struct ieee80211_tx_info, control.rates));
>         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) !=
>                      offsetof(struct ieee80211_tx_info, driver_rates));
> +       BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, control.tx_time_est) !=
> +                    offsetof(struct ieee80211_tx_info, status.tx_time_est));
>         BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, status.rates) != 8);
>         /* clear the rate counts */
>         for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
>