Re: [PATCHv2] mac80211: add stop/start logic for software TXQs

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Manikanta Pubbisetty <mpubbise@codeaurora.org> writes:

> On 7/10/2018 6:28 PM, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>
>>> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
>>> index 172aeae..d07f7f9 100644
>>> --- a/net/mac80211/ieee80211_i.h
>>> +++ b/net/mac80211/ieee80211_i.h
>>> @@ -818,6 +818,7 @@ enum txq_info_flags {
>>>   	IEEE80211_TXQ_STOP,
>>>   	IEEE80211_TXQ_AMPDU,
>>>   	IEEE80211_TXQ_NO_AMSDU,
>>> +	IEEE80211_TXQ_PAUSED,
>>>   };
>> I think it would be a good idea to either rename the flags, or at least
>> add an explanation somewhere of the difference between a paused and a
>> stopped queue...
>
> Initially, the idea was to use IEEE80211_TXQ_STOP flag to indicate that=20
> iTXQs are stopped; since this flag was used in the aggregation code, I=20
> was unsure whether the same flag can be used to indicate the iTXQ stop=20
> condition.
> I could not find any better name for this:-).

Hmm, yeah, not sure whether the two code paths can stomp on each other
if you reuse the flag. It would be neat to be able to reuse it, though...

Otherwise, how about renaming the old one to _STOP_AGGR and calling the
new one _STOP_NETIF or something?

>>>   /**
>>> @@ -1226,6 +1227,7 @@ struct ieee80211_local {
>>>=20=20=20
>>>   	struct sk_buff_head pending[IEEE80211_MAX_QUEUES];
>>>   	struct tasklet_struct tx_pending_tasklet;
>>> +	struct tasklet_struct wake_txqs_tasklet;
>> It's not quite clear to me why a tasklet is needed? Couldn't you just
>> call the ieee80211_wake_txqs() function at the same place where you
>> currently schedule the tasklet?
>
> Since driver can also invoke wake_queues() operation; there can be a=20
> possible deadlock situation. At least, in ath10k there was deadlock on=20
> the "htt->tx_lock"; wake_queues() is invoked by taking the tx_lock and=20
> the same lock is acquired again in wake_tx_queue().
> Also, by deferring wake_txqs(), drivers can safely invoke wake_queues().

Right, that makes sense :)

-Toke