Re: [rt2x00-users] [PATCH 1/3] rt2x00: Serialize TX operations on a queue.

Re: [rt2x00-users] [PATCH 1/3] rt2x00: Serialize TX operations on a queue.

[Helmut Schaa]

[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

Re: [rt2x00-users] [PATCH 1/3] rt2x00: Serialize TX operations on a queue.

[Johannes Berg]

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

Re: [rt2x00-users] [PATCH 1/3] rt2x00: Serialize TX operations on a queue.

[Gertjan van Wingerde]

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

Re: [rt2x00-users] [PATCH 1/3] rt2x00: Serialize TX operations on a queue.

[Johannes Berg]

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