Re: [PATCH RFC v3 0/4] Move TXQ scheduling into mac80211
On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: > - I didn't get rid of the register_airtime() callback. As far as I can tell, > only iwlwifi uses the tx_time field in the struct tx_info. Which means that > we *could* probably use it for this and just make the other drivers set it; > but I'm not sure what effects that would have in relation to WMM-AC for > those drivers, so I chickened out. Will have to try it out, I guess; but it > also depends on whether ath10k needs to be able to report airtime > asynchronously anyway. So I'll hold off on that for a bit more. I don't think you need to be concerned, the reporting through this has no immediate effect as the driver would also have to set the feature flag (NL80211_FEATURE_SUPPORTS_WMM_ADMISSION) for userspace to be able to use WMM admission TSPECs, and getting tx_tspec->admitted_time to be non-zero in ieee80211_sta_tx_wmm_ac_notify(). I just think that this may be desirable to drivers eventually, and/or maybe iwlwifi wants to take advantage of the airtime scheduling eventually, so having two APIs overlapping seems a bit strange. johannes
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: > > Usage of the new API is optional, so drivers can be ported one at a time. With the 1:1 hardware queue/txq mapping in iwlwifi (we're close to getting that patch in, though now the Jewish holidays mean a delay), I'm not sure we'd be able to do this at all in iwlwifi. So this may not be a case of porting one at a time until we can get rid of it ... It would be nice to be able to use it, for better queue behaviour, but it would mean much more accounting in iwlwifi? Not even sure off the top of my head how to do that. johannes
Re: [PATCH RFC v3 0/4] Move TXQ scheduling into mac80211
On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: > > A few things that were discussed in the last round that I did *not* change: Thanks for this list btw. > - I did not add any locking around next_txq(); the driver is still supposed > to maintain a lock that prevents two threads from trying to schedule the > same AC at the same time. This is what drivers already do, so I figured it > was easier to just keep it that way rather than do it in mac80211. I'll look at this in the code, but from a maintainer perspective I'm somewhat worried that this will lead to issues that are really the driver's fault, but surface in mac80211. I don't know how easy it would be to catch that. Might be fine, but something to keep in mind. johannes
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote:
> +/**
> + * ieee80211_next_txq - get next tx queue to pull packets from
> + *
> + * @hw: pointer as obtained from ieee80211_alloc_hw()
> + * @ac: AC number to return packets from.
> + * @first: Setting this to true signifies the start of a new scheduling
> round.
> + * Each TXQ will only be returned at most exactly once in each round.
> + *
> + * Returns the next txq if successful, %NULL if no queue is eligible. If a
> txq
> + * is returned, it will have been removed from the scheduler queue and needs
> to
> + * be re-scheduled with ieee80211_schedule_txq() to continue to be active.
> Note
> + * that the driver is responsible for locking to ensuring two different
> threads
> + * don't schedule the same AC simultaneously.
"Note that [...] to ensure [...]" would seem better to me?
> + */
> +struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
> + bool first);
Why should "ac" be signed? Is there some (undocumented) behaviour that
allows for passing -1 for "don't care" or "pick highest with frames"?
You said "optionally" in the commit message, but I don't think that's
true, since you just use ac to index the array in the code.
> +bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
> + struct ieee80211_txq *txq)
> +{
> + struct ieee80211_local *local = hw_to_local(hw);
> + struct txq_info *txqi = to_txq_info(txq);
> + bool ret = false;
> +
> + spin_lock_bh(&local->active_txq_lock);
> +
> + if (list_empty(&txqi->schedule_order)) {
Is there a case where it's allowed to call this while *not* empty? At
least for the drivers it seems not, so perhaps when called from the
driver it should WARN() here or so?
I'm just a bit worried that drivers will get this a bit wrong, and we'll
never know, but things won't work properly in some weird way.
> + list_add_tail(&txqi->schedule_order,
> + &local->active_txqs[txq->ac]);
> + ret = true;
> + }
> +
> + spin_unlock_bh(&local->active_txq_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(ieee80211_schedule_txq);
I'd remove the return value - the driver has no need to know that it
raced with potential rescheduling in mac80211 due to a new packet, and
you don't use the return value in mac80211 either.
It also seems to me that you forgot to say when it should be
rescheduled? As is, following the documentation would sort of makes it
seem you should take the queue and put it back at the end (without any
conditions), and that could lead to "scheduling loops" since empty
queues would always be put back when they shouldn't be?
Also, come to think of it, but I guess the next patch(es) solve(s) that
- if mac80211 adds a packet while you have this queue scheduled then you
would take that packet even if it's questionable it belongs to this
round?
>From a driver perspective, I think I'd prefer an API called e.g.
ieee80211_return_txq()
that does the check internally if we need to schedule the queue (i.e.
there are packets on it). I was going to say we could even annotate with
sparse, but no - we can't because ieee80211_next_txq() may return NULL.
Still, it's easier to ensure that all cleanup paths call
ieee80211_return_txq() if it's not conditional, IMHO.
> +struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
> + bool first)
> +{
> + struct ieee80211_local *local = hw_to_local(hw);
> + struct txq_info *txqi = NULL;
> +
> + spin_lock_bh(&local->active_txq_lock);
> +
> + if (first)
> + local->schedule_round[ac]++;
(here - clearly ac must be 0 <= ac < IEEE80211_NUM_ACS)
> + if (list_empty(&local->active_txqs[ac]))
> + goto out;
> +
> + txqi = list_first_entry(&local->active_txqs[ac],
> + struct txq_info,
> + schedule_order);
> +
> + if (txqi->schedule_round == local->schedule_round[ac])
> + txqi = NULL;
that assigment seems unnecessary? txqi defaults to NULL.
> + else
> + list_del_init(&txqi->schedule_order);
> + out:
> + spin_unlock_bh(&local->active_txq_lock);
> +
> + if (!txqi)
> + return NULL;
> +
> + txqi->schedule_round = local->schedule_round[ac];
> + return &txqi->txq;
> +}
> +EXPORT_SYMBOL(ieee80211_next_txq);
johannes
Re: [PATCH RFC v3 2/4] mac80211: Add airtime accounting and scheduling to TXQs
On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote:
>
> +/**
> + * ieee80211_airtime_may_transmit - check whether TXQ is allowed to transmit
> + *
> + * This function is used to check whether given txq is allowed to transmit by
> + * the airtime scheduler, and can be used by drivers to access the airtime
> + * fairness accounting without going using the scheduling order enfored by
> + * next_txq().
> + *
> + * Returns %true if the airtime scheduler does not think the TXQ should be
> + * throttled. This function can also have the side effect of rotating the
> TXQ in
> + * the scheduler rotation, which will eventually bring the deficit to
> positive
> + * and allow the station to transmit again.
> + *
> + * If this function returns %true, the TXQ will have been removed from the
> + * scheduling. It is the driver's responsibility to add it back (by calling
> + * ieee80211_schedule_txq()) if needed.
> + *
> + * @hw: pointer as obtained from ieee80211_alloc_hw()
> + * @txq: pointer obtained from station or virtual interface
> + *
> + */
Spurious newline there at the end.
(As an aside from the review, perhaps this would be what we could use in
iwlwifi, but I'll need to think about _when_ to add it back to the
scheduling later).
> + * @NL80211_EXT_FEATURE_AIRTIME_FAIRNESS: Driver supports airtime fairness
> + * scheduling.
> + *
> * @NUM_NL80211_EXT_FEATURES: number of extended features.
> * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
> */
> @@ -5269,6 +5272,7 @@ enum nl80211_ext_feature_index {
> NL80211_EXT_FEATURE_SCAN_RANDOM_SN,
> NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT,
> NL80211_EXT_FEATURE_CAN_REPLACE_PTK0,
> + NL80211_EXT_FEATURE_AIRTIME_FAIRNESS,
Why is this necessary in this patch?
I think that this should probably come with more documentation too,
particularly what's expected of the driver here.
I'd prefer you reorder patches 2 and 3, and define everything in
cfg80211 first. That also makes it easier to reason about what happens
when the feature is not supported (since it will not be supported
anywhere). Then this can be included in the cfg80211 patch instead,
which places it better, and the mac80211 bits from the cfg80211 patch
can be included here, which also places those better.
> -txqi->flags & (1< "RUN",
> -txqi->flags & (1< AMPDU" : "",
> -txqi->flags & (1< NO-AMSDU" : "");
> +txqi->flags & (1 << IEEE80211_TXQ_STOP) ? "STOP"
> : "RUN",
> +txqi->flags & (1 << IEEE80211_TXQ_AMPDU) ? "
> AMPDU" : "",
> +txqi->flags & (1 << IEEE80211_TXQ_NO_AMSDU) ? "
> NO-AMSDU" : "");
consider BIT() instead as a cleanup? :)
(or maybe this is just a leftover from merging some other patches?)
> +static bool ieee80211_txq_check_deficit(struct ieee80211_local *local,
> + struct txq_info *txqi)
Maybe this should be called "has_deficit" or so - the polarity isn't
immediately clear to me.
> +{
> + struct sta_info *sta;
> +
> + lockdep_assert_held(&local->active_txq_lock);
> +
> + if (!txqi->txq.sta)
> + return true;
> +
> + sta = container_of(txqi->txq.sta, struct sta_info, sta);
> +
> + if (sta->airtime.deficit[txqi->txq.ac] > 0)
> + return true;
> +
> + if (txqi == list_first_entry_or_null(&local->active_txqs[txqi->txq.ac],
nit: I don't think you need _or_null here, since list_first_entry() will
"return" the head itself if the list is empty, which cannot match txqi?
Doesn't matter that much though, and if you think this is easier to
understand then that's fine.
> + struct txq_info,
> + schedule_order)) {
> + sta->airtime.deficit[txqi->txq.ac] += sta->airtime.weight;
> + list_move_tail(&txqi->schedule_order,
> +&local->active_txqs[txqi->txq.ac]);
> + }
> +
> + return false;
> +}
> +
> struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
>bool first)
> {
> @@ -3659,6 +3701,7 @@ struct ieee80211_txq *ieee80211_next_txq(struct
> ieee80211_hw *hw, s8 ac,
> if (first)
> local->schedule_round[ac]++;
>
> + begin:
> if (list_empty(&local->active_txqs[ac]))
> goto out;
>
> @@ -3666,6 +3709,9 @@ struct ieee80211_txq *ieee80211_next_txq(struct
> ieee80211_hw *hw, s8 ac,
> struct txq_info,
> schedule_order);
>
> + if (!ieee80211_txq_check_deficit(local, txqi))
> + goto begin;
This code isn't so badly indented - you could use a do { } while () loop
instead?
> if (txqi->schedule_round == local->schedule_round[ac])
> txqi = NULL;
and maybe you want this check fi
Re: [PATCH RFC v3 3/4] cfg80211: Add airtime statistics and settings
On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: > This adds airtime statistics to the cfg80211 station dump, and also adds a new > parameter to set the airtime weight of each station. The latter allows > userspace > to implement policies for different stations by varying their weights. Maybe some documentation on the weights - both for other implementers as well as users - would be useful. > + * @NL80211_STA_INFO_AIRTIME_WEIGHT: current airtime weight for station > + * (u16, usec) how could the weight be in usec? copy/paste issue? johannes
Re: [PATCH 09/22] mt76: remove q->hw_idx
On 2018-09-04 16:41, Stanislaw Gruszka wrote: > We use q->hw_idx in very few places, remove it and provide hwq index > by diffrent way. This will allow ot unify conf_tx mac80211 callback. > > Signed-off-by: Stanislaw Gruszka I think this patch is a step in the wrong direction, especially when it comes to dealing with extra hardware queues for powersave delivery, or beacons (on MT7603). Also, removing q->hw_idx is not really necessary for unifying conf_tx callbacks either. - Felix
Re: [PATCH 39/42] mt76: move some irq code to common mmio module
On 2018-09-06 12:43, Stanislaw Gruszka wrote: > On Thu, Sep 06, 2018 at 11:33:55AM +0200, Felix Fietkau wrote: >> On 2018-09-06 11:18, Stanislaw Gruszka wrote: >> > Move some irq handling code to generic mmio module. >> > >> > Signed-off-by: Stanislaw Gruszka >> Please drop this patch. This won't work on MT7603 and later. > > Yeah, I thought MT_INT_MASK_CSR is the same 0x0204 for all chips, > but for MT7603 this is MT_HIF(0x204). I assume we can still unify > irq_lock, irqmask fields in common structure. > > Anyway would be good to have mt7603 driver unstreamed, so we could > easily see what can be shared in mt76 generic code for all chips. > > Felix, do you have plans to upstream it? Want some help with that ? I plan on sending it upstream soon. It's still not fully stable, but that shouldn't be a blocker. - Felix
Re: [PATCH 06/42] mt76: move mt76x2u_set_txinfo in mt76x02-lib module
On 2018-09-06 11:18, Stanislaw Gruszka wrote:
> From: Lorenzo Bianconi
>
> Move mt76x2u_set_txinfo routine in mt76x02-lib module and rename it in
> mt76x02_set_txinfo in order to be reused in mt76x0 driver
>
> Signed-off-by: Lorenzo Bianconi
> Signed-off-by: Stanislaw Gruszka
> ---
> drivers/net/wireless/mediatek/mt76/mt76x02_util.c | 21
> drivers/net/wireless/mediatek/mt76/mt76x02_util.h | 1 +
> drivers/net/wireless/mediatek/mt76/mt76x2u_core.c | 24
> +--
> 3 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> index e4cf4e934153..6ed558c12786 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> @@ -412,4 +412,25 @@ void mt76x02_tx_complete_skb(struct mt76_dev *mdev,
> struct mt76_queue *q,
> }
> EXPORT_SYMBOL_GPL(mt76x02_tx_complete_skb);
>
> +int mt76x02_set_txinfo(struct sk_buff *skb, struct mt76_wcid *wcid, u8 ep)
> +{
> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> + enum mt76_qsel qsel;
> + u32 flags;
> +
> + if ((info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE) ||
> + ep == MT_EP_OUT_HCCA)
> + qsel = MT_QSEL_MGMT;
> + else
> + qsel = MT_QSEL_EDCA;
> +
> + flags = FIELD_PREP(MT_TXD_INFO_QSEL, qsel) |
> + MT_TXD_INFO_80211;
> + if (!wcid || wcid->hw_key_idx == 0xff || wcid->sw_iv)
> + flags |= MT_TXD_INFO_WIV;
> +
> + return mt76u_skb_dma_info(skb, WLAN_PORT, flags);
Common MT76x02 code must not call USB specific functions.
- Felix
Troubleshooting hostapd and TLS authentication
hI, I would like to test network connection between simulated Wi-Fi access point and a station by using hostapd. Both reside on the same tesing machine which does not have Wi-Fi adapter. I would like to test 802.1x + TLS authentication. When I run hostapd then I see error message: OpenSSL: tls_read_pkcs12 - Failed to use PKCS#12 file error:0D0680A8:asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag OpenSSL: pending error: error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 error OpenSSL: tls_global_private_key - Failed to load private key error::lib(0):func(0):reason(0) TLS: Failed to set global parameters Failed to set TLS parameters DETAILS Distro: RHEL 7.6 Beta kernel-3.10.0-933.el7.x86_64 NetworkManager-1.12.0-2.el7.x86_64 hostapd-2.6-7.el7.x86_64 openssl-1.0.2k-15.el7.x86_64 wpa_supplicant-2.6-11.el7.x86_64 The simulated AP has interface "wlan1". The simulated station has "wlan0". hostapd's CONFIGURATION # Hostapd configuration for 802.1x client testing interface=wlan1 driver=nl80211 ctrl_interface=/var/run/hostapd ctrl_interface_group=0 ssid=wpa2-eap country_code=EN hw_mode=g channel=7 auth_algs=3 wpa=3 ieee8021x=1 eapol_version=1 wpa_key_mgmt=WPA-EAP WPA-PSK wpa_passphrase=secret123 eap_reauth_period=3600 eap_server=1 use_pae_group_addr=1 eap_user_file=/etc/hostapd/hostapd.eap_user ca_cert=/etc/hostapd/ssl/hostapd.ca.pem dh_file=/etc/hostapd/ssl/hostapd.dh.pem server_cert=/etc/hostapd/ssl/hostapd.cert.pem private_key=/etc/hostapd/ssl/hostapd.key.pem private_key_passwd=redhat All mentioned certificates exist on the testing machine. Server certificate and private key match. CONTENTS OF /etc/hostapd/hostapd.eap_user # Create hostapd peap user file # Phase 1 authentication "user" MD5 "password" "test" TLS,TTLS,PEAP # Phase 2 authentication (tunnelled within EAP-PEAP or EAP-TTLS) "TESTERS\test_mschapv2" MSCHAPV2 "password" [2] "test_md5" MD5 "password" [2] "test_gtc" GTC "password" [2] # Tunneled TLS and non-EAP authentication inside the tunnel. "test_ttls" TTLS-PAP,TTLS-CHAP,TTLS-MSCHAP,TTLS-MSCHAPV2 "password" [2] QUESTIONS 1. Are there any errors in the hostapd's configuration? 2. Is the error after start due to certificates? 3. Is the error due to OpenSSL on the testing machine? Thanks Pavlin
Re: [PATCH RFC v3 0/4] Move TXQ scheduling into mac80211
Kan Yan writes: > Hi Toke, > > It is great to see finally there will be a general airtime fairness > support in mac80211. I feel this patch set could still use some > improvements to make it works better with 11ac drivers like ath10k. I > did a version of airtime fairness in the ath10k driver that used in > Google Wifi and I'd like to share a few things I learned from this > experience. Hi Kan Thanks for weighing in! > The idea is to use airtime accounting to give firmware just enough > frames to keep it busy and form good aggregations, and keeps the rest > of frames in mac80211 layer queues so Fq_Codel can work on it to > control latency. I think this is a great approach, that we should definitely adopt in mac80211. However, I'm not sure the outstanding airtime limiting should be integrated into the fairness scheduling, since there are drivers such as ath9k that don't need it. Rather, I'd propose that we figure out the API for fairness scheduling first, and add the BQL-like limiter as a separate layer. They can be integrated in the sense that the limiter's estimate of airtime can be used for fairness scheduling as well in the case where the driver/firmware doesn't have a better source of airtime usage. Does that make sense? -Toke
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
Johannes Berg writes: > On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: >> >> Usage of the new API is optional, so drivers can be ported one at a time. > > With the 1:1 hardware queue/txq mapping in iwlwifi (we're close to > getting that patch in, though now the Jewish holidays mean a delay), > I'm not sure we'd be able to do this at all in iwlwifi. So this may > not be a case of porting one at a time until we can get rid of it ... Could you elaborate a bit more on how the hwq/txq stuff works in iwl? Does the driver just hand off a TXQ to the hardware on wake_txq(), which is then scheduled by the hardware after that? Or how does the mapping to hwqs work, and how is the hardware notified that there are still packets queued / that new packets have arrived for an already mapped txq? > It would be nice to be able to use it, for better queue behaviour, but > it would mean much more accounting in iwlwifi? Not even sure off the > top of my head how to do that. I think we'll need to have some kind of fallback airtime estimation in mac80211 that calculates airtime from packet size and rate information. Not all drivers can get this from the hardware, it seems. See my reply to Kan about the BQL-like behaviour as well. Does iwl get airtime usage information for every packet on tx complete? -Toke
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
Johannes Berg writes:
> On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote:
>
>> +/**
>> + * ieee80211_next_txq - get next tx queue to pull packets from
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @ac: AC number to return packets from.
>> + * @first: Setting this to true signifies the start of a new scheduling
>> round.
>> + * Each TXQ will only be returned at most exactly once in each
>> round.
>> + *
>> + * Returns the next txq if successful, %NULL if no queue is eligible. If a
>> txq
>> + * is returned, it will have been removed from the scheduler queue and
>> needs to
>> + * be re-scheduled with ieee80211_schedule_txq() to continue to be active.
>> Note
>> + * that the driver is responsible for locking to ensuring two different
>> threads
>> + * don't schedule the same AC simultaneously.
>
> "Note that [...] to ensure [...]" would seem better to me?
>
>> + */
>> +struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
>> + bool first);
>
> Why should "ac" be signed? Is there some (undocumented) behaviour that
> allows for passing -1 for "don't care" or "pick highest with frames"?
>
> You said "optionally" in the commit message, but I don't think that's
> true, since you just use ac to index the array in the code.
There was in the previous version, but I removed that; guess I forgot to
change the sign and the commit message ;)
>> +bool ieee80211_schedule_txq(struct ieee80211_hw *hw,
>> +struct ieee80211_txq *txq)
>> +{
>> +struct ieee80211_local *local = hw_to_local(hw);
>> +struct txq_info *txqi = to_txq_info(txq);
>> +bool ret = false;
>> +
>> +spin_lock_bh(&local->active_txq_lock);
>> +
>> +if (list_empty(&txqi->schedule_order)) {
>
> Is there a case where it's allowed to call this while *not* empty? At
> least for the drivers it seems not, so perhaps when called from the
> driver it should WARN() here or so?
Yeah, mac80211 calls this on wake_txq, where it will often be scheduled
already. And since that can happen while drivers schedule stuff, it
could also be the case that a driver re-schedules a queue that is
already scheduled.
> I'm just a bit worried that drivers will get this a bit wrong, and
> we'll never know, but things won't work properly in some weird way.
What, subtle bugs in the data path that causes hangs in hard-to-debug
cases? Nah, that never happens ;)
>> +list_add_tail(&txqi->schedule_order,
>> + &local->active_txqs[txq->ac]);
>> +ret = true;
>> +}
>> +
>> +spin_unlock_bh(&local->active_txq_lock);
>> +
>> +return ret;
>> +}
>> +EXPORT_SYMBOL(ieee80211_schedule_txq);
>
> I'd remove the return value - the driver has no need to know that it
> raced with potential rescheduling in mac80211 due to a new packet, and
> you don't use the return value in mac80211 either.
Yeah, good point; that was left over from when the call to wake_txq was
conditional on this actually doing something...
> It also seems to me that you forgot to say when it should be
> rescheduled? As is, following the documentation would sort of makes it
> seem you should take the queue and put it back at the end (without any
> conditions), and that could lead to "scheduling loops" since empty
> queues would always be put back when they shouldn't be?
>
> Also, come to think of it, but I guess the next patch(es) solve(s) that
> - if mac80211 adds a packet while you have this queue scheduled then you
> would take that packet even if it's questionable it belongs to this
> round?
>
> From a driver perspective, I think I'd prefer an API called e.g.
>
> ieee80211_return_txq()
>
> that does the check internally if we need to schedule the queue (i.e.
> there are packets on it). I was going to say we could even annotate with
> sparse, but no - we can't because ieee80211_next_txq() may return NULL.
> Still, it's easier to ensure that all cleanup paths call
> ieee80211_return_txq() if it's not conditional, IMHO.
Good idea, will do.
>> +struct ieee80211_txq *ieee80211_next_txq(struct ieee80211_hw *hw, s8 ac,
>> + bool first)
>> +{
>> +struct ieee80211_local *local = hw_to_local(hw);
>> +struct txq_info *txqi = NULL;
>> +
>> +spin_lock_bh(&local->active_txq_lock);
>> +
>> +if (first)
>> +local->schedule_round[ac]++;
>
> (here - clearly ac must be 0 <= ac < IEEE80211_NUM_ACS)
>
>> +if (list_empty(&local->active_txqs[ac]))
>> +goto out;
>> +
>> +txqi = list_first_entry(&local->active_txqs[ac],
>> +struct txq_info,
>> +schedule_order);
>> +
>> +if (txqi->schedule_round == local->schedule_round[ac])
>> +txqi = NULL;
>
> that assigment seems unnecessary? txqi defaults to NULL.
No, this is an 'undo' of the previous line. I.e., we get the first txqi,
check if we've
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
On Mon, 2018-09-10 at 12:57 +0200, Toke Høiland-Jørgensen wrote: > Johannes Berg writes: > > > On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: > > > > > > Usage of the new API is optional, so drivers can be ported one at a time. > > > > With the 1:1 hardware queue/txq mapping in iwlwifi (we're close to > > getting that patch in, though now the Jewish holidays mean a delay), > > I'm not sure we'd be able to do this at all in iwlwifi. So this may > > not be a case of porting one at a time until we can get rid of it ... > > Could you elaborate a bit more on how the hwq/txq stuff works in iwl? I can try. > Does the driver just hand off a TXQ to the hardware on wake_txq(), which > is then scheduled by the hardware after that? Or how does the mapping to > hwqs work, and how is the hardware notified that there are still packets > queued / that new packets have arrived for an already mapped txq? Basically, we use the TXQ driver data to do this. On initialization, we set all TXQs to have no hardware queue mapped. Then, when the first wake happens for this TXQ, we allocate a hardware queue for this RA/TID. >From then on, right now we basically just try to refill the hardware queue from the TXQ whenever the hardware releases frames from that queue. If there are none, we fall back to putting them on the hardware queue from the wake signal. > > It would be nice to be able to use it, for better queue behaviour, but > > it would mean much more accounting in iwlwifi? Not even sure off the > > top of my head how to do that. > > I think we'll need to have some kind of fallback airtime estimation in > mac80211 that calculates airtime from packet size and rate information. > Not all drivers can get this from the hardware, it seems. See my reply > to Kan about the BQL-like behaviour as well. > > Does iwl get airtime usage information for every packet on tx complete? Yes, we do. johannes
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
On Mon, 2018-09-10 at 13:02 +0200, Toke Høiland-Jørgensen wrote: > > > Is there a case where it's allowed to call this while *not* empty? At > > least for the drivers it seems not, so perhaps when called from the > > driver it should WARN() here or so? > > Yeah, mac80211 calls this on wake_txq, where it will often be scheduled > already. And since that can happen while drivers schedule stuff, it > could also be the case that a driver re-schedules a queue that is > already scheduled. Right, I kinda gathered that but was thinking more from a driver POV as I was reviewing, makes sense. > > I'm just a bit worried that drivers will get this a bit wrong, and > > we'll never know, but things won't work properly in some weird way. > > What, subtle bugs in the data path that causes hangs in hard-to-debug > cases? Nah, that never happens ;) Good :-P Actually though, we can't put a warn on there, because the driver might take a txq, then mac80211 puts it on the list due to a new packet, and then the driver also returns it. > > > + txqi = list_first_entry(&local->active_txqs[ac], > > > + struct txq_info, > > > + schedule_order); > > > + > > > + if (txqi->schedule_round == local->schedule_round[ac]) > > > + txqi = NULL; > > > > that assigment seems unnecessary? txqi defaults to NULL. > > No, this is an 'undo' of the previous line. I.e., we get the first txqi, > check if we've already seen it this round, and if we have, unset txqi so > the function returns NULL (causing the driver to stop looping). Err, yeah, obviously. johannes
Re: [PATCH RFC v3 2/4] mac80211: Add airtime accounting and scheduling to TXQs
Johannes Berg writes:
> On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote:
>>
>> +/**
>> + * ieee80211_airtime_may_transmit - check whether TXQ is allowed to transmit
>> + *
>> + * This function is used to check whether given txq is allowed to transmit
>> by
>> + * the airtime scheduler, and can be used by drivers to access the airtime
>> + * fairness accounting without going using the scheduling order enfored by
>> + * next_txq().
>> + *
>> + * Returns %true if the airtime scheduler does not think the TXQ should be
>> + * throttled. This function can also have the side effect of rotating the
>> TXQ in
>> + * the scheduler rotation, which will eventually bring the deficit to
>> positive
>> + * and allow the station to transmit again.
>> + *
>> + * If this function returns %true, the TXQ will have been removed from the
>> + * scheduling. It is the driver's responsibility to add it back (by calling
>> + * ieee80211_schedule_txq()) if needed.
>> + *
>> + * @hw: pointer as obtained from ieee80211_alloc_hw()
>> + * @txq: pointer obtained from station or virtual interface
>> + *
>> + */
>
> Spurious newline there at the end.
>
> (As an aside from the review, perhaps this would be what we could use in
> iwlwifi, but I'll need to think about _when_ to add it back to the
> scheduling later).
Yeah, this is the API that would be used by drivers where the actual
scheduling of TXQs is done by the hardware (i.e., ath10k in pull/push
mode, and iwlwifi if I understand you correctly); whereas the next_txq()
call is for drivers that do the scheduling in software (ath10k without
pull/push, ath9k and mt76).
Basically, the way I envision this is the drivers doing something like:
function on_hw_notify(hw, txq) {
if (ieee80211_airtime_may_transmit(txq)) {
while (hw_has_space() && (pkt = ieee80211_tx_dequeue(hw, txq)))
push_to_hw(pkt);
ieee80211_schedule_txq(txq);
}
}
>> + * @NL80211_EXT_FEATURE_AIRTIME_FAIRNESS: Driver supports airtime fairness
>> + * scheduling.
>> + *
>> * @NUM_NL80211_EXT_FEATURES: number of extended features.
>> * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
>> */
>> @@ -5269,6 +5272,7 @@ enum nl80211_ext_feature_index {
>> NL80211_EXT_FEATURE_SCAN_RANDOM_SN,
>> NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT,
>> NL80211_EXT_FEATURE_CAN_REPLACE_PTK0,
>> +NL80211_EXT_FEATURE_AIRTIME_FAIRNESS,
>
> Why is this necessary in this patch?
It's meant as a way for the driver to signal that it has a source of
airtime information, and thus would like to be scheduled to take this
into account.
> I think that this should probably come with more documentation too,
> particularly what's expected of the driver here.
Right :)
> I'd prefer you reorder patches 2 and 3, and define everything in
> cfg80211 first. That also makes it easier to reason about what happens
> when the feature is not supported (since it will not be supported
> anywhere). Then this can be included in the cfg80211 patch instead,
> which places it better, and the mac80211 bits from the cfg80211 patch
> can be included here, which also places those better.
Good idea, will do!
>> - txqi->flags & (1<> "RUN",
>> - txqi->flags & (1<> AMPDU" : "",
>> - txqi->flags & (1<> NO-AMSDU" : "");
>> + txqi->flags & (1 << IEEE80211_TXQ_STOP) ? "STOP"
>> : "RUN",
>> + txqi->flags & (1 << IEEE80211_TXQ_AMPDU) ? "
>> AMPDU" : "",
>> + txqi->flags & (1 << IEEE80211_TXQ_NO_AMSDU) ? "
>> NO-AMSDU" : "");
>
> consider BIT() instead as a cleanup? :)
>
> (or maybe this is just a leftover from merging some other patches?)
Yeah, this is a merging artifact; Rajkumar's patch added another flag,
that I removed again. Didn't notice that there was still a whitespace
change in this patch...
>> +static bool ieee80211_txq_check_deficit(struct ieee80211_local *local,
>> +struct txq_info *txqi)
>
> Maybe this should be called "has_deficit" or so - the polarity isn't
> immediately clear to me.
Yup, I suck at naming; gotcha ;)
>> +{
>> +struct sta_info *sta;
>> +
>> +lockdep_assert_held(&local->active_txq_lock);
>> +
>> +if (!txqi->txq.sta)
>> +return true;
>> +
>> +sta = container_of(txqi->txq.sta, struct sta_info, sta);
>> +
>> +if (sta->airtime.deficit[txqi->txq.ac] > 0)
>> +return true;
>> +
>> +if (txqi == list_first_entry_or_null(&local->active_txqs[txqi->txq.ac],
>
> nit: I don't think you need _or_null here, since list_first_entry() will
> "return" the head itself if the list is empty, which cannot match txqi?
> Doesn't matter that much though, and if you think this is easier to
> understand then that's fine.
>
>> + struct txq_info,
>> + schedule_order)) {
>> +
Re: [PATCH RFC v3 3/4] cfg80211: Add airtime statistics and settings
Johannes Berg writes: > On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: >> This adds airtime statistics to the cfg80211 station dump, and also adds a >> new >> parameter to set the airtime weight of each station. The latter allows >> userspace >> to implement policies for different stations by varying their weights. > > Maybe some documentation on the weights - both for other implementers as > well as users - would be useful. > >> + * @NL80211_STA_INFO_AIRTIME_WEIGHT: current airtime weight for station >> + * (u16, usec) > > how could the weight be in usec? copy/paste issue? Well, technically it is a usec value (the amount of airtime added to the deficit each round); but I agree that it shouldn't be documented as such if we want freedom to change the mechanism later... -Toke
Re: [PATCH RFC v3 0/4] Move TXQ scheduling into mac80211
Johannes Berg writes: > On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: > >> - I didn't get rid of the register_airtime() callback. As far as I can tell, >> only iwlwifi uses the tx_time field in the struct tx_info. Which means that >> we *could* probably use it for this and just make the other drivers set it; >> but I'm not sure what effects that would have in relation to WMM-AC for >> those drivers, so I chickened out. Will have to try it out, I guess; but it >> also depends on whether ath10k needs to be able to report airtime >> asynchronously anyway. So I'll hold off on that for a bit more. > > I don't think you need to be concerned, the reporting through this has > no immediate effect as the driver would also have to set the feature > flag (NL80211_FEATURE_SUPPORTS_WMM_ADMISSION) for userspace to be able > to use WMM admission TSPECs, and getting tx_tspec->admitted_time to be > non-zero in ieee80211_sta_tx_wmm_ac_notify(). Great! In that case I'll try moving the reporting go through the tx_info struct and check that it works for ath9k :) -Toke
Re: [PATCH RFC v3 0/4] Move TXQ scheduling into mac80211
Johannes Berg writes: > On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: >> >> A few things that were discussed in the last round that I did *not* change: > > Thanks for this list btw. > >> - I did not add any locking around next_txq(); the driver is still supposed >> to maintain a lock that prevents two threads from trying to schedule the >> same AC at the same time. This is what drivers already do, so I figured it >> was easier to just keep it that way rather than do it in mac80211. > > I'll look at this in the code, but from a maintainer perspective I'm > somewhat worried that this will lead to issues that are really the > driver's fault, but surface in mac80211. I don't know how easy it > would be to catch that. Yeah, I get what you mean. The alternative would be to have a ieee80211_start_schedule(ac) and ieee80211_end_schedule(ac), which basically just takes a lock. Would mean we could get rid of the 'first' parameter for next_txq(), so might not be such a bad idea; and if the driver has its own locking the extra locking in mac80211 would just be an always-uncontested spinlock, which shouldn't be much overhead, right? -Toke
Re: [PATCH RFC v3 2/4] mac80211: Add airtime accounting and scheduling to TXQs
On Mon, 2018-09-10 at 13:13 +0200, Toke Høiland-Jørgensen wrote:
>
> Yeah, this is the API that would be used by drivers where the actual
> scheduling of TXQs is done by the hardware (i.e., ath10k in pull/push
> mode, and iwlwifi if I understand you correctly); whereas the next_txq()
> call is for drivers that do the scheduling in software (ath10k without
> pull/push, ath9k and mt76).
>
> Basically, the way I envision this is the drivers doing something like:
>
> function on_hw_notify(hw, txq) {
> if (ieee80211_airtime_may_transmit(txq)) {
> while (hw_has_space() && (pkt = ieee80211_tx_dequeue(hw, txq)))
> push_to_hw(pkt);
>
> ieee80211_schedule_txq(txq);
> }
> }
Right, we could do that in iwlwifi.
> > > +static bool ieee80211_txq_check_deficit(struct ieee80211_local *local,
> > > + struct txq_info *txqi)
> >
> > Maybe this should be called "has_deficit" or so - the polarity isn't
> > immediately clear to me.
>
> Yup, I suck at naming; gotcha ;)
Common problem :-)
> The move to the end for check_deficit() is what replenishes the airtime
> deficit and ensures fairness.
Ok, yeah, I guess I was reading mostly the first part ("does this have a
deficit") vs. the action on it.
> So that can loop several times in a single
> call to the function.
That I don't understand, check_deficit() doesn't contain any loops? The
caller might, via "goto begin", but not sure what you're saying.
> The schedule_round counter is there to prevent the
> driver from looping indefinitely when doing `while (txq =
> ieee80211_next_txq())`. We'd generally want the deficit replenish to
> happen in any case; previously, the schedule_round type check was in the
> driver; moving it here is an attempt at simplifying the logic needed in
> the driver when using the API.
Right.
Anyway, maybe the naming should be different than "has_deficit" since
that would seem to imply some sort of "checker function" only.
Maybe the most readable thing would be to have an enum return value,
then the function name matters less.
> > Manipulating the list twice like this here seems slightly odd ...
> > perhaps move the list manipulation out of txq_check_deficit() entirely?
> > Clearly it's logically fine, but seems harder than necessary to
> > understand.
> >
> > Also, if the check_deficit() function just returns a value, the renaming
> > I suggested is easier to accept :)
>
> Yeah, maybe; it'll result in some duplication between next_txq() and
> txq_may_transmit() (to the point where I'm not sure if the separate
> check_deficit() function is really needed anymore), but it may be that
> that is actually easier to understand?
Well to be honest ... I hadn't even fully read check_deficit(), but the
naming didn't make it very clear what the contract between it and the
caller is. That's all I'm saying. As long as we clear up that contract a
bit, everything is fine with me.
What you're saying is that has_deficit() doesn't really work since it
actually does something in a few rounds there.
I guess I also got thrown off by "check" since that (to me) implies it's
just checked. Perhaps "adjust_deficit" would be better, and then you
could reverse polarity so that returning true indicates that something
was modified?
johannes
Re: [PATCH RFC v3 0/4] Move TXQ scheduling into mac80211
On Mon, 2018-09-10 at 13:16 +0200, Toke Høiland-Jørgensen wrote: > > > - I didn't get rid of the register_airtime() callback. As far as I can > > > tell, > > > only iwlwifi uses the tx_time field in the struct tx_info. Which means > > > that > > > we *could* probably use it for this and just make the other drivers set > > > it; > > > but I'm not sure what effects that would have in relation to WMM-AC for > > > those drivers, so I chickened out. Will have to try it out, I guess; > > > but it > > > also depends on whether ath10k needs to be able to report airtime > > > asynchronously anyway. So I'll hold off on that for a bit more. > > > > I don't think you need to be concerned, the reporting through this has > > no immediate effect as the driver would also have to set the feature > > flag (NL80211_FEATURE_SUPPORTS_WMM_ADMISSION) for userspace to be able > > to use WMM admission TSPECs, and getting tx_tspec->admitted_time to be > > non-zero in ieee80211_sta_tx_wmm_ac_notify(). > > Great! In that case I'll try moving the reporting go through the tx_info > struct and check that it works for ath9k :) I'm not sure it would work for everyone (though I guess it should for ath9k), so it may well be necessary to report it out-of-band from frames. My objection wasn't so much to the out-of-band mechanism, but to the fact that we have two reporting mechanisms that are each tied to one feature, while reporting the same thing. So if you figure out that you need to keep the reporting out-of-band I'm perfectly fine with that, but I guess I'd argue the two should cross over: reporting in-band should feed back to this, reporting out-of-band should feed the admission mechanism. johannes
Re: [PATCH RFC v3 0/4] Move TXQ scheduling into mac80211
On Mon, 2018-09-10 at 13:17 +0200, Toke Høiland-Jørgensen wrote: > > > - I did not add any locking around next_txq(); the driver is still > > > supposed > > > to maintain a lock that prevents two threads from trying to schedule the > > > same AC at the same time. This is what drivers already do, so I figured > > > it > > > was easier to just keep it that way rather than do it in mac80211. > > > > I'll look at this in the code, but from a maintainer perspective I'm > > somewhat worried that this will lead to issues that are really the > > driver's fault, but surface in mac80211. I don't know how easy it > > would be to catch that. > > Yeah, I get what you mean. The alternative would be to have a > ieee80211_start_schedule(ac) and ieee80211_end_schedule(ac), which > basically just takes a lock. And I guess start would increment the schedule number, which is now dependent on first > Would mean we could get rid of the 'first' > parameter for next_txq(), so might not be such a bad idea; Right, that's what I meant. > and if the > driver has its own locking the extra locking in mac80211 would just be > an always-uncontested spinlock, which shouldn't be much overhead, right? It may still bounce around CPUs if you call this from other places, but I suspect that wouldn't be the biggest issue. There are a lot of calculations going on too... johannes
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
Johannes Berg writes: > On Mon, 2018-09-10 at 12:57 +0200, Toke Høiland-Jørgensen wrote: >> Johannes Berg writes: >> >> > On Sat, 2018-09-08 at 00:22 +0200, Toke Høiland-Jørgensen wrote: >> > > >> > > Usage of the new API is optional, so drivers can be ported one at a time. >> > >> > With the 1:1 hardware queue/txq mapping in iwlwifi (we're close to >> > getting that patch in, though now the Jewish holidays mean a delay), >> > I'm not sure we'd be able to do this at all in iwlwifi. So this may >> > not be a case of porting one at a time until we can get rid of it ... >> >> Could you elaborate a bit more on how the hwq/txq stuff works in iwl? > > I can try. > >> Does the driver just hand off a TXQ to the hardware on wake_txq(), which >> is then scheduled by the hardware after that? Or how does the mapping to >> hwqs work, and how is the hardware notified that there are still packets >> queued / that new packets have arrived for an already mapped txq? > > Basically, we use the TXQ driver data to do this. On initialization, we > set all TXQs to have no hardware queue mapped. Then, when the first wake > happens for this TXQ, we allocate a hardware queue for this RA/TID. > > From then on, right now we basically just try to refill the hardware > queue from the TXQ whenever the hardware releases frames from that > queue. If there are none, we fall back to putting them on the hardware > queue from the wake signal. OK. So if the TXQ is ineligible to dequeue packets, but still have them queued, there would need to be a signal (such as wake_txq()) when it becomes eligible again, right? -Toke
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
On Mon, 2018-09-10 at 14:39 +0200, Toke Høiland-Jørgensen wrote: > > From then on, right now we basically just try to refill the hardware > > queue from the TXQ whenever the hardware releases frames from that > > queue. If there are none, we fall back to putting them on the hardware > > queue from the wake signal. > > OK. So if the TXQ is ineligible to dequeue packets, but still have them > queued, there would need to be a signal (such as wake_txq()) when it > becomes eligible again, right? Right. It wouldn't have to be wake_txq, but that seems as appropriate as anything else. If we were to use ieee80211_airtime_may_transmit(), we'd have to have something that tells us "I previously told you it may not transmit, but now I changed my mind" :-) johannes
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
Johannes Berg writes: > On Mon, 2018-09-10 at 14:39 +0200, Toke Høiland-Jørgensen wrote: > >> > From then on, right now we basically just try to refill the hardware >> > queue from the TXQ whenever the hardware releases frames from that >> > queue. If there are none, we fall back to putting them on the hardware >> > queue from the wake signal. >> >> OK. So if the TXQ is ineligible to dequeue packets, but still have them >> queued, there would need to be a signal (such as wake_txq()) when it >> becomes eligible again, right? > > Right. It wouldn't have to be wake_txq, but that seems as appropriate as > anything else. > > If we were to use ieee80211_airtime_may_transmit(), we'd have to have > something that tells us "I previously told you it may not transmit, but > now I changed my mind" :-) Yes, exactly. Hmm, I'll see what I can come up with :) -Toke
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
On Mon, 2018-09-10 at 15:08 +0200, Toke Høiland-Jørgensen wrote: > Johannes Berg writes: > > > On Mon, 2018-09-10 at 14:39 +0200, Toke Høiland-Jørgensen wrote: > > > > > > From then on, right now we basically just try to refill the hardware > > > > queue from the TXQ whenever the hardware releases frames from that > > > > queue. If there are none, we fall back to putting them on the hardware > > > > queue from the wake signal. > > > > > > OK. So if the TXQ is ineligible to dequeue packets, but still have them > > > queued, there would need to be a signal (such as wake_txq()) when it > > > becomes eligible again, right? > > > > Right. It wouldn't have to be wake_txq, but that seems as appropriate as > > anything else. > > > > If we were to use ieee80211_airtime_may_transmit(), we'd have to have > > something that tells us "I previously told you it may not transmit, but > > now I changed my mind" :-) > > Yes, exactly. Hmm, I'll see what I can come up with :) No need to look at this right now - let's get this stuff sorted out first :) johannes
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
Johannes Berg writes:
> On Mon, 2018-09-10 at 15:08 +0200, Toke Høiland-Jørgensen wrote:
>> Johannes Berg writes:
>>
>> > On Mon, 2018-09-10 at 14:39 +0200, Toke Høiland-Jørgensen wrote:
>> >
>> > > > From then on, right now we basically just try to refill the hardware
>> > > > queue from the TXQ whenever the hardware releases frames from that
>> > > > queue. If there are none, we fall back to putting them on the hardware
>> > > > queue from the wake signal.
>> > >
>> > > OK. So if the TXQ is ineligible to dequeue packets, but still have them
>> > > queued, there would need to be a signal (such as wake_txq()) when it
>> > > becomes eligible again, right?
>> >
>> > Right. It wouldn't have to be wake_txq, but that seems as appropriate as
>> > anything else.
>> >
>> > If we were to use ieee80211_airtime_may_transmit(), we'd have to have
>> > something that tells us "I previously told you it may not transmit, but
>> > now I changed my mind" :-)
>>
>> Yes, exactly. Hmm, I'll see what I can come up with :)
>
> No need to look at this right now - let's get this stuff sorted out
> first :)
Right, sure, I'll get the port of ath9k done first; but doesn't hurt to
think about API compatibility with the other drivers at the same time as
well...
If we have the start_schedule() / end_schedule() pair anyway, the latter
could notify any TXQs that became eligible during the scheduling round.
Also, instead of having the three different API functions
(next_txq()/may_tx()/schedule_txq()), we could have get_txq(txq)/put_txq(txq)
which would always need to be paired; but the argument to get_txq()
could be optional, and if the driver passes NULL it means "give me the
next available TXQ".
So for ath9k it would be:
start_schedule(ac);
while ((txq = get_txq(NULL)) {
queue_aggregate(txq);
put_txq(txq);
}
end_schedule(ac);
And for ath10k/iwlwifi it would be:
on_hw_notify(txq) {
start_schedule(ac);
if (txq = get_txq(txq)) {
queue_packets(txq);
put_txq(txq);
}
end_schedule(ac);
}
I think that would be simpler, API-wise?
-Toke
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
On Mon, 2018-09-10 at 15:18 +0200, Toke Høiland-Jørgensen wrote:
> If we have the start_schedule() / end_schedule() pair anyway, the latter
> could notify any TXQs that became eligible during the scheduling round.
Do we even need end_schedule()? It's hard to pass multiple things to a
single call (do you build a list?), so having
start_schedule(), get_txq(), return_txq()
would be sufficient?
> Also, instead of having the three different API functions
> (next_txq()/may_tx()/schedule_txq()), we could have get_txq(txq)/put_txq(txq)
> which would always need to be paired; but the argument to get_txq()
> could be optional, and if the driver passes NULL it means "give me the
> next available TXQ".
I can't say I like this. It makes the meaning totally different:
* with NULL: use the internal scheduler to determine which one is good
to use next
* non-NULL: essentially equivalent to may_tx()
> So for ath9k it would be:
>
>
> start_schedule(ac);
> while ((txq = get_txq(NULL)) {
> queue_aggregate(txq);
> put_txq(txq);
> }
> end_schedule(ac);
>
> And for ath10k/iwlwifi it would be:
>
> on_hw_notify(txq) {
> start_schedule(ac);
> if (txq = get_txq(txq)) {
>queue_packets(txq);
>put_txq(txq);
> }
> end_schedule(ac);
> }
>
>
> I think that would be simpler, API-wise?
I can't say I see much point in overloading get_txq() that way. You'd
never use it the same way.
Also, would you really start_schedule(ac) in the hw-managed case? It
seems like not? Basically it seems to me that in the hw-managed case all
you need is may_tx()? And in fact, once you opt in you don't even really
need *that* since mac80211 can just return NULL from get_skb()?
johannes
Re: [PATCH RFC v3 1/4] mac80211: Add TXQ scheduling API
Johannes Berg writes:
> On Mon, 2018-09-10 at 15:18 +0200, Toke Høiland-Jørgensen wrote:
>
>> If we have the start_schedule() / end_schedule() pair anyway, the latter
>> could notify any TXQs that became eligible during the scheduling round.
>
> Do we even need end_schedule()? It's hard to pass multiple things to a
> single call (do you build a list?), so having
>
> start_schedule(), get_txq(), return_txq()
>
> would be sufficient?
Well, start_schedule() / end_schedule() would be needed if we are going
to add locking in mac80211?
>> Also, instead of having the three different API functions
>> (next_txq()/may_tx()/schedule_txq()), we could have
>> get_txq(txq)/put_txq(txq)
>> which would always need to be paired; but the argument to get_txq()
>> could be optional, and if the driver passes NULL it means "give me the
>> next available TXQ".
>
> I can't say I like this. It makes the meaning totally different:
>
> * with NULL: use the internal scheduler to determine which one is good
>to use next
> * non-NULL: essentially equivalent to may_tx()
Yeah, it'll be two completely different uses for the same function; but
there wouldn't be two different APIs to keep track of. I'm fine with
keeping them as separately named functions. :)
>> So for ath9k it would be:
>>
>>
>> start_schedule(ac);
>> while ((txq = get_txq(NULL)) {
>> queue_aggregate(txq);
>> put_txq(txq);
>> }
>> end_schedule(ac);
>>
>> And for ath10k/iwlwifi it would be:
>>
>> on_hw_notify(txq) {
>> start_schedule(ac);
>> if (txq = get_txq(txq)) {
>>queue_packets(txq);
>>put_txq(txq);
>> }
>> end_schedule(ac);
>> }
>>
>>
>> I think that would be simpler, API-wise?
>
> I can't say I see much point in overloading get_txq() that way. You'd
> never use it the same way.
>
> Also, would you really start_schedule(ac) in the hw-managed case?
If we decide mac80211 needs to do locking to prevent two threads from
scheduling the same ac, that would also be needed for the hw-managed
case?
> It seems like not? Basically it seems to me that in the hw-managed
> case all you need is may_tx()? And in fact, once you opt in you don't
> even really need *that* since mac80211 can just return NULL from
> get_skb()?
Yeah, we could just throttle in get_skb(); the separate call was to
avoid the overhead of the check for every packet. Typically, you'll pick
a TXQ, then dequeue multiple packets from it in succession; with a
separate call to may_tx(), you only do the check once, not for every
packet...
-Toke
desktop hard lock up (kernel panic) with Alfa AWUSB036ACM and wireless-testing 72d1e548defbf
Start wpa_supplicant Attach to a Cisco AP (haven't tried too many other APs yet). Desktop locks up hard. I have some shell scripts I use start wpa_supp and run wpa_cli (I'm not going through the GNOME ui or other). (Occurs to me I need to verify I didn't have the system's wpa_supplicant running... I forget to kill the systemd started wpa_supp before starting mine.) Captured a kernel oops (attached) with netconsole. Same kernel with my 2nd wifi (an Intel 8265 PCI-e card) works. Running: git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless- testing.git 72d1e548defbf75c6bb42fc99276cd88fda7dda0 I can 'iw dev scan' & scan through wpa_supplicant with the card fairly successfully. I haven't tried monitor mode. Have recreated the oops a couple times. Alfa AWUSB036ACM Bus 001 Device 004: ID 0e8d:7612 MediaTek Inc. Device Descriptor: bLength18 bDescriptorType 1 bcdUSB 2.10 bDeviceClass0 bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize064 idVendor 0x0e8d MediaTek Inc. idProduct 0x7612 bcdDevice1.00 iManufacturer 2 MediaTek Inc. iProduct3 Wireless iSerial 4 0 phy#0 Interface wlp0s20f0u1u2 ifindex 5 wdev 0x1 addr 00:c0:ca:95:78:45 type managed txpower 24.00 dBm phy#1 Unnamed/non-netdev interface wdev 0x10002 addr 60:f6:77:b0:21:e3 type P2P-device txpower 0.00 dBm Interface wlp2s0 ifindex 4 wdev 0x10001 addr 60:f6:77:b0:21:e2 type managed txpower 0.00 dBm [ 546.841947] wlp0s20f0u1u2: authenticate with c4:b9:cd:dc:48:40 [ 547.140202] wlp0s20f0u1u2: send auth to c4:b9:cd:dc:48:40 (try 1/3) [ 547.140226] BUG: unable to handle kernel NULL pointer dereference at 0011 [ 547.140228] PGD 80083e025067 P4D 80083e025067 PUD 83e072067 PMD 0 [ 547.140233] Oops: [#1] SMP PTI [ 547.140236] CPU: 2 PID: 2503 Comm: wpa_supplicant Not tainted 4.19.0-rc2-wt #1 [ 547.140238] Hardware name: Shuttle Inc. SZ170/FZ170, BIOS 2.09 08/01/2017 [ 547.140258] RIP: 0010:ieee80211_wake_txqs+0x1e3/0x3d0 [mac80211] [ 547.140261] Code: 4c 89 fe 4c 89 ef 48 8b 92 b0 02 00 00 e8 45 d6 2e f4 48 8b 3c 24 e8 0c bc 00 f4 48 83 c5 08 48 3b 6c 24 08 74 a0 4c 8b 7d 00 <41> 0f b6 57 11 3b 54 24 18 75 e6 4d 8d a7 28 ff ff ff f0 49 0f ba [ 547.140263] RSP: 0018:95038ea83ed0 EFLAGS: 00010293 [ 547.140265] RAX: 95038419e978 RBX: 95038419e000 RCX: 950384b18760 [ 547.140267] RDX: RSI: RDI: 950384b18828 [ 547.140269] RBP: 95038419e970 R08: 00039e862fcdb16e R09: 95038a9b4230 [ 547.140271] R10: 0420 R11: b8efc46c78d0 R12: 9503798251d0 [ 547.140273] R13: 950384b18760 R14: R15: [ 547.140275] FS: 7f42ed60cdc0() GS:95038ea8() knlGS: [ 547.140277] CS: 0010 DS: ES: CR0: 80050033 [ 547.140279] CR2: 0011 CR3: 000835b30001 CR4: 003606e0 [ 547.140281] DR0: DR1: DR2: [ 547.140283] DR3: DR6: fffe0ff0 DR7: 0400 [ 547.140284] Call Trace: [ 547.140287] [ 547.140293] tasklet_action_common.isra.17+0x5a/0x100 [ 547.140297] __do_softirq+0xe4/0x2d9 [ 547.140300] do_softirq_own_stack+0x2a/0x40 [ 547.140302] [ 547.140305] do_softirq.part.15+0x4f/0x60 [ 547.140307] __local_bh_enable_ip+0x4b/0x50 [ 547.140322] ieee80211_auth.cold.39+0xec/0x127 [mac80211] [ 547.140336] ieee80211_mgd_auth.cold.49+0xfc/0x126 [mac80211] [ 547.140354] cfg80211_mlme_auth+0x125/0x230 [cfg80211] [ 547.140368] nl80211_authenticate+0x2f1/0x360 [cfg80211] [ 547.140373] genl_family_rcv_msg+0x1ca/0x3a0 [ 547.140377] ? schedule+0x28/0x80 [ 547.140380] ? schedule_preempt_disabled+0xa/0x10 [ 547.140382] ? __mutex_lock.isra.5+0x1be/0x490 [ 547.140385] genl_rcv_msg+0x47/0x8c [ 547.140389] ? __kmalloc_node_track_caller+0x1df/0x2a0 [ 547.140391] ? genl_family_rcv_msg+0x3a0/0x3a0 [ 547.140394] netlink_rcv_skb+0x4c/0x120 [ 547.140397] genl_rcv+0x24/0x40 [ 547.140399] netlink_unicast+0x19e/0x260 [ 547.140401] netlink_sendmsg+0x1ff/0x3c0 [ 547.140405] sock_sendmsg+0x36/0x40 [ 547.140408] ___sys_sendmsg+0x295/0x2f0 [ 547.140412] ? __wake_up_common_lock+0x89/0xc0 [ 547.140415] ? tty_write+0x1e8/0x300 [ 547.140418] ? process_echoes+0x60/0x60 [ 547.140421] __sys_sendmsg+0x57/0xa0 [ 547.140425] do_syscall_64+0x5b/0x160 [ 547.140428] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 547.140431] RIP: 0033:0x7f42ec2412b4 [ 547.140434] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b5 0f 1f 80
Re: desktop hard lock up (kernel panic) with Alfa AWUSB036ACM and wireless-testing 72d1e548defbf
> > Start wpa_supplicant > Attach to a Cisco AP (haven't tried too many other APs yet). Desktop > locks up hard. I have some shell scripts I use start wpa_supp and run > wpa_cli (I'm not going through the GNOME ui or other). > > (Occurs to me I need to verify I didn't have the system's > wpa_supplicant running... I forget to kill the systemd started wpa_supp > before starting mine.) > > Captured a kernel oops (attached) with netconsole. Same kernel with my > 2nd wifi (an Intel 8265 PCI-e card) works. > > Running: > git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless- > testing.git 72d1e548defbf75c6bb42fc99276cd88fda7dda0 > > > I can 'iw dev scan' & scan through wpa_supplicant with the card fairly > successfully. I haven't tried monitor mode. Have recreated the oops a > couple times. > > > Alfa AWUSB036ACM > Hi David, according to the kernel log you provided the crash occurs in ieee80211_wake_txqs(). Could you please run gdb in order to identify the point where NULL pointer dereference occurs? E.g: $gdb mac80211.ko $info address ieee80211_wake_txqs $l*(+) offset is 0x1e3 in your log Regards, Lorenzo
Re: desktop hard lock up (kernel panic) with Alfa AWUSB036ACM and wireless-testing 72d1e548defbf
On Mon, 2018-09-10 at 19:27 +0200, Lorenzo Bianconi wrote:
> >
> > Start wpa_supplicant
> > Attach to a Cisco AP (haven't tried too many other APs yet).
> > Desktop
> > locks up hard. I have some shell scripts I use start wpa_supp and
> > run
> > wpa_cli (I'm not going through the GNOME ui or other).
> >
> > (Occurs to me I need to verify I didn't have the system's
> > wpa_supplicant running... I forget to kill the systemd started
> > wpa_supp
> > before starting mine.)
> >
> > Captured a kernel oops (attached) with netconsole. Same kernel with
> > my
> > 2nd wifi (an Intel 8265 PCI-e card) works.
> >
> > Running:
> > git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-
> > testing.git 72d1e548defbf75c6bb42fc99276cd88fda7dda0
> >
> >
> > I can 'iw dev scan' & scan through wpa_supplicant with the card
> > fairly
> > successfully. I haven't tried monitor mode. Have recreated the oops
> > a
> > couple times.
> >
> >
> > Alfa AWUSB036ACM
> >
>
> Hi David,
>
> according to the kernel log you provided the crash occurs in
> ieee80211_wake_txqs().
> Could you please run gdb in order to identify the point where NULL
> pointer dereference occurs? E.g:
>
> $gdb mac80211.ko
> $info address ieee80211_wake_txqs
> $l*(+)
> offset is 0x1e3 in your log
>
> Regards,
> Lorenzo
>
Hello. Here's the requested info.
...trillian:wireless-testing% gdb ./net/mac80211/mac80211.ko
[...]
(gdb) info address ieee80211_wake_txqs
Symbol "ieee80211_wake_txqs" is a function at address 0x35890.
[...]
(gdb) l*(0x35890+0x1e3)
0x35a73 is in ieee80211_wake_txqs (net/mac80211/util.c:269).
264 for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++)
{
265 struct ieee80211_txq *txq = sta-
>sta.txq[i];
266
267 txqi = to_txq_info(txq);
268
269 if (ac != txq->ac)
270 continue;
271
272 if
(!test_and_clear_bit(IEEE80211_TXQ_STOP_NETIF_TX,
273 &txqi->flags))
smime.p7s
Description: S/MIME cryptographic signature
Re: Troubleshooting compilation of hostapd
Hi Rami, When I delete the project's directory and I clone the project again and I compile it with uncommented parameter "CONFIG_LIBNL32=y" then the compilation is successful. Thanks for the advice. HI Pavlin, Please try from scratch cloning the tree from scratch, then cp defconfig .config, then uncomment the following entry in .config: CONFIG_LIBNL32=y and then run "make", and post here the results. Regards, Rami Rosen http://ramirose.wixsite.com/ramirosen
