* Re: [rt2x00-users] [PATCH 1/3] rt2x00: Serialize TX operations on a queue. [not found] ` <4DE288DB.7040309@gmail.com> @ 2011-05-30 7:55 ` Helmut Schaa 2011-05-30 9:03 ` Johannes Berg 0 siblings, 1 reply; 4+ messages in thread From: Helmut Schaa @ 2011-05-30 7:55 UTC (permalink / raw) To: Gertjan van Wingerde; +Cc: users, johannes, linux-wireless [CC'ed Johannes & linux-wireless] Am Sonntag, 29. Mai 2011 schrieb Gertjan van Wingerde: > On 05/28/11 16:47, Helmut Schaa wrote: > > Am Samstag, 28. Mai 2011 schrieb Gertjan van Wingerde: > >>>> diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c > >>>> index f1e1381..2ace0f9 100644 > >>>> --- a/drivers/net/wireless/rt2x00/rt2x00queue.c > >>>> +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c > >>>> @@ -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; > >>>> + > >>>> + 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. > >> > >> 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). > > > >> 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [rt2x00-users] [PATCH 1/3] rt2x00: Serialize TX operations on a queue. 2011-05-30 7:55 ` [rt2x00-users] [PATCH 1/3] rt2x00: Serialize TX operations on a queue Helmut Schaa @ 2011-05-30 9:03 ` Johannes Berg 2011-05-30 9:32 ` Gertjan van Wingerde 0 siblings, 1 reply; 4+ messages in thread From: Johannes Berg @ 2011-05-30 9:03 UTC (permalink / raw) To: Helmut Schaa; +Cc: Gertjan van Wingerde, users, linux-wireless On Mon, 2011-05-30 at 09:55 +0200, Helmut Schaa wrote: > > >> 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). > > > > > >> 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. > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [rt2x00-users] [PATCH 1/3] rt2x00: Serialize TX operations on a queue. 2011-05-30 9:03 ` Johannes Berg @ 2011-05-30 9:32 ` Gertjan van Wingerde 2011-05-30 9:38 ` Johannes Berg 0 siblings, 1 reply; 4+ messages in thread From: Gertjan van Wingerde @ 2011-05-30 9:32 UTC (permalink / raw) To: Johannes Berg; +Cc: Helmut Schaa, users, linux-wireless On Mon, May 30, 2011 at 11:03 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2011-05-30 at 09:55 +0200, Helmut Schaa wrote: > >> > >> 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). >> > > >> > >> 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. > >> 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [rt2x00-users] [PATCH 1/3] rt2x00: Serialize TX operations on a queue. 2011-05-30 9:32 ` Gertjan van Wingerde @ 2011-05-30 9:38 ` Johannes Berg 0 siblings, 0 replies; 4+ messages in thread From: Johannes Berg @ 2011-05-30 9:38 UTC (permalink / raw) To: Gertjan van Wingerde; +Cc: Helmut Schaa, users, linux-wireless On Mon, 2011-05-30 at 11:32 +0200, Gertjan van Wingerde wrote: > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-05-30 9:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1306586311.git.gwingerde@gmail.com>
[not found] ` <201105281647.35855.helmut.schaa@googlemail.com>
[not found] ` <4DE288DB.7040309@gmail.com>
2011-05-30 7:55 ` [rt2x00-users] [PATCH 1/3] rt2x00: Serialize TX operations on a queue Helmut Schaa
2011-05-30 9:03 ` Johannes Berg
2011-05-30 9:32 ` Gertjan van Wingerde
2011-05-30 9:38 ` Johannes Berg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).