From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sipsolutions.net (s3.sipsolutions.net [IPv6:2a01:4f8:191:72ef::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id 7984C3B2A4 for ; Tue, 17 Oct 2017 03:07:28 -0400 (EDT) Received: by sipsolutions.net with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1e4LyJ-0003gb-HL; Tue, 17 Oct 2017 09:07:27 +0200 Message-ID: <1508224046.10607.67.camel@sipsolutions.net> From: Johannes Berg To: Toke =?ISO-8859-1?Q?H=F8iland-J=F8rgensen?= , make-wifi-fast@lists.bufferbloat.net, linux-wireless@vger.kernel.org Date: Tue, 17 Oct 2017 09:07:26 +0200 In-Reply-To: <20171016160902.8970-2-toke@toke.dk> (sfid-20171016_180933_942626_9939A5C5) References: <20171016160902.8970-1-toke@toke.dk> <20171016160902.8970-2-toke@toke.dk> (sfid-20171016_180933_942626_9939A5C5) Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.26.0-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Make-wifi-fast] [PATCH 2/2] mac80211: Add airtime accounting and scheduling to TXQs X-BeenThere: make-wifi-fast@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Oct 2017 07:07:28 -0000 > Only ath9k currently sets the AIRTIME_ACCOUNTING flag. I think you should actually make that a separate patch. In the previous patch that's not possible or it breaks things, but this patch just adds a new feature that drivers _may_ use, they don't have to since it doesn't change the API, so you should split the patches. [snip ath9k, I don't really know anything about it] > --- a/include/net/mac80211.h > +++ b/include/net/mac80211.h > @@ -1202,6 +1202,7 @@ struct ieee80211_rx_status { > u32 device_timestamp; > u32 ampdu_reference; > u32 flag; > + u16 rx_time; I'd prefer this were called "airtime" or such, and you clearly need to add documentation regarding this field. In particular, you should point to the IEEE80211_HW_AIRTIME_ACCOUNTING hw flag, and document what should be taken into account here (preamble length, IFS, rts/cts/ack, etc.?) and additionally how this should be handled wrt. A-MPDU (and A- MSDU where decapsulated by the device.) For aggregation, either form, I expect you just want to see 0 for all but the very first frame? but that may be very difficult for some drivers to implement. > +++ b/net/mac80211/rx.c > @@ -1637,6 +1637,15 @@ ieee80211_rx_h_sta_process(struct > ieee80211_rx_data *rx) > if (ieee80211_vif_is_mesh(&rx->sdata->vif)) > ieee80211_mps_rx_h_sta_process(sta, hdr); > > + /* airtime accounting */ > + if (ieee80211_hw_check(&sta->local->hw, AIRTIME_ACCOUNTING) > && Is there much point in this check? I think we can assume drivers are well-behaved and just leave the field at 0 if they don't support it? > + status->rx_time) { > + spin_lock_bh(&sta->lock); > + sta->airtime_stats.rx_airtime += status->rx_time; > + sta->airtime_deficit -= status->rx_time; > + spin_unlock_bh(&sta->lock); > + } I can't say I'm a big fan of the locking here, we have multi-queue RX where we spread the load across multiple CPUs, and this will lead to massive cache-line bouncing. Maybe we can make it per-CPU or something? This gets tricky though, so perhaps we can defer that to a separate patch to make it multi-queue aware. Perhaps for such drivers it'd be better anyway to report the used RX airtime per station *separately*, as part of some kind of statistics API, rather than inline through RX - there's no need for the inline reporting after all as long as the values are updated frequently enough. > +++ b/net/mac80211/status.c > @@ -823,6 +823,13 @@ static void __ieee80211_tx_status(struct > ieee80211_hw *hw, > ieee80211_lost_packet(sta, info); > } > } > + > + if (info->status.tx_time) { > + spin_lock_bh(&sta->lock); > + sta->airtime_stats.tx_airtime += info->status.tx_time; > + sta->airtime_deficit -= info->status.tx_time; > + spin_unlock_bh(&sta->lock); > + } > } Those lines also seem pretty long, and the concerns from above apply. Here you also don't have the hw_check, > /* SNMP counters > @@ -947,6 +954,14 @@ void ieee80211_tx_status_ext(struct ieee80211_hw > *hw, > sta->status_stats.retry_failed++; > sta->status_stats.retry_count += retry_count; > > + if (ieee80211_hw_check(&local->hw, AIRTIME_ACCOUNTING) && but here you do? tx_time already existed anyway though, for other purposes, so you probably need the check (or do useless work if the driver uses tx_time for WMM-AC but not for airtime accounting). > if (list_empty(&txqi->schedule_order)) { > - list_add_tail(&txqi->schedule_order, &local->active_txqs); > + list_add_tail(&txqi->schedule_order, &local->active_txqs_new); > ret = 1; > } lines seem long? > + txqi = list_first_entry(head, struct txq_info, > schedule_order); > + > + if (txqi->txq.sta) { > + struct sta_info *sta = container_of(txqi->txq.sta, > struct sta_info, sta); > + > + spin_lock_bh(&sta->lock); > + if (sta->airtime_deficit < 0) { > + sta->airtime_deficit += > IEEE80211_AIRTIME_QUANTUM; > + list_move_tail(&txqi->schedule_order, > &local->active_txqs_old); > + spin_unlock_bh(&sta->lock); > + goto begin; > + } > + spin_unlock_bh(&sta->lock); > + } ditto here. johannes