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@xxxxxxxxxx> wrote:
>
> From: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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++)
>
