| Message ID | 201104181527.44542.IvDoorn@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi, At Mon, 18 Apr 2011 15:27:43 +0200, Ivo van Doorn wrote: > > From: Helmut Schaa <helmut.schaa-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> > > Allow passing a void pointer to rt2x00_queue_entry_for_each which in > turn in provided to the callback function. > > Furthermore, allow the callback function to stop processing by returning > true. And also notify the caller of rt2x00_queue_entry_for_each if the > loop was canceled by the callback. My colleague just tested this patch set and found out that the in question makes our 400MHz ARM cpu board with ralink usb dongle non functional due to high cpu consumption. it seems for us that exiting from function every time it finds an entry is too expensive on systems slower than PCs. To verify our thought, we changed the source code as the patch below. What we intended to do with this change is to continue processing all entry without breaking semantics. With the patch below our board seem to work fine again, but not sure exactly why it takes so much time to check the list again. We are not against the idea of the patch at all. We just want to ask you guys how we should go to track this problem. it might be the slow usb? Thanks, > diff -urN compat-wireless-2011-04-26/drivers/net/wireless/rt2x00/rt2x00queue.c compat-wireless-2011-04-26-armadillo/drivers/net/wireless/rt2x00/rt2x00queue.c > --- compat-wireless-2011-04-26/drivers/net/wireless/rt2x00/rt2x00queue.c 2011-04-27 04:04:26.000000000 +0900 > +++ compat-wireless-2011-04-26-armadillo/drivers/net/wireless/rt2x00/rt2x00queue.c 2011-04-27 17:13:45.000000000 +0900 > @@ -740,6 +740,7 @@ > unsigned int index_start; > unsigned int index_end; > unsigned int i; > + bool ret = false; > > if (unlikely(start >= Q_INDEX_MAX || end >= Q_INDEX_MAX)) { > ERROR(queue->rt2x00dev, > @@ -766,21 +767,21 @@ > if (index_start < index_end) { > for (i = index_start; i < index_end; i++) { > if (fn(&queue->entries[i], data)) > - return true; > + ret = true; > } > } else { > for (i = index_start; i < queue->limit; i++) { > if (fn(&queue->entries[i], data)) > - return true; > + ret = true; > } > > for (i = 0; i < index_end; i++) { > if (fn(&queue->entries[i], data)) > - return true; > + ret = true; > } > } > > - return false; > + return ret; > } > EXPORT_SYMBOL_GPL(rt2x00queue_for_each_entry);
Hi, At Mon, 18 Apr 2011 15:27:43 +0200, Ivo van Doorn wrote: > > From: Helmut Schaa <helmut.schaa-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> > > Allow passing a void pointer to rt2x00_queue_entry_for_each which in > turn in provided to the callback function. > > Furthermore, allow the callback function to stop processing by returning > true. And also notify the caller of rt2x00_queue_entry_for_each if the > loop was canceled by the callback. My colleague just tested this patch set and found out that the in question makes our 400MHz ARM cpu board with ralink usb dongle non functional due to high cpu consumption. it seems for us that exiting from function every time it finds an entry is too expensive on systems slower than PCs. To verify our thought, we changed the source code as the patch below. What we intended to do with this change is to continue processing all entry without breaking semantics. With the patch below our board seem to work fine again, but not sure exactly why it takes so much time to check the list again. We are not against the idea of the patch at all. We just want to ask you guys how we should go to track this problem. it might be the slow usb? Thanks, > diff -urN compat-wireless-2011-04-26/drivers/net/wireless/rt2x00/rt2x00queue.c compat-wireless-2011-04-26-armadillo/drivers/net/wireless/rt2x00/rt2x00queue.c > --- compat-wireless-2011-04-26/drivers/net/wireless/rt2x00/rt2x00queue.c 2011-04-27 04:04:26.000000000 +0900 > +++ compat-wireless-2011-04-26-armadillo/drivers/net/wireless/rt2x00/rt2x00queue.c 2011-04-27 17:13:45.000000000 +0900 > @@ -740,6 +740,7 @@ > unsigned int index_start; > unsigned int index_end; > unsigned int i; > + bool ret = false; > > if (unlikely(start >= Q_INDEX_MAX || end >= Q_INDEX_MAX)) { > ERROR(queue->rt2x00dev, > @@ -766,21 +767,21 @@ > if (index_start < index_end) { > for (i = index_start; i < index_end; i++) { > if (fn(&queue->entries[i], data)) > - return true; > + ret = true; > } > } else { > for (i = index_start; i < queue->limit; i++) { > if (fn(&queue->entries[i], data)) > - return true; > + ret = true; > } > > for (i = 0; i < index_end; i++) { > if (fn(&queue->entries[i], data)) > - return true; > + ret = true; > } > } > > - return false; > + return ret; > } > EXPORT_SYMBOL_GPL(rt2x00queue_for_each_entry);
Hi, > My colleague just tested this patch set and found out that the in > question makes our 400MHz ARM cpu board with ralink usb dongle non > functional due to high cpu consumption. it seems for us that exiting > from function every time it finds an entry is too expensive on systems > slower than PCs. Interesting, I would suspected the patch to reduce the CPU consumption rather then increasing it. I can do some testing regarding this looping during the weekend, but I haven't seen high CPU consumption on my system during my last test. However I wasn't testing on an embedded system... > To verify our thought, we changed the source code as the patch below. > What we intended to do with this change is to continue processing all > entry without breaking semantics. > > With the patch below our board seem to work fine again, but not sure > exactly why it takes so much time to check the list again. We are not > against the idea of the patch at all. We just want to ask you guys > how we should go to track this problem. it might be the slow usb? Could you try use debugfs and see if some queue and packet counters of mac80211/rt2x00 show excessive values? Have you been running the test with powersaving enabled or disabled? Ivo -- 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 04/28/11 20:55, Ivo Van Doorn wrote: > Hi, > >> My colleague just tested this patch set and found out that the in >> question makes our 400MHz ARM cpu board with ralink usb dongle non >> functional due to high cpu consumption. it seems for us that exiting >> from function every time it finds an entry is too expensive on systems >> slower than PCs. > > Interesting, I would suspected the patch to reduce the CPU consumption > rather then increasing it. I can do some testing regarding this looping > during the weekend, but I haven't seen high CPU consumption on my > system during my last test. However I wasn't testing on an embedded system... > >> To verify our thought, we changed the source code as the patch below. >> What we intended to do with this change is to continue processing all >> entry without breaking semantics. >> >> With the patch below our board seem to work fine again, but not sure >> exactly why it takes so much time to check the list again. We are not >> against the idea of the patch at all. We just want to ask you guys >> how we should go to track this problem. it might be the slow usb? > > Could you try use debugfs and see if some queue and packet counters > of mac80211/rt2x00 show excessive values? > > Have you been running the test with powersaving enabled or disabled? > Yeah, I had the same impression as Ivo, that it should save CPU cycles, especially since the patch was submitted by Helmut, who is very keen on saving CPU cycles, as he tests on an embedded (PCI-based) platform as well. But, if this proves to not be useful for now, then we should revert the entire patch, as the patch proposed simply negates the intended behavior and leaves a mess. I believe the offending patch was preparation for another patch which so far has not materialized in a usable form, so there should be no harm in reverting the offending patch. --- Gertjan -- 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/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c index d03eef2..458bb48 100644 --- a/drivers/net/wireless/rt2x00/rt2x00queue.c +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c @@ -650,10 +650,12 @@ int rt2x00queue_update_beacon(struct rt2x00_dev *rt2x00dev, return ret; } -void rt2x00queue_for_each_entry(struct data_queue *queue, +bool rt2x00queue_for_each_entry(struct data_queue *queue, enum queue_index start, enum queue_index end, - void (*fn)(struct queue_entry *entry)) + void *data, + bool (*fn)(struct queue_entry *entry, + void *data)) { unsigned long irqflags; unsigned int index_start; @@ -664,7 +666,7 @@ void rt2x00queue_for_each_entry(struct data_queue *queue, ERROR(queue->rt2x00dev, "Entry requested from invalid index range (%d - %d)\n", start, end); - return; + return true; } /* @@ -683,15 +685,23 @@ void rt2x00queue_for_each_entry(struct data_queue *queue, * send out all frames in the correct order. */ if (index_start < index_end) { - for (i = index_start; i < index_end; i++) - fn(&queue->entries[i]); + for (i = index_start; i < index_end; i++) { + if (fn(&queue->entries[i], data)) + return true; + } } else { - for (i = index_start; i < queue->limit; i++) - fn(&queue->entries[i]); + for (i = index_start; i < queue->limit; i++) { + if (fn(&queue->entries[i], data)) + return true; + } - for (i = 0; i < index_end; i++) - fn(&queue->entries[i]); + for (i = 0; i < index_end; i++) { + if (fn(&queue->entries[i], data)) + return true; + } } + + return false; } EXPORT_SYMBOL_GPL(rt2x00queue_for_each_entry); diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.h b/drivers/net/wireless/rt2x00/rt2x00queue.h index 6ae8200..6b66452 100644 --- a/drivers/net/wireless/rt2x00/rt2x00queue.h +++ b/drivers/net/wireless/rt2x00/rt2x00queue.h @@ -580,16 +580,22 @@ struct data_queue_desc { * @queue: Pointer to @data_queue * @start: &enum queue_index Pointer to start index * @end: &enum queue_index Pointer to end index + * @data: Data to pass to the callback function * @fn: The function to call for each &struct queue_entry * * This will walk through all entries in the queue, in chronological * order. This means it will start at the current @start pointer * and will walk through the queue until it reaches the @end pointer. + * + * If fn returns true for an entry rt2x00queue_for_each_entry will stop + * processing and return true as well. */ -void rt2x00queue_for_each_entry(struct data_queue *queue, +bool rt2x00queue_for_each_entry(struct data_queue *queue, enum queue_index start, enum queue_index end, - void (*fn)(struct queue_entry *entry)); + void *data, + bool (*fn)(struct queue_entry *entry, + void *data)); /** * rt2x00queue_empty - Check if the queue is empty. diff --git a/drivers/net/wireless/rt2x00/rt2x00usb.c b/drivers/net/wireless/rt2x00/rt2x00usb.c index 94047e9..0bc8dcc 100644 --- a/drivers/net/wireless/rt2x00/rt2x00usb.c +++ b/drivers/net/wireless/rt2x00/rt2x00usb.c @@ -230,7 +230,7 @@ static void rt2x00usb_interrupt_txdone(struct urb *urb) queue_work(rt2x00dev->workqueue, &rt2x00dev->txdone_work); } -static void rt2x00usb_kick_tx_entry(struct queue_entry *entry) +static bool rt2x00usb_kick_tx_entry(struct queue_entry *entry, void* data) { struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev; struct usb_device *usb_dev = to_usb_device_intf(rt2x00dev->dev); @@ -240,7 +240,7 @@ static void rt2x00usb_kick_tx_entry(struct queue_entry *entry) if (!test_and_clear_bit(ENTRY_DATA_PENDING, &entry->flags) || test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags)) - return; + return true; /* * USB devices cannot blindly pass the skb->len as the @@ -261,6 +261,8 @@ static void rt2x00usb_kick_tx_entry(struct queue_entry *entry) set_bit(ENTRY_DATA_IO_FAILED, &entry->flags); rt2x00lib_dmadone(entry); } + + return false; } /* @@ -323,7 +325,7 @@ static void rt2x00usb_interrupt_rxdone(struct urb *urb) queue_work(rt2x00dev->workqueue, &rt2x00dev->rxdone_work); } -static void rt2x00usb_kick_rx_entry(struct queue_entry *entry) +static bool rt2x00usb_kick_rx_entry(struct queue_entry *entry, void* data) { struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev; struct usb_device *usb_dev = to_usb_device_intf(rt2x00dev->dev); @@ -332,7 +334,7 @@ static void rt2x00usb_kick_rx_entry(struct queue_entry *entry) if (test_and_set_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags) || test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags)) - return; + return true; rt2x00lib_dmastart(entry); @@ -348,6 +350,8 @@ static void rt2x00usb_kick_rx_entry(struct queue_entry *entry) set_bit(ENTRY_DATA_IO_FAILED, &entry->flags); rt2x00lib_dmadone(entry); } + + return false; } void rt2x00usb_kick_queue(struct data_queue *queue) @@ -358,12 +362,18 @@ void rt2x00usb_kick_queue(struct data_queue *queue) case QID_AC_BE: case QID_AC_BK: if (!rt2x00queue_empty(queue)) - rt2x00queue_for_each_entry(queue, Q_INDEX_DONE, Q_INDEX, + rt2x00queue_for_each_entry(queue, + Q_INDEX_DONE, + Q_INDEX, + NULL, rt2x00usb_kick_tx_entry); break; case QID_RX: if (!rt2x00queue_full(queue)) - rt2x00queue_for_each_entry(queue, Q_INDEX_DONE, Q_INDEX, + rt2x00queue_for_each_entry(queue, + Q_INDEX_DONE, + Q_INDEX, + NULL, rt2x00usb_kick_rx_entry); break; default: @@ -372,14 +382,14 @@ void rt2x00usb_kick_queue(struct data_queue *queue) } EXPORT_SYMBOL_GPL(rt2x00usb_kick_queue); -static void rt2x00usb_flush_entry(struct queue_entry *entry) +static bool rt2x00usb_flush_entry(struct queue_entry *entry, void* data) { struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev; struct queue_entry_priv_usb *entry_priv = entry->priv_data; struct queue_entry_priv_usb_bcn *bcn_priv = entry->priv_data; if (!test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags)) - return; + return true; usb_kill_urb(entry_priv->urb); @@ -389,6 +399,8 @@ static void rt2x00usb_flush_entry(struct queue_entry *entry) if ((entry->queue->qid == QID_BEACON) && (test_bit(REQUIRE_BEACON_GUARD, &rt2x00dev->cap_flags))) usb_kill_urb(bcn_priv->guardian_urb); + + return false; } void rt2x00usb_flush_queue(struct data_queue *queue) @@ -396,7 +408,7 @@ void rt2x00usb_flush_queue(struct data_queue *queue) struct work_struct *completion; unsigned int i; - rt2x00queue_for_each_entry(queue, Q_INDEX_DONE, Q_INDEX, + rt2x00queue_for_each_entry(queue, Q_INDEX_DONE, Q_INDEX, NULL, rt2x00usb_flush_entry); /* @@ -489,7 +501,7 @@ void rt2x00usb_clear_entry(struct queue_entry *entry) entry->flags = 0; if (entry->queue->qid == QID_RX) - rt2x00usb_kick_rx_entry(entry); + rt2x00usb_kick_rx_entry(entry, NULL); } EXPORT_SYMBOL_GPL(rt2x00usb_clear_entry);