rt2x00: Serialize TX operations on a queue.

PATCH Helmut Schaa 3 participants 4 messages May 30, 2011 3 messages not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Helmut Schaa

author

2011-05-30 07:57 UTC #3 lore raw

[CC'ed Johannes & linux-wireless]

Am Sonntag, 29. Mai 2011 schrieb Gertjan van Wingerde:

Quoted 33 lines · Helmut Schaa

Quoted 32 lines
Am Samstag, 28. Mai 2011 schrieb Gertjan van Wingerde:
Quoted 23 lines
Quoted 14 lines
Quoted 14 lines
drivers/net/wireless/rt2x00/rt2x00queue.c +2 -1
index f1e1381..2ace0f9 100644
@@ -555,15 +555,21 @@ int rt2x00queue_write_tx_frame(struct data_queue *queue, struct sk_buff *skb,
 			       bool local)
 {
 	struct ieee80211_tx_info *tx_info;
-	struct queue_entry *entry = rt2x00queue_get_entry(queue, Q_INDEX);
+	struct queue_entry *entry;
 	struct txentry_desc txdesc;
 	struct skb_frame_desc *skbdesc;
 	u8 rate_idx, rate_flags;
+	int ret = 0;
Quoted 8 lines
drivers/net/wireless/rt2x00/rt2x00queue.c int rt2x00queue_write_tx_frame(struct data_queue *queue, struct sk_buff *skb, · line 565
Quoted 3 lines
drivers/net/wireless/rt2x00/rt2x00queue.c +3 -0
+
+	spin_lock_bh(&queue->tx_lock);
+
Shouldn't spin_lock be enough already? All tx calls from mac80211 are already done in softirq context or protected by local_bh_disable/enable. This would be especially useful on single CPU machines since spin_lock (without _bh) can be optimized out on these.
Quoted 4 lines
AFAICT not all tx calls from mac80211 are protected by local_bh_disable/enable. I believe that this is the case for the "process" context tx calls from
ieee80211_subif_start_xmit
This should only be called from the network softirq and thus doesn't require bottom halves to be disabled (AFAIK).

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
and
ieee80211_monitor_start_xmit.
Same here.
Hmm, are you sure about this?

Maybe Johannes can clarify this for us ;)

Johannes, are all calls to the drivers tx callback done in softirq context or adequately protected by local_bh_disable/enable? Or is this presumption wrong?

Thanks,
Helmut

Johannes Berg

2011-05-30 09:21 UTC #4 lore raw

Quoted 8 lines · Helmut Schaa

Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
AFAICT not all tx calls from mac80211 are protected by local_bh_disable/enable. I believe that this is the case for the "process" context tx calls from
ieee80211_subif_start_xmit
This should only be called from the network softirq and thus doesn't require bottom halves to be disabled (AFAIK).

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
and
ieee80211_monitor_start_xmit.
Same here.
Hmm, are you sure about this?
Maybe Johannes can clarify this for us ;)

The above assertion ("not all TX calls are protected") is wrong.

Quoted 2 lines

Johannes, are all calls to the drivers tx callback done in softirq context or adequately protected by local_bh_disable/enable? Or is this presumption wrong?

All of them are invoked with BHs disabled.

Cf. ieee80211_tx_skb(), which is the only function that can be called from process context. The _start_xmit functions listed above are called by the networking layer and always invoked with BHs disabled.

johannes

Gertjan van Wingerde

2011-05-30 09:32 UTC #5 lore raw

Quoted 21 lines · Johannes Berg

Quoted 8 lines · Helmut Schaa
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
AFAICT not all tx calls from mac80211 are protected by local_bh_disable/enable. I believe that this is the case for the "process" context tx calls from
ieee80211_subif_start_xmit
This should only be called from the network softirq and thus doesn't require bottom halves to be disabled (AFAIK).
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
and
ieee80211_monitor_start_xmit.
Same here.
Hmm, are you sure about this?
Maybe Johannes can clarify this for us ;)
The above assertion ("not all TX calls are protected") is wrong.

Quoted 9 lines

Quoted 2 lines
Johannes, are all calls to the drivers tx callback done in softirq context or adequately protected by local_bh_disable/enable? Or is this presumption wrong?
All of them are invoked with BHs disabled.
Cf. ieee80211_tx_skb(), which is the only function that can be called from process context. The _start_xmit functions listed above are called by the networking layer and always invoked with BHs disabled.

OK. Thanks for the clarification. I'll update the patch to use spin_lock / spin_unlock instead
of spin_lock_bh / spin_unlock_bh when I return home on Wednesday.

As a follow-up question, is it valid that mac80211 calls the driver's tx function simultaneously
for multiple frames, or are we experiencing an issue with mac80211 here? We are experiencing in rt2x00 that our tx function is called while it is already running, leading
to races in the queue handling when enqueuing the frame for the hw.

---
Gertjan

Johannes Berg

2011-05-30 09:38 UTC #6 lore raw

Quoted 3 lines · Gertjan van Wingerde

As a follow-up question, is it valid that mac80211 calls the driver's tx function simultaneously
for multiple frames, or are we experiencing an issue with mac80211 here?

That can happen by design, yes.

johannes