| Message ID | 156889576534.191202.17686228416284995279.stgit@alrua-x1 (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Add Airtime Queue Limits (AQL) to mac80211 | expand |
Hi, Sorry for the long time to review here ... On Thu, 2019-09-19 at 14:22 +0200, Toke Høiland-Jørgensen 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. Seems reasonable to me, if we end up needing it and don't have an out- of-band path (that you seem to have been discussing in this long thread too) johannes
Johannes Berg <johannes@sipsolutions.net> writes: > Hi, > > Sorry for the long time to review here ... > > On Thu, 2019-09-19 at 14:22 +0200, Toke Høiland-Jørgensen 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. > > Seems reasonable to me, if we end up needing it and don't have an out- > of-band path (that you seem to have been discussing in this long > thread too) Awesome! Any idea for how to make it work on big-endian systems? I got a splat from the kbuild robot that triggered the BUILD_BUG_ON when building for m68k. I assume it's the union with codel_time_t that ends up being aligned wrong... -Toke
On Tue, 2019-10-01 at 11:08 +0200, Toke Høiland-Jørgensen wrote: > > Awesome! Any idea for how to make it work on big-endian systems? I got a > splat from the kbuild robot that triggered the BUILD_BUG_ON when > building for m68k. I assume it's the union with codel_time_t that ends > up being aligned wrong... Hmm. Pad out the u16 part of the union by putting it into a struct, or perhaps it's enough to make the union __packed? johannes
Johannes Berg <johannes@sipsolutions.net> writes: > On Tue, 2019-10-01 at 11:08 +0200, Toke Høiland-Jørgensen wrote: >> >> Awesome! Any idea for how to make it work on big-endian systems? I got a >> splat from the kbuild robot that triggered the BUILD_BUG_ON when >> building for m68k. I assume it's the union with codel_time_t that ends >> up being aligned wrong... > > Hmm. Pad out the u16 part of the union by putting it into a struct, or > perhaps it's enough to make the union __packed? Yeah, another level of structs would probably work, but wanted to avoid another level of indentation. Although I guess even that would not make this the most-indented part of ieee80211_tx_info ;) I guess I'll look into the __packed thing, and go with another struct otherwise... -Toke
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 523c6a09e1c8..b12d378621b0 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -975,20 +975,22 @@ 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.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 +1028,14 @@ struct ieee80211_tx_info { /* only needed before rate control */ unsigned long jiffies; }; + union { + codel_time_t enqueue_time; + u16 tx_time_est; /* shared with status */ + }; + 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 +1043,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 +1132,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++)