On Tue, 1 May 2007 04:01:00 +0100 (BST), Daniel Drake wrote:
> The old code allowed unlimited buffing of tx frames in URBs
> submitted for transfer to the device. This patch stops the
> ieee80211_hw queue(s) if to many URBs are ready for submit to the
> device. Actually the ZD1211 device supports currently only one
> queue.
This doesn't look correct to me. The limits should be per queue and you
should always stop queues selectively.
The ieee80211_stop_queues/ieee80211_wake_queues are for another purposes
and should be used only rarely - one example is Michael Buesch's comment
about a need to stop sending frames while tuning to a new channel in a
workqueue.
> +/**
> + * wake_queues - wakes all queues
> + * @hw: a &struct ieee80211_hw pointer
> + *
> + * Such a function is not provided by mac80211, so we have to provide them on
> + * our own.
> + */
> +static void wake_queues(struct ieee80211_hw *hw)
It is :-) Look for ieee80211_wake_queues. But as I said, you shouldn't need
that.
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
On Wed, 2 May 2007 00:40:45 +0200, Ulrich Kunitz wrote:
> Jiri, even if ZD1211B supports multiple queues, there is only one
> USB endpoint receiving packets for transmission. I suppose that
> the queue for the packet can be set in the control data for the
> packet. However if the device doesn't read USB blocks anymore, all
> queues will be affected.
Ah, that's stupid. I hope nobody is advertising these devices have a decent
QoS support.
> At this point in time I have to stop all
> queues. Sure I could care for the different priorities of the
> queues by stopping low-priority queues earlier, but if the user is
> overloading the device, I have to stop all queues.
Or you can split the available buffer space in the device, i.e. reserve
some fixed part for the first queue, some part for the second queue, etc.
Otherwise, the QoS is not so useful (you will still profit from the shorter
backoff, of course, so it's not completely useless).
But I don't know how many urbs the device can accept, it may not be
feasible to do that. (But if it can accept reasonable amount of urbs, I'd
suggest to implement that. It will need some testing to see whether it
really improves something or not, though.)
> > If you support one queue only, call ieee80211_stop_queue(hw, 0). Calling
> > ieee80211_stop_queues if you have just a full queue is wrong.
>
> Again I don't just have a full queue, I cannot send any packets to
> the device anymore. Under this condition I simply stop all queues,
> not caring whether there are 1 or 1024.
Okay, ACK then.
Thanks for the clarification,
Jiri
--
Jiri Benc
SUSE Labs
On Tue, 1 May 2007 21:50:08 +0200, Ulrich Kunitz wrote:
> On 07-05-01 12:34 Jiri Benc wrote:
> > On Tue, 1 May 2007 04:01:00 +0100 (BST), Daniel Drake wrote:
> > > The old code allowed unlimited buffing of tx frames in URBs
> > > submitted for transfer to the device. This patch stops the
> > > ieee80211_hw queue(s) if to many URBs are ready for submit to the
> > > device. Actually the ZD1211 device supports currently only one
> > > queue.
> >
> > This doesn't look correct to me. The limits should be per queue and you
> > should always stop queues selectively.
>
> The old ZD1211 chip doesn't support queuing and the new ZD1211B
> chip has support, but it is unclear how to put packets in the
> different queues. However the error condition here is, that
> packets can't be transmitted over the USB, which will affect all
> queues.
Really? From what you wrote ("if too many URBs are ready for submit") it
seems that the code is triggered when the queue is just full. That's not
necessarily an error condition and the only thing needed to do is to stop
the queue. Unless zd1211 is really special here (and then I'd like to know
how is it special).
> Sure one could manage different high level marks for the
> different queues, but this is all theoretical currently. I could
> have coded with the explicit knowledge that we support only one
> queue, but it is really work the hassle.
If you support one queue only, call ieee80211_stop_queue(hw, 0). Calling
ieee80211_stop_queues if you have just a full queue is wrong.
Thanks,
Jiri
--
Jiri Benc
SUSE Labs
On Tue, May 01, 2007 at 09:50:08PM +0200, Ulrich Kunitz wrote:
> On 07-05-01 12:34 Jiri Benc wrote:
> > It is :-) Look for ieee80211_wake_queues. But as I said, you shouldn't need
> > that.
>
> I provided the patch to add ieee80211_wake_queues(). Sorry for
> missing the moment, when it has been integrated. I will update the
> patch.
No need, I have adjusted the patch accordingly.
Thanks,
John
--
John W. Linville
[email protected]
On 07-05-01 12:34 Jiri Benc wrote:
> On Tue, 1 May 2007 04:01:00 +0100 (BST), Daniel Drake wrote:
> > The old code allowed unlimited buffing of tx frames in URBs
> > submitted for transfer to the device. This patch stops the
> > ieee80211_hw queue(s) if to many URBs are ready for submit to the
> > device. Actually the ZD1211 device supports currently only one
> > queue.
>
> This doesn't look correct to me. The limits should be per queue and you
> should always stop queues selectively.
The old ZD1211 chip doesn't support queuing and the new ZD1211B
chip has support, but it is unclear how to put packets in the
different queues. However the error condition here is, that
packets can't be transmitted over the USB, which will affect all
queues. Sure one could manage different high level marks for the
different queues, but this is all theoretical currently. I could
have coded with the explicit knowledge that we support only one
queue, but it is really work the hassle.
> > +/**
> > + * wake_queues - wakes all queues
> > + * @hw: a &struct ieee80211_hw pointer
> > + *
> > + * Such a function is not provided by mac80211, so we have to provide them on
> > + * our own.
> > + */
> > +static void wake_queues(struct ieee80211_hw *hw)
>
> It is :-) Look for ieee80211_wake_queues. But as I said, you shouldn't need
> that.
I provided the patch to add ieee80211_wake_queues(). Sorry for
missing the moment, when it has been integrated. I will update the
patch.
--
Uli Kunitz
On 07-05-01 23:10 Jiri Benc wrote:
> Really? From what you wrote ("if too many URBs are ready for submit") it
> seems that the code is triggered when the queue is just full. That's not
> necessarily an error condition and the only thing needed to do is to stop
> the queue. Unless zd1211 is really special here (and then I'd like to know
> how is it special).
Jiri, even if ZD1211B supports multiple queues, there is only one
USB endpoint receiving packets for transmission. I suppose that
the queue for the packet can be set in the control data for the
packet. However if the device doesn't read USB blocks anymore, all
queues will be affected. At this point in time I have to stop all
queues. Sure I could care for the different priorities of the
queues by stopping low-priority queues earlier, but if the user is
overloading the device, I have to stop all queues.
> If you support one queue only, call ieee80211_stop_queue(hw, 0). Calling
> ieee80211_stop_queues if you have just a full queue is wrong.
Again I don't just have a full queue, I cannot send any packets to
the device anymore. Under this condition I simply stop all queues,
not caring whether there are 1 or 1024. Sure with the explicit
knowledge that I have only one queue, I could stop it explicitly,
but this would require a change of the code as soon as a second
queue comes into play.
--
Uli Kunitz