| Message ID | 157115993866.2500430.13989567853855880476.stgit@toke.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Airtime Queue Limits (AQL) to mac80211 | expand |
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++)
>
Kan Yan <kyan@google.com> writes: > 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; Ah, bugger, of course the driver that actually needs this is using the full driver_data space :P > 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? Hmm, the driver_data field is defined as an array of pointers, so we can only shrink it in increments of sizeof(void *). I think it may be feasible to shrink it (as in, I don't think any drivers are actually using the full 40 bytes), but doing this in a way that will gain us a 2-byte space that is also usable in the case driver_data is *not* used (i.e., it needs be able to align with a field in .control and .status as well) would require some serious surgery of the whole ieee80211_tx_info... However, there's a nice juicy 'u16 ack_frame_id' at the start of ieee80211_tx_info. Could we potentially use that? We could use the top bit as a disambiguation flag; I think we're fine with 15 bits for the TX time itself (a single packet won't exceed 8ms or TX time), so if we can live with 15 bits of ACK frame ID space, that could be a way forward? Johannes, what do you think? -Toke
On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote: > However, there's a nice juicy 'u16 ack_frame_id' at the start of > ieee80211_tx_info. Could we potentially use that? We could use the top > bit as a disambiguation flag; I think we're fine with 15 bits for the TX > time itself (a single packet won't exceed 8ms or TX time), so if we can > live with 15 bits of ACK frame ID space, that could be a way forward? I was going to say that should work as we only ever have a handful of ACK frame IDs, but ... you still need the airtime even for a frame that userspace wants to know the ACK status of, no? We could pull the ack_frame_id out-of-line using the skb extensions framework, but I'm not sure we should allocate one of the possible 8 extension IDs for that either ... What we really should do is convert all (relevant) drivers to use rate tables instead of having all the rates in the TX info, then we'd get a lot of space, but that's a lot of work ... johannes
On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote: > Kan Yan <kyan@google.com> writes: > > > 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; > > Ah, bugger, of course the driver that actually needs this is using the > full driver_data space :P Looks like you could shrink *this* fairly easily though. E.g. most likely vif == txq->vif unless txq==NULL, so it's down to 22 bytes plus a bit/flag for knowing whether the pointer is a vif directly (if no TXQ) or a TXQ. > > 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? > > Hmm, the driver_data field is defined as an array of pointers, so we can > only shrink it in increments of sizeof(void *). I think it may be > feasible to shrink it (as in, I don't think any drivers are actually > using the full 40 bytes), It doesn't have to be defined like that, just was most convenient as driers were using pointers there. > but doing this in a way that will gain us a > 2-byte space that is also usable in the case driver_data is *not* used > (i.e., it needs be able to align with a field in .control and .status as > well) would require some serious surgery of the whole ieee80211_tx_info... Yeah, good point, this doesn't help at all ... johannes
On 10/18/2019 05:35 AM, Johannes Berg wrote: > On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote: >> Kan Yan <kyan@google.com> writes: >> >>> 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; >> >> Ah, bugger, of course the driver that actually needs this is using the >> full driver_data space :P > > Looks like you could shrink *this* fairly easily though. > > E.g. most likely vif == txq->vif unless txq==NULL, so it's down to 22 > bytes plus a bit/flag for knowing whether the pointer is a vif directly > (if no TXQ) or a TXQ. And of course you get two bits in every pointer (0x3) and likely the dma addr too. Plenty of space! Thanks, Ben
Johannes Berg <johannes@sipsolutions.net> writes: > On Fri, 2019-10-18 at 12:15 +0200, Toke Høiland-Jørgensen wrote: > >> However, there's a nice juicy 'u16 ack_frame_id' at the start of >> ieee80211_tx_info. Could we potentially use that? We could use the top >> bit as a disambiguation flag; I think we're fine with 15 bits for the TX >> time itself (a single packet won't exceed 8ms or TX time), so if we can >> live with 15 bits of ACK frame ID space, that could be a way forward? > > I was going to say that should work as we only ever have a handful of > ACK frame IDs, but ... you still need the airtime even for a frame that > userspace wants to know the ACK status of, no? Oh, right. Well, let's try to do the actual math... A full-size (1538 bytes) packet takes ~2050 microseconds to transmit at 6 Mbps. Adding in overhead, it's certainly still less that 4096 us, so 12 bits is plenty. That leaves four bits for the ACK status ID if we just split the u16; if we only ever have "a handful", that should be enough, no? We could also split 5/11. That would support up to 32 ACK IDs, and we can just truncate the airtime at 2048 us, which is not a big deal I'd say. We could even split 6/10 and only need to truncate the TX time at rates below 13 Mbps... Or we could sacrifice a bit to the flag and only truncate if the ACK status flag is set. Think it mostly depends on what is the smallest ID space for ACK IDs we can live with? :) -Toke
On Fri, 2019-10-18 at 15:31 +0200, Toke Høiland-Jørgensen wrote: > Well, let's try to do the actual math... A full-size (1538 bytes) packet > takes ~2050 microseconds to transmit at 6 Mbps. Adding in overhead, it's > certainly still less that 4096 us, so 12 bits is plenty. What about A-MSDUs? But I guess maximum continous transmissions are at most 4ms anyway, so a single packet should never be longer. > That leaves > four bits for the ACK status ID if we just split the u16; if we only > ever have "a handful", that should be enough, no? It's how many are in flight at a time, 16 doesn't seem likely to happen, but I don't really know what applications are doing with it now. Probably only wpa_s for the EAPOL TX status. > We could also split 5/11. That would support up to 32 ACK IDs, and we > can just truncate the airtime at 2048 us, which is not a big deal I'd > say. We can also play with the units of the airtime, e.g. making that a multiple of 2 or 4 us? Seems unlikely to matter much? > Think it mostly depends on what is the smallest ID space for ACK IDs we > can live with? :) :) TBH, I don't really know. In a lot of hardware using this is really bad for performance so it shouldn't be used much, so ... johannes
Johannes Berg <johannes@sipsolutions.net> writes: > On Fri, 2019-10-18 at 15:31 +0200, Toke Høiland-Jørgensen wrote: > >> Well, let's try to do the actual math... A full-size (1538 bytes) packet >> takes ~2050 microseconds to transmit at 6 Mbps. Adding in overhead, it's >> certainly still less that 4096 us, so 12 bits is plenty. > > What about A-MSDUs? But I guess maximum continous transmissions are at > most 4ms anyway, so a single packet should never be longer. Ah yeah, those could be a bit bigger, but yeah, 4ms should at least be enough. >> That leaves >> four bits for the ACK status ID if we just split the u16; if we only >> ever have "a handful", that should be enough, no? > > It's how many are in flight at a time, 16 doesn't seem likely to happen, > but I don't really know what applications are doing with it now. > Probably only wpa_s for the EAPOL TX status. Right. Well in that case, let's try it. As long as we fail in a reasonable way, we can just see if we run into anything that breaks? I guess in this case that means rejecting requests from userspace if we run out of IDs rather than silently wrapping and returning wrong data :) >> We could also split 5/11. That would support up to 32 ACK IDs, and we >> can just truncate the airtime at 2048 us, which is not a big deal I'd >> say. > > We can also play with the units of the airtime, e.g. making that a > multiple of 2 or 4 us? Seems unlikely to matter much? Sure, that's a good point! Increments of 4us means we can fit 4ms is 10 bits, leaving plenty of space for ACK IDs (hopefully). I'll rework the series to use that instead :) -Toke
On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote: > Right. Well in that case, let's try it. As long as we fail in a > reasonable way, we can just see if we run into anything that breaks? I > guess in this case that means rejecting requests from userspace if we > run out of IDs rather than silently wrapping and returning wrong data :) We can't reject due to how this works, but if the idr_alloc() fails then we'll just not give a status back to userspace later. > > > We could also split 5/11. That would support up to 32 ACK IDs, and we > > > can just truncate the airtime at 2048 us, which is not a big deal I'd > > > say. > > > > We can also play with the units of the airtime, e.g. making that a > > multiple of 2 or 4 us? Seems unlikely to matter much? > > Sure, that's a good point! Increments of 4us means we can fit 4ms is 10 > bits, leaving plenty of space for ACK IDs (hopefully). > > I'll rework the series to use that instead :) OK. There are two places that call idr_alloc() with a hardcoded limit of 0x10000, you'll have to fix those to have the right limit according to the bits you leave for the ACK id. johannes
On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote: > > > We can also play with the units of the airtime, e.g. making that a > > multiple of 2 or 4 us? Seems unlikely to matter much? > > Sure, that's a good point! Increments of 4us means we can fit 4ms is 10 > bits, leaving plenty of space for ACK IDs (hopefully). If you do need more bits (e.g. to be on the safe side and have space for 8ms) you could also steal bits out of 'band' (we only need 3 I think) and 'hw_queue' (not sure what the limit really is, but there aren't many users, seems like only iwlwifi/dvm and hwsim care, and those certainly don't need >32 queues). Of course if you leave more bits for later that's good too ;-) johannes
Johannes Berg <johannes@sipsolutions.net> writes: > On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote: > >> Right. Well in that case, let's try it. As long as we fail in a >> reasonable way, we can just see if we run into anything that breaks? I >> guess in this case that means rejecting requests from userspace if we >> run out of IDs rather than silently wrapping and returning wrong data :) > > We can't reject due to how this works, but if the idr_alloc() fails then > we'll just not give a status back to userspace later. OK, I guess someone will notice if that suddenly starts happening all the time ;) >> > > We could also split 5/11. That would support up to 32 ACK IDs, and we >> > > can just truncate the airtime at 2048 us, which is not a big deal I'd >> > > say. >> > >> > We can also play with the units of the airtime, e.g. making that a >> > multiple of 2 or 4 us? Seems unlikely to matter much? >> >> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10 >> bits, leaving plenty of space for ACK IDs (hopefully). >> >> I'll rework the series to use that instead :) > > OK. > > There are two places that call idr_alloc() with a hardcoded limit of > 0x10000, you'll have to fix those to have the right limit according to > the bits you leave for the ACK id. Yup, found those. Will send a new version of the series in a bit. -Toke
Johannes Berg <johannes@sipsolutions.net> writes: > On Fri, 2019-10-18 at 16:01 +0200, Toke Høiland-Jørgensen wrote: >> >> > We can also play with the units of the airtime, e.g. making that a >> > multiple of 2 or 4 us? Seems unlikely to matter much? >> >> Sure, that's a good point! Increments of 4us means we can fit 4ms is 10 >> bits, leaving plenty of space for ACK IDs (hopefully). > > If you do need more bits (e.g. to be on the safe side and have space for > 8ms) you could also steal bits out of 'band' (we only need 3 I think) > and 'hw_queue' (not sure what the limit really is, but there aren't many > users, seems like only iwlwifi/dvm and hwsim care, and those certainly > don't need >32 queues). > > Of course if you leave more bits for later that's good too ;-) Yeah, let's leave that for later. After all, with the limits we currently have configured, if a single packet takes up 4096 us, that will trigger the per-station queue throttling in itself, so I think we'll be fine (famous last words). -Toke
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++)