On 29.01.25 08:55, Johannes Berg wrote:
...
+static void ieee80211_vif_txq_init(struct ieee80211_sub_if_data *sdata,
+ int txq_offset, int txq_size, int num_queues)
+{
+ void *buffer = (char *)sdata + txq_offset;
That seems ... awkward at best, my first instinct would be to say this
must be wrong.
Do we really need to do this dynamically just to save two a bit of
memory for VLAN and (virtual) monitor interfaces?
Would probably be better to use pointers to a TXQ array instead of
embedding them then, or otherwise just embed enough for all kinds of
interfaces and just not use them.
Allocating all TXQs always should be trivial. I just must find all the
parts which care about that. __ieee80211_wake_txqs() e.q would try to
wake the useless queues otherwise anyhow.
But I don't see a obvious simple way to avoid the awkward
initialization. Just a complicated one...
Problem is, that the txqi's are allocated with the netdev priv area,
behind sdata.
With the existing code that's just one offset:
int size = ALIGN(sizeof(*sdata) + local>hw.vif_data_size,
sizeof(void *));
...
sdata = netdev_priv(ndev);
...
if (txq_size) {
txqi = netdev_priv(ndev) + size;
ieee80211_txq_init(sdata, NULL, txqi, 0);
The patch "repeats" that for the new TXQs.
Since txqi size depends on local->hw.txq_data_size (for driver private
data) I don't see how that can be squeezed in a static struct or type.
So the only way I can think of to avoid something like
void *buffer = (char *)sdata + txq_offset;
would require to allocate each txqi independently with kzalloc() outside
of netdev private.
Is there maybe a simpler solution I'm missing here or is that what we
should do here?
...
+static void __ieee80211_wake_txq(struct ieee80211_local *local,
+ struct ieee80211_txq *txq)
+{
+ struct txq_info *txqi = to_txq_info(txq);
+ struct fq *fq = &local->fq;
+
+ if (WARN_ON(!txq))
+ return;
+ if (test_and_clear_bit(IEEE80211_TXQ_DIRTY, &txqi->flags)) {
+ spin_unlock(&fq->lock);
+ drv_wake_tx_queue(local, txqi);
+ spin_lock(&fq->lock);
+ }
+}
I really don't like dropping locks in the middle - if that's *really*
needed there better be a very good reason and a good comment indicating
why it's safe, including a comment in the caller that says this happens.
Sure, I'll add some comments here.
But that's nothing new, just restructuring the existing
__ieee80211_wake_txqs():
if (!test_and_clear_bit(IEEE80211_TXQ_DIRTY,
&txqi->flags))
continue;
spin_unlock(&fq->lock);
drv_wake_tx_queue(local, txqi);
spin_lock(&fq->lock);
We can't hold fq->lock when calling drv_wake_tx_queue since since
ieee80211_tx_dequeue() takes it, too.
And I assume the spinlock in __ieee80211_wake_txqs() is required to
make sure nobody will mark a queue as dirty while we are starting it and
thus end up with a stuck queue.
+ /* %IEEE80211_VIF_TXQ_NOQUEUE must be ignored here */
No need for % outside kernel-doc.
+
+ if (ac == IEEE80211_AC_VO) {
+ __ieee80211_wake_txq(local,
+ vif->txq[IEEE80211_VIF_TXQ_FALLBACK]);
+ }
nit: no need for braces
Why is VO special, not sure I understand the logic of checking the AC?
Guess that needs a comment, too:-)
This is not about that VO is somehow special. This is my decision to
handle frames in the VIF fallback queue with VO priority.
The patch added fallback TXQs per-vif and per-sta. While we can ignore
the new "noqueue" TXQs here - which is always directly TX a packet or
drop it - the fallback queue can get dirty and must be started after
that again.
The existing code is already something similar for the Multicast TXQ,
just BE for that.
+ if (ac == IEEE80211_AC_BE && vif->txq[IEEE80211_VIF_TXQ_MULTICAST] &&
+ (!ps || !atomic_read(&ps->num_sta_ps)))
+ __ieee80211_wake_txq(local,
+ vif->txq[IEEE80211_VIF_TXQ_MULTICAST]);
Also here, why BE?
And here is the "existing" code mentioned above. It basically was moved
up from the end of __ieee80211_wake_txqs() and changed to work here.
(I moved it since once PS starts using this TXQ we better send out the
queued frames here directly after the beacon and don't serve individual
stations first.)
The old code was:
if (!test_and_clear_bit(IEEE80211_TXQ_DIRTY, &txqi->flags) ||
(ps && atomic_read(&ps->num_sta_ps)) || ac != vif->txq->ac)
goto out;
spin_unlock(&fq->lock);
drv_wake_tx_queue(local, txqi);
local_bh_enable();
return;
The old code was just using "ac != vif->txq->ac" to make sure we are
working on BE. (The Multicast queue is set to IEEE80211_AC_BE in
ieee80211_txq_init()).
I could always check the Fallback and Multicast TXQs if as matches to
the current run. Just seems to be wasteful when these are fixed. Of
course when someone decides to change the ac in ieee80211_txq_init() and
not here they still run based on the old settings.
I'll keep the code but add some comments here, too.
list_for_each_entry_rcu(sta, &local->sta_list, list) {
if (sdata != sta->sdata)
continue;
- for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
+ /* IEEE80211_TXQ_NOQUEUE must be ignored here. */
some of these comments should probably say _why_ too
I'll update the comments to make it clear that these "noqueue" queues
can't become dirty and thus don't need a wake.
Alexander
