| Message ID | 1365444377-9959-3-git-send-email-thomas@cozybit.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Thomas, On Mon, Apr 08, 2013 at 11:06:14AM -0700, Thomas Pedersen wrote: > Have ieee80211_queue_stopped() return a bitmap of enum > queue_stop_reasons while checking queue status. This is > handy when we care about the queue stop reason codes. > > Signed-off-by: Thomas Pedersen <thomas@cozybit.com> > --- > include/net/mac80211.h | 10 +++++++--- > net/mac80211/util.c | 8 ++++---- > 2 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/include/net/mac80211.h b/include/net/mac80211.h > index 64faf01..8bbe799 100644 > --- a/include/net/mac80211.h > +++ b/include/net/mac80211.h > @@ -3589,15 +3589,19 @@ void ieee80211_stop_queue(struct ieee80211_hw *hw, int queue); > > /** > * ieee80211_queue_stopped - test status of the queue > + * I think you added this new line by mistake. Cheers,
On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote: > Have ieee80211_queue_stopped() return a bitmap of enum > queue_stop_reasons while checking queue status. This is > handy when we care about the queue stop reason codes. Nack. The driver must not care about the reasons, they're purely internal to mac80211. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 8, 2013 at 11:37 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote: >> Have ieee80211_queue_stopped() return a bitmap of enum >> queue_stop_reasons while checking queue status. This is >> handy when we care about the queue stop reason codes. > > Nack. The driver must not care about the reasons, they're purely > internal to mac80211. OK those functions are exported to the drivers. The 'enum queue_stop_reason' is defined in ieee80211_i.h, so the driver wouldn't be able to interpret them anyway? Would you prefer a utility function internal to mac80211 which does return the reason, or just the following pattern? if (ieee80211_queue_stopped(hw, queue)) { qreason = hw_to_local(hw)->queue_stop_reasons[queue]; if (qreason & ~(ALLOWED_QUEUE_STOP_REASONS)) something; } Thanks,
On Mon, 2013-04-08 at 12:30 -0700, Thomas Pedersen wrote: > OK those functions are exported to the drivers. The 'enum > queue_stop_reason' is defined in ieee80211_i.h, so the driver wouldn't > be able to interpret them anyway? Oh, you're way underestimating the creativity of driver authors :-) > Would you prefer a utility function internal to mac80211 which does > return the reason, That seems fine, although a bit more inefficient? > or just the following pattern? > > if (ieee80211_queue_stopped(hw, queue)) { > qreason = hw_to_local(hw)->queue_stop_reasons[queue]; > if (qreason & ~(ALLOWED_QUEUE_STOP_REASONS)) > something; > } That seems racy? Even asking whether it's stopped is racy though, what are you even trying to accomplish? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 8, 2013 at 12:38 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2013-04-08 at 12:30 -0700, Thomas Pedersen wrote: > >> OK those functions are exported to the drivers. The 'enum >> queue_stop_reason' is defined in ieee80211_i.h, so the driver wouldn't >> be able to interpret them anyway? > > Oh, you're way underestimating the creativity of driver authors :-) > >> Would you prefer a utility function internal to mac80211 which does >> return the reason, > > That seems fine, although a bit more inefficient? > >> or just the following pattern? >> >> if (ieee80211_queue_stopped(hw, queue)) { >> qreason = hw_to_local(hw)->queue_stop_reasons[queue]; >> if (qreason & ~(ALLOWED_QUEUE_STOP_REASONS)) >> something; >> } > > That seems racy? Even asking whether it's stopped is racy though, what > are you even trying to accomplish? Yeah, that's why I'd like to get the reason the first time. _h_mesh_fwding() checks whether the outgoing queue is stopped, to avoid piling frames on the pending queue if the outgoing medium is busy. I guess the idea was to avoid queueing frames faster than the hardware could unload them. Maybe this doesn't actually happen, but we can be a little bit smarter about when to drop forwarded frames. Like if skbs are just being added to the outgoing queue, we probably shouldn't. This patch and the next one show a small improvement in throughput and loss % in the forwarding path.
On Mon, 2013-04-08 at 12:57 -0700, Thomas Pedersen wrote: > > That seems racy? Even asking whether it's stopped is racy though, what > > are you even trying to accomplish? > > Yeah, that's why I'd like to get the reason the first time. > > _h_mesh_fwding() checks whether the outgoing queue is stopped, to > avoid piling frames on the pending queue if the outgoing medium is > busy. I guess the idea was to avoid queueing frames faster than the > hardware could unload them. Maybe this doesn't actually happen, but we > can be a little bit smarter about when to drop forwarded frames. Like > if skbs are just being added to the outgoing queue, we probably > shouldn't. Technically I guess that can happen if your inbound link is better than the outbound one? But it'll depend on the AC parameters etc. too. However it seems that ieee80211_queue_stopped() is actually kinda broken and should only return the 'driver-stopped' reason to start with. If you fix that, it's probably good enough for you. I don't think in patch 4 you should drop frames for scanning, for example. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 9, 2013 at 2:37 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2013-04-08 at 12:57 -0700, Thomas Pedersen wrote: > >> > That seems racy? Even asking whether it's stopped is racy though, what >> > are you even trying to accomplish? >> >> Yeah, that's why I'd like to get the reason the first time. >> >> _h_mesh_fwding() checks whether the outgoing queue is stopped, to >> avoid piling frames on the pending queue if the outgoing medium is >> busy. I guess the idea was to avoid queueing frames faster than the >> hardware could unload them. Maybe this doesn't actually happen, but we >> can be a little bit smarter about when to drop forwarded frames. Like >> if skbs are just being added to the outgoing queue, we probably >> shouldn't. > > Technically I guess that can happen if your inbound link is better than > the outbound one? But it'll depend on the AC parameters etc. too. > > However it seems that ieee80211_queue_stopped() is actually kinda broken > and should only return the 'driver-stopped' reason to start with. If you > fix that, it's probably good enough for you. I don't think in patch 4 > you should drop frames for scanning, for example. It looks like mac80211 already just checks local->queue_stop_reasons for being 0 (or not), so that check would still hold. I'm not 100% sure the driver calls to ieee80211_queue_stopped() only returning true for REASON_DRIVER are ok? What if the queues are being flushed, or say REASON_PS is set?
On Wed, 2013-04-10 at 10:36 -0700, Thomas Pedersen wrote: > > However it seems that ieee80211_queue_stopped() is actually kinda broken > > and should only return the 'driver-stopped' reason to start with. If you > > fix that, it's probably good enough for you. I don't think in patch 4 > > you should drop frames for scanning, for example. > > It looks like mac80211 already just checks local->queue_stop_reasons > for being 0 (or not), so that check would still hold. I'm not 100% > sure the driver calls to ieee80211_queue_stopped() only returning true > for REASON_DRIVER are ok? What if the queues are being flushed, or say > REASON_PS is set? I looked at the drivers, and while (almost?) all of them use it incorrectly they really want only REASON_DRIVER. In fact, they'd be better off with just that even in the incorrect usages. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 64faf01..8bbe799 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -3589,15 +3589,19 @@ void ieee80211_stop_queue(struct ieee80211_hw *hw, int queue); /** * ieee80211_queue_stopped - test status of the queue + * * @hw: pointer as obtained from ieee80211_alloc_hw(). * @queue: queue number (counted from zero). * * Drivers should use this function instead of netif_stop_queue. * - * Return: %true if the queue is stopped. %false otherwise. + * Return 0 if queue not stopped, or else a bitmap of + * queue_stop_reasons. + * + * If @queue doesn't exist, return -1UL. + * */ - -int ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue); +unsigned long ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue); /** * ieee80211_stop_queues - stop all queues diff --git a/net/mac80211/util.c b/net/mac80211/util.c index 447e665..739cddb 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -475,17 +475,17 @@ void ieee80211_stop_queues(struct ieee80211_hw *hw) } EXPORT_SYMBOL(ieee80211_stop_queues); -int ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue) +unsigned long ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue) { struct ieee80211_local *local = hw_to_local(hw); unsigned long flags; - int ret; + unsigned long ret; if (WARN_ON(queue >= hw->queues)) - return true; + return -1UL; spin_lock_irqsave(&local->queue_stop_reason_lock, flags); - ret = !!local->queue_stop_reasons[queue]; + ret = local->queue_stop_reasons[queue]; spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags); return ret; }
Have ieee80211_queue_stopped() return a bitmap of enum queue_stop_reasons while checking queue status. This is handy when we care about the queue stop reason codes. Signed-off-by: Thomas Pedersen <thomas@cozybit.com> --- include/net/mac80211.h | 10 +++++++--- net/mac80211/util.c | 8 ++++---- 2 files changed, 11 insertions(+), 7 deletions(-)