Re: [PATCH] mac80211: add an intermediate software queue implementation

2014-12-31 Thread Johan Almbladh 
On Wed, Nov 19, 2014 at 12:14 AM, Felix Fietkau n...@openwrt.org wrote:

 +static void ieee80211_drv_tx(struct ieee80211_local *local,
 +struct ieee80211_vif *vif,
 +struct ieee80211_sta *pubsta,
 +struct sk_buff *skb)
 +{
 +   struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb-data;
 +   struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
 +   struct ieee80211_tx_control control = {
 +   .sta = pubsta
 +   };
 +   struct ieee80211_txq *pubtxq = NULL;
 +   struct txq_info *txq;
 +   u8 ac;
 +
 +   if (ieee80211_is_mgmt(hdr-frame_control) ||
 +   ieee80211_is_ctl(hdr-frame_control))
 +   goto tx_normal;
 +
 +   if (pubsta) {
 +   u8 tid = skb-priority  IEEE80211_QOS_CTL_TID_MASK;
 +   pubtxq = pubsta-txq[tid];
 +   } else {
 +   pubtxq = vif-txq;
 +   }

In the monitor frame injection tx path the vif pointer may actually be
NULL when we get here, see the function __ieee80211_tx(). This causes
a NULL pointer dereference crash. The wperf tool
(https://github.com/anyfi/wperf) can be used to reproduce the crash by
specifying a BSSID that does not belong to a vif.

The following one-line change in the patch fixes the crash for me:

   if (pubsta) {
   u8 tid = skb-priority  IEEE80211_QOS_CTL_TID_MASK;
   pubtxq = pubsta-txq[tid];
- } else {
+ } else if (vif) {
   pubtxq = vif-txq;
   }

Johan
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mac80211: add an intermediate software queue implementation

2014-12-16 Thread Felix Fietkau 
On 2014-12-16 00:25, Bartosz Szczepanek wrote:
 As for drv_wake_tx_queue and ieee80211_tx_dequeue - is it really
 necessary? There are ieee80211_tx_status and ieee80211_free_txskb
 already, which can be used to decide from mac80211 level when to
 dequeue packet. It could be used even in case of drivers that are not
 aware of new mechanism at all. We could compute difference between
 drv_tx and tx_status/free_txskb calls, therefore getting number of
 frames in HW. What could help us to keep queues short.
Keeping the driver queue short is a bad idea if the driver needs deeper
queues to do aggregation.

 I've already written some code. This http://pastebin.com/dSd1zWt7 is
 patch that implements counter of frames in hardware in the way
 described above. It was necessary to differentiate between free_txskb
 and free_txskb. Information about frames in HW is exported to debugfs.
 I thought I could submit it, but just now did I found this thread, so
 I hope that it's adequate place to propose that. I tested it on ath5k
 and brcmsmac.
For aggregation, different drivers need different kinds of scheduling.
Only packets belonging to the same sta/tid can be aggregated, and in AP
mode you can have concurrent traffic of multiple different sta/tid.
The only way to keep queues really short in that case without
sacrificing throughput is to let the aggregation code fetch packets
directly from sta/tid queues.
With ath5k there is no aggregation yet (we could add A-MSDU at some
point), and with brcmsmac, the driver has an internal layer of queueing
to create per-sta/tid queues.
What I'm proposing is having per-sta/tid queues that are shared between
mac80211 and the driver, which will significantly improve queue
management compared to having multiple competing layers of queueing.

 One more thing - why not to use local-pending for holding packets?
 There is tx_pending tasklet already. I'm not sure if I understand the
 idea of local-pending queues correctly, but it seems to be a bit
 incoherent to have both pending and proposed ieee80211_txq.
local-pending is useless for normal tx queueing purposes, because it's
not per-sta/tid.

- Felix
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mac80211: add an intermediate software queue implementation

2014-12-15 Thread Bartosz Szczepanek 
As for drv_wake_tx_queue and ieee80211_tx_dequeue - is it really
necessary? There are ieee80211_tx_status and ieee80211_free_txskb
already, which can be used to decide from mac80211 level when to
dequeue packet. It could be used even in case of drivers that are not
aware of new mechanism at all. We could compute difference between
drv_tx and tx_status/free_txskb calls, therefore getting number of
frames in HW. What could help us to keep queues short.

I've already written some code. This http://pastebin.com/dSd1zWt7 is
patch that implements counter of frames in hardware in the way
described above. It was necessary to differentiate between free_txskb
and free_txskb. Information about frames in HW is exported to debugfs.
I thought I could submit it, but just now did I found this thread, so
I hope that it's adequate place to propose that. I tested it on ath5k
and brcmsmac.

One more thing - why not to use local-pending for holding packets?
There is tx_pending tasklet already. I'm not sure if I understand the
idea of local-pending queues correctly, but it seems to be a bit
incoherent to have both pending and proposed ieee80211_txq.

(It's my first post on linux kernel mailing list. Please, let me know
if I did something wrong.)

Best regards.

2014-12-15 13:00 GMT+01:00 Johannes Berg johan...@sipsolutions.net:
 On Fri, 2014-12-12 at 15:28 +0100, Felix Fietkau wrote:

  Management (and maybe control) frames can have different priorities as
  well, this is only used for something with TDLS now I think though.
 With my implementation, those would go through the normal tx codepath,
 bypassing the software tx queues. Can you think of anything else that
 would need per-AC vif queues?

 Not off the top of my head. I just didn't even quite understand that you
 were still using the normal tx path

 johannes

 --
 To unsubscribe from this list: send the line unsubscribe linux-wireless in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mac80211: add an intermediate software queue implementation

2014-12-12 Thread Johannes Berg 
On Wed, 2014-11-19 at 00:14 +0100, Felix Fietkau wrote:

 + struct txq_info *txq;
 + atomic_t txq_len[IEEE80211_NUM_ACS];

I think you should consider renaming the latter to txqs_len or so - it
doesn't just cover one txq as is be implied by the name now. Otherwise
the skb_queue_head also maintains the length anyway, but I think you
need the aggregate for all stations here...

Some documentation for this and the vif.txq would be good too :)

In fact - it might be worthwhile to take parts of the commit message and
elaborate a bit on it and write a whole DOC: section?

 --- a/net/mac80211/sta_info.h
 +++ b/net/mac80211/sta_info.h
 @@ -371,6 +371,7 @@ struct sta_info {
   struct sk_buff_head ps_tx_buf[IEEE80211_NUM_ACS];
   struct sk_buff_head tx_filtered[IEEE80211_NUM_ACS];
   unsigned long driver_buffered_tids;
 + void *txq;

You can still use struct txq_info * here even when it's not declared yet
(since it's in the other header file)
 
 +static void ieee80211_drv_tx(struct ieee80211_local *local,
 +  struct ieee80211_vif *vif,
 +  struct ieee80211_sta *pubsta,
 +  struct sk_buff *skb)
 +{
 + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb-data;
 + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
 + struct ieee80211_tx_control control = {
 + .sta = pubsta
 + };
 + struct ieee80211_txq *pubtxq = NULL;
 + struct txq_info *txq;
 + u8 ac;
 +
 + if (ieee80211_is_mgmt(hdr-frame_control) ||
 + ieee80211_is_ctl(hdr-frame_control))
 + goto tx_normal;
 +
 + if (pubsta) {
 + u8 tid = skb-priority  IEEE80211_QOS_CTL_TID_MASK;
 + pubtxq = pubsta-txq[tid];
 + } else {
 + pubtxq = vif-txq;
 + }

This is a bit confusing - isn't this the same as sdata-txq.txq? Then
again what even sets vif-txq? Shouldn't those be per-AC? Do you really
want to mix 'normal' and txq-TX?

I think you should also use txqi as variables for txq_info - it gets
cumbersome to distinguish the two everywhere.

Also in many cases where you have txq allocation failures you just
continue as is, I'm not sure that's such a great idea. Those driver
paths will practically never get tested.

 + if (!pubtxq)
 + goto tx_normal;
 +
 + ac = pubtxq-ac;
 + txq = container_of(pubtxq, struct txq_info, txq);
 + atomic_inc(sdata-txq_len[ac]);
 + if (atomic_read(sdata-txq_len[ac]) = local-hw.txq_ac_max_pending)
 + netif_stop_subqueue(sdata-dev, ac);
 +
 + skb_queue_tail(txq-queue, skb);
 + drv_wake_tx_queue(local, txq);

You might consider doing locking differently here - I think you probably
don't need the txq-queue spinlock at all since you're in per-AC and
mappings are static. Not sure how that interacts with other parts of the
code though.

 +int ieee80211_tx_dequeue(struct ieee80211_hw *hw, struct ieee80211_txq 
 *pubtxq,
 +  struct sk_buff **dest)

I'd prefer you return the skb and use ERR_PTR() for errors.

 +void ieee80211_init_tx_queue(struct ieee80211_sub_if_data *sdata,
 +  struct sta_info *sta,
 +  struct txq_info *txq, int tid)
 +{
 + skb_queue_head_init(txq-queue);
 + txq-txq.vif = sdata-vif;
 +
 + if (sta) {
 + txq-txq.sta = sta-sta;
 + sta-sta.txq[tid] = txq-txq;
 + txq-txq.ac = ieee802_1d_to_ac[tid  7];
 + } else {
 + sdata-vif.txq = txq-txq;
 + txq-txq.ac = IEEE80211_AC_BE;
 + }

Again, I don't quite understand the single AC queue here per vif. It
seems it should be one for each AC and possibly one for cab? Or none at
all - I don't really see what this single one would be used for, in the
TX code you seem to use it for mcast data only but then I don't really
see the point. It's also not part of the queue length accounting.

 +void ieee80211_flush_tx_queue(struct ieee80211_local *local,
 +   struct ieee80211_txq *pubtxq)
 +{
 + struct txq_info *txq = container_of(pubtxq, struct txq_info, txq);
 + struct ieee80211_sub_if_data *sdata = vif_to_sdata(pubtxq-vif);
 + struct sk_buff *skb;
 +
 + while ((skb = skb_dequeue(txq-queue)) != NULL) {
 + atomic_dec(sdata-txq_len[pubtxq-ac]);
 + ieee80211_free_txskb(local-hw, skb);
 + }
 +}

You can rewrite this a bit smarter to just do one atomic op.

johannes

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mac80211: add an intermediate software queue implementation

2014-12-12 Thread Felix Fietkau 
On 2014-12-12 14:21, Johannes Berg wrote:
 On Wed, 2014-11-19 at 00:14 +0100, Felix Fietkau wrote:
 
 +struct txq_info *txq;
 +atomic_t txq_len[IEEE80211_NUM_ACS];
 
 I think you should consider renaming the latter to txqs_len or so - it
 doesn't just cover one txq as is be implied by the name now. Otherwise
 the skb_queue_head also maintains the length anyway, but I think you
 need the aggregate for all stations here...
 
 Some documentation for this and the vif.txq would be good too :)
 
 In fact - it might be worthwhile to take parts of the commit message and
 elaborate a bit on it and write a whole DOC: section?
Yeah, makes sense.

 --- a/net/mac80211/sta_info.h
 +++ b/net/mac80211/sta_info.h
 @@ -371,6 +371,7 @@ struct sta_info {
  struct sk_buff_head ps_tx_buf[IEEE80211_NUM_ACS];
  struct sk_buff_head tx_filtered[IEEE80211_NUM_ACS];
  unsigned long driver_buffered_tids;
 +void *txq;
 
 You can still use struct txq_info * here even when it's not declared yet
 (since it's in the other header file)
OK.

 +static void ieee80211_drv_tx(struct ieee80211_local *local,
 + struct ieee80211_vif *vif,
 + struct ieee80211_sta *pubsta,
 + struct sk_buff *skb)
 +{
 +struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb-data;
 +struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
 +struct ieee80211_tx_control control = {
 +.sta = pubsta
 +};
 +struct ieee80211_txq *pubtxq = NULL;
 +struct txq_info *txq;
 +u8 ac;
 +
 +if (ieee80211_is_mgmt(hdr-frame_control) ||
 +ieee80211_is_ctl(hdr-frame_control))
 +goto tx_normal;
 +
 +if (pubsta) {
 +u8 tid = skb-priority  IEEE80211_QOS_CTL_TID_MASK;
 +pubtxq = pubsta-txq[tid];
 +} else {
 +pubtxq = vif-txq;
 +}
 
 This is a bit confusing - isn't this the same as sdata-txq.txq?
Right, I should probably use that.

 Then
 again what even sets vif-txq? Shouldn't those be per-AC? Do you really
 want to mix 'normal' and txq-TX?
Are we even using multiple ACs for packets that don't belong to a
particular sta? I thought normal mcast data frames only use non-QoS
frames. And yes, I'm currently mixing normal and txq-TX to prioritize
ctl/mgmt frames over other less important traffic.

 I think you should also use txqi as variables for txq_info - it gets
 cumbersome to distinguish the two everywhere.
Will do.

 Also in many cases where you have txq allocation failures you just
 continue as is, I'm not sure that's such a great idea. Those driver
 paths will practically never get tested.
It will just do normal tx in that case, which should work.

 +if (!pubtxq)
 +goto tx_normal;
 +
 +ac = pubtxq-ac;
 +txq = container_of(pubtxq, struct txq_info, txq);
 +atomic_inc(sdata-txq_len[ac]);
 +if (atomic_read(sdata-txq_len[ac]) = local-hw.txq_ac_max_pending)
 +netif_stop_subqueue(sdata-dev, ac);
 +
 +skb_queue_tail(txq-queue, skb);
 +drv_wake_tx_queue(local, txq);
 
 You might consider doing locking differently here - I think you probably
 don't need the txq-queue spinlock at all since you're in per-AC and
 mappings are static. Not sure how that interacts with other parts of the
 code though.
I wanted to use the lock to give the driver the freedom to call
ieee80211_tx_dequeue from outside of normal per-AC tx context.

 +int ieee80211_tx_dequeue(struct ieee80211_hw *hw, struct ieee80211_txq 
 *pubtxq,
 + struct sk_buff **dest)
 
 I'd prefer you return the skb and use ERR_PTR() for errors.
Will do

 +void ieee80211_init_tx_queue(struct ieee80211_sub_if_data *sdata,
 + struct sta_info *sta,
 + struct txq_info *txq, int tid)
 +{
 +skb_queue_head_init(txq-queue);
 +txq-txq.vif = sdata-vif;
 +
 +if (sta) {
 +txq-txq.sta = sta-sta;
 +sta-sta.txq[tid] = txq-txq;
 +txq-txq.ac = ieee802_1d_to_ac[tid  7];
 +} else {
 +sdata-vif.txq = txq-txq;
 +txq-txq.ac = IEEE80211_AC_BE;
 +}
 
 Again, I don't quite understand the single AC queue here per vif. It
 seems it should be one for each AC and possibly one for cab? Or none at
 all - I don't really see what this single one would be used for, in the
 TX code you seem to use it for mcast data only but then I don't really
 see the point. It's also not part of the queue length accounting.
I handle CAB through normal mac80211 mcast buffering.

 +void ieee80211_flush_tx_queue(struct ieee80211_local *local,
 +  struct ieee80211_txq *pubtxq)
 +{
 +struct txq_info *txq = container_of(pubtxq, struct txq_info, txq);
 +struct ieee80211_sub_if_data *sdata = vif_to_sdata(pubtxq-vif);
 +struct sk_buff *skb;
 +
 +while ((skb = skb_dequeue(txq-queue)) != NULL) {
 +atomic_dec(sdata-txq_len[pubtxq-ac]);
 +

Re: [PATCH] mac80211: add an intermediate software queue implementation

2014-12-12 Thread Felix Fietkau 
On 2014-12-12 15:01, Johannes Berg wrote:
 On Fri, 2014-12-12 at 14:40 +0100, Felix Fietkau wrote:
 
  Then
  again what even sets vif-txq? Shouldn't those be per-AC? Do you really
  want to mix 'normal' and txq-TX?
 Are we even using multiple ACs for packets that don't belong to a
 particular sta? I thought normal mcast data frames only use non-QoS
 frames. And yes, I'm currently mixing normal and txq-TX to prioritize
 ctl/mgmt frames over other less important traffic.
 
 Management (and maybe control) frames can have different priorities as
 well, this is only used for something with TDLS now I think though.
With my implementation, those would go through the normal tx codepath,
bypassing the software tx queues. Can you think of anything else that
would need per-AC vif queues?

- Felix
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html